Re: [PATCH] Initialize strtok_r's saveptr to NULL

[Manish Goregaokar <manish at mozilla dot com>]

Fixed and pushed.

Is this something we can fix in gcc? Pointer overflow is UB IIRC so
<nonnullptr>+<positive thing> should never be null. Not sure if the
compiler knows that this is positive though.
-Manish


On Wed, Jun 29, 2016 at 7:55 PM, Pedro Alves <palves@redhat.com> wrote:
> On 06/29/2016 01:45 PM, Manish Goregaokar wrote:
>> Accidentally sent this directly to palves instead of to the list.
>>
>>
>> In the review of the previous patch it was mentioned that we shouldn't
>> need to initialize this,
>> but it seems like elsewhere in the codebase we initialize the saveptr
>> of strtok_r to NULL too.
>
> No, strtok_r is only used in a handful of places, and not all
> initialize it.  E.g., linux-tdep.c:
>
>   if (data != NULL)
>     {
>       struct cleanup *cleanup = make_cleanup (xfree, data);
>       char *line, *t;
>
>       line = strtok_r (data, "\n", &t);
>       while (line != NULL)
>         {
>
>>
>>
>> ----
>>
>> This fixes a build warning.
>
>
> The buildbot that failed is the one that builds gdb in C mode
> I can reproduce this here too, with --enable-build-with-cxx=no.
>
> gcc 7 shows a bit more detail, though it's still not very clear:
>
> /home/pedro/gdb/mygit/src/gdb/rust-lang.c: In function ârust_get_disr_info.isra.5â:
> /home/pedro/gdb/mygit/src/gdb/rust-lang.c:173:15: error: â__sâ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>     ret.name = concat (TYPE_NAME (type), "::", token, (char *) NULL);
>                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: some warnings being treated as errors
>
>
> The mention of "__s" is a hint -- I think it comes from an
> expansion of glibc's inline strtok_r, in /usr/include/bits/string2.h:
>
>  __STRING_INLINE char *__strtok_r_1c (char *__s, char __sep, char **__nextp);
>  __STRING_INLINE char *
>  __strtok_r_1c (char *__s, char __sep, char **__nextp)
>  {
>    char *__result;
>    if (__s == NULL)
>      __s = *__nextp;
>  ...
>
>
> So if on the first call to strtok_r, "tail" is NULL, __s here is NULL
> and "token" becomes the uninitialized "savedptr".
>
> So the problem is that gcc doesn't understand that in:
>
>       name = xstrdup (TYPE_FIELD_NAME (type, 0));
>       cleanup = make_cleanup (xfree, name);
>       tail = name + strlen (RUST_ENUM_PREFIX);
>
>       /* The location of the value that doubles as a discriminant is
>          stored in the name of the field, as
>          RUST$ENCODED$ENUM$<fieldno>$<fieldno>$...$<variantname>
>          where the fieldnos are the indices of the fields that should be
>          traversed in order to find the field (which may be several fields deep)
>          and the variantname is the name of the variant of the case when the
>          field is zero.  */
>       for (token = strtok_r (tail, "$", &saveptr);
>
> "tail" can _never_ be NULL.  Adding:
>
>       gdb_assert (tail != NULL);
>
> just before the strtok_r makes the warning go away, which proves
> that that's indeed the problem.
>
> If "tail" could ever be NULL here, then the warning would be
> revealing a bug -- exactly the sort of bug that I was hoping a warning
> would catch.  But it looks like it's revealing a gcc bug instead...
>
> xstrdup is marked with __attribute__ ((__returns_nonnull__)),
> so gcc knows "name" is never NULL.  Hacking the "tail" initialization
> like this:
>
>  -      tail = name + strlen (RUST_ENUM_PREFIX);
>  +      tail = name;
>
> Makes the warning go away.
>
> Changing the strlen to a sizeof, to make sure gcc understands this is
> a constant offset does not help.  Even writing it as
>
>      tail = &name[sizeof (RUST_ENUM_PREFIX) - 1];
>
> does not help either.  It looks like a gcc bug to me that gcc
> doesn't propagate the non-nullness to "tail" (while g++ does).
>
> Oh well...
>
> It'd be good to include a bit more detail in the commit log, about
> at least which warning triggered.  E.g., something like this:
>
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Building gdb with --enable-build-with-cxx=no trips on a warning:
>
>  ../../binutils-gdb/gdb/rust-lang.c:173:15: error: saveptr may be used
>  uninitialized in this function [-Werror=maybe-uninitialized]
>      ret.name = concat (TYPE_NAME (type), "::", token, (char *) NULL);
>
> The problem is that gcc doesn't understand that "tail" can never be
> NULL in the call to strtok_r:
>
>       name = xstrdup (TYPE_FIELD_NAME (type, 0));
>       cleanup = make_cleanup (xfree, name);
>       tail = name + strlen (RUST_ENUM_PREFIX);
> ...
>       for (token = strtok_r (tail, "$", &saveptr);
>
> Fix this by always initializing saveptr.
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>
> OK with that change, and ...
>
>> 2016-06-29  Manish Goregaokar  <manish@mozilla.com>
>>
>> gdb/ChangeLog:
>>     * rust-lang.c (rust_get_disr_info): Initialize saveptr to NULL
>
> ... add missing period.
>
> Thanks,
> Pedro Alves
>