Re: NULL pointer arithmetic issues

[Roman Lebedev <lebedev.ri%gmail.com@localhost>]

Warning: i'm not subscribed to the lists CC'd here.

On Sun, Mar 8, 2020 at 8:26 PM Kamil Rytarowski <n54%gmx.com@localhost> wrote:
>
> On 08.03.2020 18:00, Taylor R Campbell wrote:
> > Thanks for doing the research.
> >
> >> Date: Sun, 8 Mar 2020 15:30:02 +0100
> >> From: Kamil Rytarowski <n54%gmx.com@localhost>
> >>
> >> NULL+0 was added to UBSan proactively as it is as of today not
> >> miscompiled, but it is planned to so in not so distant time. It is a
> >> chicken-egg problem.
> >
> > If you say it is planned, can you please cite the plan?
> >
>
> Adding Roman Lebedev, we discussed about enabling NULL+0 optimization.

> Teaching LLVM about this will allow more aggressive optimization, so
> code like this:
>
> http://netbsd.org/~kamil/null_plus_0-ub.c
>
> will report different results depending on optimization levels.
I believe this point is correct. Given

int
statu(char *a)
{
  a += getuid() - geteuid();
  return a == NULL;
}

in C, as per all the wording cited below \/, we are allowed to assume
that given `ptr + offset`, regardless of `offset`,
both `ptr` and `ptr + offset` are non-NULL,
so we are allowed to lower that in clang front-end as:

int
statu(char *a)
{
  __builtin_assume(a != NULL);
  a += getuid() - geteuid();
  __builtin_assume(a != NULL);
   return a == NULL;
}

And that will naturally fold to

int
statu(char *a)
{
  (void)getuid();
  (void)geteuid();
  return false;
}

.. which isn't what the code initially intended to do.
I.e. it's a dormant miscompile.

> > In C++, adding zero to a null pointer is explicitly allowed and
> > guaranteed to return a null pointer.  See, for example, C++11 5.7
> > `Additive operators', clause 7, p. 117: `If the value 0 is added to or
> > subtracted from a pointer value, the result compares equal to the
> > original pointer value.'  C++17 clarifies in 8.7 `Additive operators',
> > clause 7, p. 132: `If the value 0 is added to or subtracted from a
> > null pointer value, the result is a null pointer.'
> >
> > So it would be a little surprising to me if compilers -- which tend to
> > focus their efforts on C++ more than C these days -- went out of their
> > way to act on the inference that evaluating p + 0 implies p is nonnull
> > in C.
> >
>
> I underlined the C language in my message. More elaborated answer here:
>
> https://reviews.llvm.org/D67122#inline-612172
>
> I was told by Roman that it was checked during a C committee meeting and
> confirmed to be an intentional UB.
Correction: Aaron Ballman asked about this in non-public WG14 reflector
mailing list, it wasn't a committee meeting, but the point still stands.

> >> There is however a fallback. If we want to use NULL+0, we must use
> >> -fno-delete-null-pointer-checks that avoids miscompilation and raising
> >> UBSan reports. If we want to allow NULL+x we must enable -fwrapv.
> >
> > Adding -fno-delete-null-pointer-checks for clang too sounds sensible
> > to me in general, but please check with joerg first.  It remains
> > unclear to me that it's necessary here.
> >
>
> Today it will calm down LLVM UBSan. In future potentially avoid
> miscompilation.
>
> There are also memcpy(NULL,NULL,0)-like cases that need research why
> they happen in the first place.

Roman