GrumpyOldTroll / quiche

BSD 3-Clause "New" or "Revised" License
1 stars 0 forks source link

Unsigned (size_t) minus signed (int literal) result #8

Open squarooticus opened 2 years ago

squarooticus commented 2 years ago

Does code like this:

ssize_t parse_u32(size_t size, const uint8_t* buf, uint32_t& val) {
  if (size < 4) {
    return size - 4;
  }

do what is intended, which is to return a negative value when size < 4? Or does a value > MAX_INT get reinterpreted as the proper negative value when cast to ssize_t on return? I tend to avoid these kinds of operations precisely because I'm unsure of the exact C sematics.

squarooticus commented 2 years ago

Ok, I finally bothered to check what the compiler does in cases like these, and in this particular case (unsigned cast to signed on return) warns with -Wconversion, but does the right thing for this code to work. So maybe explicitly cast to prevent such warnings (though -Wno-sign-conversion silences this).

GrumpyOldTroll commented 2 years ago

Yeah, good question, if there's a chance this goes wrong that could be ugly. I thought this was well defined because subtracting 4 makes the result signed, but now that I see you asked I'm not so sure.

I guess the idea here would be to cast size before the subtraction, right?

Otherwise, if we somehow got a compiler saying "size - 4" is an unsigned 32-bit number that wrapped, casting that result to a 64-bit signed integer would give a large positive value instead of a small negative, so an explicit cast that did the same thing would just remove the warning, not remove the bad behavior.

GrumpyOldTroll commented 2 years ago

Note: this code goes away once we move to protobuf, so we should close this with the PR that closes https://github.com/GrumpyOldTroll/quiche/issues/1