5

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.

  • Just so I get this right: You are writing this kind of library code, that is being used cross-plattform by users whos code should remain compatible, but may cause a warning in future? – LorToso Jul 8 '15 at 9:18
  • Can you clarify whether or not you're at liberty to change the API and the code in the callers? Or is the goal here 100% compatibility with existing code? – Blrfl Jul 8 '15 at 10:10
  • LosToro and Blrfl. Thanks. I added additional information to the question. If you guys (or gals) had questions, then others may have them too. – jww Jul 8 '15 at 11:02
  • 1
    If you're struggling to compile with high compiler warnings, then wait until you try running a static analysis in the code... – glampert Jul 9 '15 at 2:33
  • 1
    @glampert - Yea, I know. I recently got to a clean compile with -Wall. I'm slowly working my way to -Wextra and -Wconversion. Its sooo painful when a project does not start with them :) Both static and dynamic analysis are steps we have planned for engineering process improvements. – jww Jul 9 '15 at 4:45
5

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;
}
  • OK, thanks. But what about existing user code where we need to respond properly to -1? (And to be clear, its not an error-code. Its just a distinguished value, like a sentinel). – jww Jul 8 '15 at 7:13
  • Related, I was also thinking about a mini-type or mini-class (is that the correct term?) like size_type. If it was a lightweight class with no runtime overhead, then it could have an overloaded constructor that performed the conversion for the user. I.e., it could take a -1 and turn it into 20 (or whatever the appropriate value is). I think part of using this is meta-programming or meta-template programming, where we make all these decisions at compile time. – jww Jul 8 '15 at 7:19
  • Wrapping the primitive type is not only a good idea, it is best practice as can be read in rule 3 of Jeff Bays Object Calisthenics. Still, (in my eyes) using a sentinel should be avoided where possible. – LorToso Jul 8 '15 at 7:26
3

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.)

  • "...have Final() validate its input and throw an exception if it's out of the expected bounds..." - yeah, this is where the library runs fast and loose at times. Its not egregious, but it does cause some discomfort (q.v.). Validation does occur, but its not that strict. – jww Jul 8 '15 at 16:58
  • 1
    @jww: That kind of thing on the part of some of my sloppier colleagues means more job security for you and yours. :-) Seriously, if you're already under the hood, this is a good time to get in and straighten some of that out. – Blrfl Jul 8 '15 at 17:41
2

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)

  • I would love to use sizes from stdint.h or cstdint, but I don't think they will go for it. I'm beginning to worry the properly handling of the distinguished value -1 to denote "max size" is going to be a problem (especially if we need something like a crypto_size type to handle the conversion properly with meta-programming). – jww Jul 8 '15 at 8:35
  • define it yourself. 32bits is 32 bits in any compiler, even ones that don't define it for you. -1 was being used to pretend to be 0xFFFFFFFF anyway. – gbjbaanb Jul 8 '15 at 8:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.