On 09 February 2018 15:56 Jean-Marc Valin wrote:> Pointers are unsigned so this shouldn't be an issue. I suspect you're > being hit by something else. That or your compiler is really broken.I don't know how important it is in this case (probably pretty minor) but in general Ruikai's right. It doesn't matter that pointers are unsigned; that fact that a pointer could have a "large" value like 0xffff_ff00 means that they can wrap if you do length checks the wrong way. The behaviour is completely defined - it just causes the code not to work as intended. The "bad" way of doing a length check is char* buf_start, buf_end; unsigned len_to_check; if (buf_start + len_to_check > buf_end) fail() Because the length is to-be-checked, it could have an unsafe large value, causing an (unsigned) overflow. For example, with buf_start = 0xffff_ff00 and buf_end = 0xffff_ff10, the maximum allowed length is 0x10, but a length of 0x100 will cause an overflow and bypass the check. The safe way of doing a length check is if (buf_end - buf_start < len_to_check) fail() The buffer bounds are known safe, so the arithmetic is OK to do that way round. Nick
Yes, I agree that buf_end - buf_start < len_to_check is better. It's the 0xFFFFFFFF overflow that's a cause of concern, not the 0x80000000. That being said, I believe that the length argument in this case can be trusted since it comes from the application and not from the user. Cheers, Jean-Marc On 02/12/2018 05:28 AM, Nicholas Wilson wrote:> On 09 February 2018 15:56 Jean-Marc Valin wrote: >> Pointers are unsigned so this shouldn't be an issue. I suspect you're >> being hit by something else. That or your compiler is really broken. > > I don't know how important it is in this case (probably pretty minor) but in general Ruikai's right. > > It doesn't matter that pointers are unsigned; that fact that a pointer could have a "large" value like 0xffff_ff00 means that they can wrap if you do length checks the wrong way. The behaviour is completely defined - it just causes the code not to work as intended. > > The "bad" way of doing a length check is > > char* buf_start, buf_end; > unsigned len_to_check; > if (buf_start + len_to_check > buf_end) > fail() > > Because the length is to-be-checked, it could have an unsafe large value, causing an (unsigned) overflow. For example, with buf_start = 0xffff_ff00 and buf_end = 0xffff_ff10, the maximum allowed length is 0x10, but a length of 0x100 will cause an overflow and bypass the check. > > The safe way of doing a length check is > > if (buf_end - buf_start < len_to_check) > fail() > > The buffer bounds are known safe, so the arithmetic is OK to do that way round. > > Nick >
On 02/12/2018 05:28 AM, Nicholas Wilson wrote:> The "bad" way of doing a length check is > > char* buf_start, buf_end; > unsigned len_to_check; > if (buf_start + len_to_check > buf_end) > fail() > > Because the length is to-be-checked, it could have an unsafe large value, causing an (unsigned) overflow. For example, with buf_start = 0xffff_ff00 and buf_end = 0xffff_ff10, the maximum allowed length is 0x10, but a length of 0x100 will cause an overflow and bypass the check.Right. And in fact the C standard says that merely computing the expression ptr+large_value is undefined (when the result points beyond the buffer) even if you don't even use the pointer.> The safe way of doing a length check is > > if (buf_end - buf_start < len_to_check) > fail() > > The buffer bounds are known safe, so the arithmetic is OK to do that way round.Yes, I agree. That would be the right way to make the check. Cheers, Jean-Marc