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

[Kurt Roeckx via RT]

On Fri, Oct 16, 2015 at 09:44:22PM +0000, Kaduk, Ben via RT wrote:
> 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.

Just to clarify what I mean, buf + len can both have defined and
undefined meaning, it depends on the value of len, so the compiler
can probably not say anything about that part without knowing all
the callers of that code.  As long as it has a defined meaning the
compiler will probably do it.  buf + len < buf also seems to have
a defined meaning, but in all defined meanings it's false, and so
the compiler can just optimize that part away.

Anyway, I would really like this to be changed.


Kurt