[cfe-dev] RFC: do not optimize on basis of __attribute__((nonnull)) in glibc headers

[James Y Knight via cfe-dev]

BTW, I just ran this test program (built with "gcc -ggdb -fno-builtin -Wall
test.c"), to check if the library functions actually do all succeed when
given null values, or if some might segfault. It would appear that in the
current library implementation, on x86-64, everything can be null when
given a zero length, other than the 1st arg to strncat.

#define _GNU_SOURCE
#include <string.h>
#include <locale.h>

char *a = 0, *b = 0;
char valid[10];

int main(int argc, char ** argv) {
  locale_t ll = newlocale(LC_CTYPE, "", NULL);
  int result = 0;
  size_t count = argc-1;
  memcpy(a, b, count);
  memmove(a, b, count);
  memset(a, 4, count);
  result += memcmp(a, b, count);
  result += memchr(a, 4, count) != 0;
  result += memrchr(a, 4, count) != 0;
  strncpy(a, b, count);
  strncat(valid, b, count); // crashes if 1st arg is not valid
  result += strncmp(a, b, count);
  strndup(a, count);
  memccpy(a, b, 4, count);
  mempcpy(a, b, count);
  result += strnlen(a, count);
  bcopy(a, b, count);
  bzero(a, count);
  result += bcmp(a, b, count);
  memfrob(a, count);
  result += strncasecmp(a, b, count);
  result += strncasecmp_l(a, b, count, ll);
  stpncpy(a, b, count);
  return result;
}


On Tue, Jan 3, 2017 at 6:55 PM, James Y Knight <jyknight at google.com> wrote:

>
>
> On Tue, Jan 3, 2017 at 6:19 PM, Richard Smith via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>> On 3 January 2017 at 14:46, Friedman, Eli <efriedma at codeaurora.org>
>> wrote:
>>
>>> On 1/3/2017 2:06 PM, Richard Smith via cfe-dev wrote:
>>>
>>>> Via https://reviews.llvm.org/D27855, LLVM is likely to gain the
>>>> ability to delete null checks in callers based on __attribute__((nonnull))
>>>> on the callee. This interacts badly with glibc's choice to mark the pointer
>>>> parameters of memcpy, memmove, etc. as __attribute__((nonnull)) -- it is
>>>> relatively common for programs to pass a null pointer and a zero size to
>>>> these functions, and in practice libc implementations accept such usage.
>>>> Indeed, LLVM's lowering of @llvm.memcpy intrinsics relies on these calls
>>>> working.
>>>>
>>>> Deleting a null pointer check on p after a memcpy(p, q, 0) call seems
>>>> extremely user-hostile, and very unlikely to result in a valuable
>>>> improvement to the program, so I propose that we stop lowering
>>>> __attribute__((nonnull)) on these builtin library functions to the llvm
>>>> nonnull attribute.
>>>>
>>>> (Chandler is working on a paper for the C++ committee proposing to give
>>>> these functions defined behavior when given a null pointer and a zero size,
>>>> but optimizing on the basis of these particular nonnull attributes seems
>>>> like a bad idea regardless of the C or C++ committees' decisions.)
>>>>
>>>> Thoughts?
>>>>
>>>
>>> "memcpy, memmove, etc." seems to be hiding some details here; do you
>>> have a complete list of functions you want to special-case?  There are a
>>> lot of functions which could potentially go on that list, and some of them
>>> are POSIX or GNU extensions.
>>>
>>
>> The following glibc functions in <string.h> take both a pointer and a
>> size providing an upper limit on the number of bytes that may be accessed
>> through that pointer, and mark the pointer as __attribute__((nonnull)):
>>
>> ISO C:
>>   memcpy, memmove, memset, memcmp, memchr, memrchr
>>   strncpy [param 1 only], strncat [param 1 only], strncmp
>>
>> Extensions:
>>   strndup, memccpy, mempcpy, strnlen, bcopy, bzero, bcmp, memfrob,
>>   strncasecmp, strncasecmp_l [not parameter 4], stpncpy
>>
>> I would be interested in covering at least the ISO C functions. I don't
>> have strong opinions about the extensions (any code built using those is
>> likely also compiled with GCC, and so presumably has to fix this
>> regardless).
>>
>
> IMO, ignoring nonnull on all of these would be a fine idea -- but if we go
> down this path, please do open a bug against glibc asking for them to be
> removed, pointing out that,
> a. it is harmful to mark them nonnull because <evidence>.
> b. clang is going to start ignoring the attributes on those functions even
> if glibc doesn't remove them, because of a.
> c. there will be a standards proposal to stop requiring nonnull, too.
>
> It's worth at least an attempt to persuade them it's wrong to have those
> attributes.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170103/9740ba0a/attachment.html>