From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 104135 invoked by alias); 29 Jun 2016 14:42:26 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 104082 invoked by uid 89); 29 Jun 2016 14:42:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,KAM_ASCII_DIVIDERS,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=no version=3.3.2 spammy= X-HELO: mail-wm0-f41.google.com Received: from mail-wm0-f41.google.com (HELO mail-wm0-f41.google.com) (74.125.82.41) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 29 Jun 2016 14:42:15 +0000 Received: by mail-wm0-f41.google.com with SMTP id a66so77219659wme.0 for ; Wed, 29 Jun 2016 07:42:15 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=U3slmDu6952DJ+9oY1aGqdZwpPYUBilu3qgbWWz1Ri8=; b=bnT1DV6B9V+1xZbasEgBLXhV6B5IEZRqnwZzhx6PRRv4aFvZqnrhDdkp3b0eSl/kwZ KVROIqv8GxV974UE6yHcOPatWcDuEvBZ14YNR6OJyLt++DAMkIRtUShAIMp04L2zK0mz YM2c8s7+PhjFKC/Ixg6vh8PCpRJxkqj3NBwYMu04qkJx8RIIHA9gst2rJ0xG/4cU17uY DmAhbGuzvfJYrWIefWQ0szsriFMk0kj9q2viaJ12ujFlgbol9z6Bx0QtG/j1l7hRbdJT dHLxku+TXObITc4H3+ytr3+L3vY757WsQHDtwZ1HtqPYqUAikrrwsJoK0l+UEB7IN25F HvRw== X-Gm-Message-State: ALyK8tJpr8xv53mxBezpXWkq3c0BtRR/AnU1Fan979n3Vfd9XHcPdG4I14NSyZPmQ1A7apicKDqa9c3ZSXGNM6Yv X-Received: by 10.194.82.161 with SMTP id j1mr9734708wjy.65.1467211332657; Wed, 29 Jun 2016 07:42:12 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.36.215 with HTTP; Wed, 29 Jun 2016 07:42:11 -0700 (PDT) In-Reply-To: References: From: Manish Goregaokar Date: Wed, 29 Jun 2016 14:42:00 -0000 Message-ID: Subject: Re: [PATCH] Initialize strtok_r's saveptr to NULL To: Pedro Alves Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2016-06/txt/msg00516.txt.bz2 Fixed and pushed. Is this something we can fix in gcc? Pointer overflow is UB IIRC so + 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 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 !=3D NULL) > { > struct cleanup *cleanup =3D make_cleanup (xfree, data); > char *line, *t; > > line =3D strtok_r (data, "\n", &t); > while (line !=3D 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=3Dno. > > 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 =E2=80=98rust_get_= disr_info.isra.5=E2=80=99: > /home/pedro/gdb/mygit/src/gdb/rust-lang.c:173:15: error: =E2=80=98__s=E2= =80=99 may be used uninitialized in this function [-Werror=3Dmaybe-uninitia= lized] > ret.name =3D 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 **__nex= tp); > __STRING_INLINE char * > __strtok_r_1c (char *__s, char __sep, char **__nextp) > { > char *__result; > if (__s =3D=3D NULL) > __s =3D *__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 =3D xstrdup (TYPE_FIELD_NAME (type, 0)); > cleanup =3D make_cleanup (xfree, name); > tail =3D 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$$$...$ > where the fieldnos are the indices of the fields that should be > traversed in order to find the field (which may be several field= s deep) > and the variantname is the name of the variant of the case when = the > field is zero. */ > for (token =3D strtok_r (tail, "$", &saveptr); > > "tail" can _never_ be NULL. Adding: > > gdb_assert (tail !=3D 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 =3D name + strlen (RUST_ENUM_PREFIX); > + tail =3D 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 =3D &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=3Dno trips on a warning: > > ../../binutils-gdb/gdb/rust-lang.c:173:15: error: saveptr may be used > uninitialized in this function [-Werror=3Dmaybe-uninitialized] > ret.name =3D 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 =3D xstrdup (TYPE_FIELD_NAME (type, 0)); > cleanup =3D make_cleanup (xfree, name); > tail =3D name + strlen (RUST_ENUM_PREFIX); > ... > for (token =3D strtok_r (tail, "$", &saveptr); > > Fix this by always initializing saveptr. > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > OK with that change, and ... > >> 2016-06-29 Manish Goregaokar >> >> gdb/ChangeLog: >> * rust-lang.c (rust_get_disr_info): Initialize saveptr to NULL > > ... add missing period. > > Thanks, > Pedro Alves >