[PATCH] Initialize strtok_r's saveptr to NULL

[PATCH] Initialize strtok_r's saveptr to NULL

[Manish Goregaokar]

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.


----

This fixes a build warning.

2016-06-29  Manish Goregaokar  <manish@mozilla.com>

gdb/ChangeLog:
    * rust-lang.c (rust_get_disr_info): Initialize saveptr to NULL
---
 gdb/rust-lang.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index c01687a..1849349 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -121,7 +121,7 @@ rust_get_disr_info (struct type *type, const
gdb_byte *valaddr,
   if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
            strlen (RUST_ENUM_PREFIX)) == 0)
     {
-      char *tail, *token, *name, *saveptr;
+      char *tail, *token, *name, *saveptr = NULL;
       unsigned long fieldno;
       struct type *member_type;
       LONGEST value;
-- 
2.8.3

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

[Pedro Alves]

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

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

[Manish Goregaokar]

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
>

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

[Pedro Alves]

On 06/29/2016 03:42 PM, Manish Goregaokar wrote:
> Fixed and pushed.

Thanks.  Please don't put the "gdb/ChangeLog:" lines in the
ChangeLog file though. :-)

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

I'd hope so.  Was already working on a minimal reproducer.  :-)
Filed a gcc bug now:

 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71699

Thanks,
Pedro Alves

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

[Manish Goregaokar]

Yipes, sorry about that. Copy-paste error :)
-Manish


On Wed, Jun 29, 2016 at 8:42 PM, Pedro Alves <palves@redhat.com> wrote:
> On 06/29/2016 03:42 PM, Manish Goregaokar wrote:
>> Fixed and pushed.
>
> Thanks.  Please don't put the "gdb/ChangeLog:" lines in the
> ChangeLog file though. :-)
>
>>
>> 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.
>
> I'd hope so.  Was already working on a minimal reproducer.  :-)
> Filed a gcc bug now:
>
>  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71699
>
> Thanks,
> Pedro Alves
>

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

[Pedro Alves]

On 06/29/2016 03:25 PM, Pedro Alves wrote:
> 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.

Filed this one as:

 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71701

Thanks,
Pedro Alves