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

[Richard Smith via cfe-dev]

On 3 January 2017 at 16:03, James Y Knight <jyknight at google.com> wrote:

> 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.
>

My bad -- the nonnull on parameter 1 of strncat is correct, it's the
nonnull on parameter 2 that is dubious.


> #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/341588ce/attachment.html>