C++ – Strategy for fixing signed/unsigned warnings

c

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.

Best Answer

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;
}
Related Topic