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

[Pedro Alves <palves at redhat dot com>]

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