[openssl-dev] [openssl.org #4094] Nonsensical pointer comparison in PACKET_buf_init

[Matt Caswell via RT]

On 15/10/15 20:53, Alexander Cherepanov via RT wrote:
> On 2015-10-15 15:41, Matt Caswell via RT wrote:
>> The purpose of the sanity check is not then for security, but to guard
>> against programmer error. For a correctly functioning program this test
>> should never fail. For an incorrectly functioning program it may do. It
>> is not guaranteed to fail because the test could be compiled away but,
>> most likely, it will. We can have some degree of confidence that the
>> test works and does not get compiled away in most instances because, as
>> you point out, an explicit check for it appears in packettest.c and, to
>> date, we have had no reported failures.
> 
> What was not entirely clear from the original bug report is that, while 
> the check is not compiled away, it's compiled into something completely 
> different from what is written in the source. Specifically, the check 
> "buf + len < buf" is optimized into "len >> 63" on 64-bit platform, i.e. 
> "(ssize_t)len < 0" or "len > SIZE_MAX / 2". This is not a check for 
> overflow at all, it doesn't even depend on the value of "buf".
> 
> If this is what was intended then it's better to write it explicitly. If 
> this is not what was intended then some other approach is required.

I'd say that is an instance of the compiler knowing better than us how
big |len| would have to be in order to trigger an overflow. Those rules
are going to be platform specific so we should not attempt to second
guess them, but instead let the optimiser do its job.

Matt