Strategy for fixing signed/unsigned warnings

Question

I'm working on obtaining a clean compile with -Wsign-conversion for existing library code. The library is 25 years old or so. Eventually, this higher bar will be a security gate, and all check-ins will need to meet it or be rejected.

The existing library code does a fair amount of the following (a gross simplification that captures the essence):

class SHA1 : <inherited interfaces (contracts) and classes (behaviors)>
{
    ...
    enum {DIGEST_SIZE = 20U};
    unsigned int MaxDigestSize() { return DIGEST_SIZE; }
    ...
    int m_digestSize;
}

Then, the library does things like (the interaction is more complex, but this captures the essence):

if(m_digestSize == -1)
    m_digestSize = MaxDiegstSize();

External callers may use it like:

SHA1 sha;
...

// Calculate truncated digest
sha.Final(buffer, 8 /*signed or unsigned*/);
// Calculate using full digest
sha.Final(buffer, -1 /*signed*/);
// Calculate using full digest
sha.Final(buffer, sha.MaxDigestSize() /*unsigned*/);

The reason -1 is popular is because the following "just works" (notice the switch from SHA-1 to SHA-512):

SHA512 sha;

// Calculate using full digest
sha.Final(buffer, -1 /*signed*/);

For the library, we want to convert to unsigned types to squash the warning. Then, setup an enum {DEFAULT_DIGEST_VALUE=...}; to handle the case of "default digest" that was compatible with the existing "-1 pattern". Finally, remove the compares to -1 and use a compare with DEFAULT_DIGEST_VALUE.

But enum {DEFAULT_DIGEST_VALUE=std::numeric_limits<unsigned int>::max()}; turned out to be a dead end due to limitations in numeric_limits.

Someone suggested to use UINT_MAX, but I'm not sure if I should use it (OS X lacks <cstdint>, so it introduces a C header dependency). Other potential candidates are __INT_MAX__ * 2U + 1 (from <limits> on OS X), and ~(0) (popular in lots of library code, but maybe not correct). In fact, I'm not sure this is the proper path in general.

What is the proper or best way to:

• switch to unsigned for consistency and eventual security gate
• handle the "-1 pattern" for existing, non-library callers
• provide a portable, C++ solution
• address potential issues with bit patterns

---

My apologies for asking a simple question. I am a security architect by trade. I need the help of the software architects because I don't have the software engineering experience.

---

From the comments, here's some more information regarding constraints:

Existing Code - The library already exists, and its called Crypto++. It was authored in the 1990s, and it enjoys continued development. It is cross platform, and runs on the BSDs, Linux, OS X, Solaris, Windows. The solution needs to support older standards like C++98 and C++03. We can drop support for C++98 if the community agrees.

ABI Changes - we can change the API if the community agrees. I'm fairly certain that will happen when we make the policy change of a "clean compile as a security gate." We don't need to have 100% compatibility, but we do need to handle an existing code base of users using -1 in the field. We can even give users warning to modify their code with the preprocessor macro CRYPTOPP_MAINTAIN_BACKWARDS_COMPATIBILITY. But in the end, things have to "just work" for them.

Answer 1

Well from my point of view, you're trying to fix the wrong problem. You use the error-code "-1" to signalize that the MaxDigestSize should be used. This is a technique that Robert C. Martin calls "Mental mapping" in his book Clean Code. Your signed/unsigned conflict is solely caused by the fact that you're using this "error-code". So the problem that you should be fixing instead is "How do I avoid using an error-code".

Lets get into it:

Easiest approach: make m_digestSize and the parameter for the sha.Final() both unsigned ints. This ensures that the Final method can only accept valid values. Now instead of giving your method an error-code, just overload with a default-parameter like:

Final(byte* buffer, unsigned int digest = DEFAULT_DIGEST);

In the example you provided this DEFAULT_DIGEST is based on the immutable value of MaxDigestSize() and would therefore be 20.

EDIT:

Based on the new information in the comments and edits: It appears to me that the big problems are the warnings. Best solution: check for -1 before assigning the parameter to the member variable. Additionally, as mentioned in the comments, you should wrap the primitive into a small structure and offer that as an alternative while marking the old function as deprecated:

Final(byte* buffer, int digest = DEFAULT_DIGEST) // Mark as deprecated
{
    if(digest < MINIMUM_DIGEST)
    {
        digest = DEFAULT_DIGEST;
    }
    m_digestSize = static_cast<unsigned int>(digest);
    [...]
}

Final(byte* buffer, Digest digest = Digest())
{
    m_digestSize = digest;
    [...]
}

class Digest
{
    public:
    Digest(usigned int value = DEFAULT_DIGEST)
    :digestValue(value){};

    unsigned int GetValue()
    {
        return digestValue
    };

    private:
    unsigned int digestValue;
}

Answer 2

This is really more a case of the design of an API being more idiomatic C (and not even good idiomatic C at that) rather than idiomatic C++.

If your callers are explicitly plugging in -1 or the result of MaxDigestSize(), it means they know ahead of time that they're using that default value. It also means they can leverage the C++ concept of a default value for an argument:

class SHA {
public:
  static const size_t MAX_DIGEST_SIZE = 20;
   ...
  Final(Buffer buffer, size_t digest_size = MAX_DIGEST_SIZE);
};

Callers wanting the largest digest invoke Final(buffer) and those wanting shorter ones invoke Final(buffer, digest_size). The important thing here is that this keeps all understanding of the default behavior inside the class and eliminates magic numbers and the signed escape hatch being used to support them.

You will, of course, have to go through all invocations of Final() and change the ones using -1 or MaxDigestSize() to drop their second arguments, but this is a good thing because it gets the problem children out of the code base. It's tedious, but it's worth doing.

If you're constrained by not being able to remove these things from the existing code, you will not be able to switch to unsigned values and expect warning-free compilation. The best way to proceed in this case would be to have Final() validate its input and throw an exception if it's out of the expected bounds. (The code should be doing this anyway for safety.)

Answer 3

It looks like you have C code, so forget c++ifying it and stick with what it is.

Converting all to unsigned is a good idea, getting your types right is a good thing. I would take the opportunity to remove int and replace with a known size type, such a long or __int32 so you know how many bytes of data are being manipulated (int is usually 32-bits, but sometimes isn't). I'd say that was important for security software.

Next, if you knwo your int is 32-bits, you can set your max_digest to 0xffffffff (which is what ~0 is doing, its inverting each bit, so all the 0s become 1s - of 0x0 becomes 0xffffffff, without having to know how many bits are involved)