[PATCH] Fix execl sentinels

Rich Felker dalias at aerifal.cx
Mon Jul 1 03:13:40 UTC 2013


On Sun, Jun 30, 2013 at 01:46:38PM +0200, Denys Vlasenko wrote:
> On Sunday 30 June 2013 03:26, Rich Felker wrote:
> > The attached patch fixes calls to execl-type functions to use the
> > correct sentinel, (char *)0, rather than NULL, which can be defined as
> > any null pointer constant, and thus has either integer type or
> > pointer-to-void type, but never pointer-to-character type. The only
> > reason the current code works is that NULL happens to be defined as
> > ((void *)0), which in turn happens to be passed the same way as a null
> > character pointer would be, and thus gets successfully processed by
> > va_arg as char *. However, formally the type is incorrect.
> > 
> > The motivation for this patch was that, in previous versions of musl,
> > NULL was defined as 0 for C++, and ((void *)0) for C. Our testers
> > uncovered very subtle, dangerous, difficult-to-track-down bugs on
> > 64-bit systems due to C++ code using NULL (a 32-bit integer 0) as a
> > sentinel, and ending up with junk in the upper bits of the pointer.
> 
> I would classify this as a compiler bug.

It's not a bug, but a case of not being as defensive as possible.
Since being defensive here is probably not costly, I would agree GCC
should make an effort to avoid this issue, but still such applications
are buggy and should be fixed.

> Even if language spec doesn't explicitly require widening
> of variadic arguments to the full width of the stack slot

If I remember correctly they weren't even on the stack but in
registers. My guess is that somehow GCC knew the low part of the
register was zero so it didn't bother to zero the upper part since it
was just passing an int. But I may be remembering incorrectly. It's
been a while, and my point here was not to revisit the issue we
encountered in these C++ apps in detail, but rather just to provide a
little bit of background on why we deemed it worthwhile for musl to
help the compiler catch these bugs and throw warnings for them.

> (IIRC C/C++ require promotions to int, but in this case
> stack slot is long-sized), it's just a prudent measure
> to do that.
> 
> On x86-64, it happens automatically if PUSH insn is used:
> it always sign-extends the pushed value to 64 bits.
> No additional insns need to be generated.

This all addresses the _mechanism_ of how it might or might not cause
real-world problems on particular archs, but the underlying issue is
that the interface contract for the function has been violated if you
pass it the wrong type, and in most cases that leads to undefined
behavior. Fortunately for buggy programs, it's undefined behavior that
has no ill effects on most implementations.

> To expect zillions of C/C++ programs out there to have
> every usage of NULL in execl fixed is not realistic.

That's why we changed the definition in musl to 0L rather than 0.
Since 0L and ((void *)0) have identical representation and argument
passing convention, the buggy code will always work now. But the fact
that 0L does not have pointer type causes GCC not to consider it a
valid sentinel for warning purposes, so incorrect calls to variadic
functions that were flagged with GCC attributes as needing a
null-pointer sentinel will be reported as warnings if you're using
musl.

Rich


More information about the busybox mailing list