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

[Kaduk, Ben via RT]

On 10/16/2015 04:35 PM, Kurt Roeckx via RT wrote:
> On Fri, Oct 16, 2015 at 06:50:36PM +0000, Kurt Roeckx via RT wrote:
>> On Fri, Oct 16, 2015 at 04:50:59PM +0000, Matt Caswell via RT wrote:
>>> In a well-behaved program there is no undefined behaviour. The "buf +
>>> len < buf" check will always evaluate to false, so in that sense is
>>> useless but it *is* well defined.
>> The defined behaviour for the "buf + len" part is as far as I know
>> that you're that the pointer should point inside the allocated
>> object or 1 byte after it.  So as long as "len" is in the valid
>> range, the "buf + len" part should be well defined.  The test with
>> -1 is clearly undefined.
>>
>> As far as I know in the comparison pointers they should point
>> to the same object.  But the check seems to imply that they might
>> not point to the same object or that buf is not the base of the
>> object.  But since len is unsigned only the option that they don't
>> point to the same object seems to be left.
>>
>> So it's unclear to me if this is defined behaviour or not.
> So thinking about this some more, it seems to be a check for
> undefined behaviour that probably itself is still defined.
>
> That probably also means the compiler can assume that it's always
> false and eliminate the check, it's probably just not smart enough
> yet.
>

I think it is unambiguous that there are values of unsigned char *buf
and size_t len for which evaluating (bug + len < buf) invokes undefined
behavior -- the sheer act of performing the addition buf + len can
produce undefined behavior, even before any comparison.  I am as-yet
uncertain whether the case when buf points into the middle of an object
and len is (size_t)-1 invokes undefined behavior, but I am inclined to
believe that it is also undefined behavior.

-Ben