From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/10408 Path: news.gmane.org!.POSTED!not-for-mail From: Noam Meltzer Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] fix strdupa evaulating expression twice Date: Mon, 29 Aug 2016 05:40:52 +0000 Message-ID: References: <20160828150816.GF15995@brightrain.aerifal.cx> <20160828170438.dhcbk2my2gkcp7ot@gordon> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=001a114cc2a04927de053b2f4fd5 X-Trace: blaine.gmane.org 1472449282 19879 195.159.176.226 (29 Aug 2016 05:41:22 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Mon, 29 Aug 2016 05:41:22 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-10421-gllmg-musl=m.gmane.org@lists.openwall.com Mon Aug 29 07:41:18 2016 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1beFJt-0004ia-Tk for gllmg-musl@m.gmane.org; Mon, 29 Aug 2016 07:41:18 +0200 Original-Received: (qmail 1918 invoked by uid 550); 29 Aug 2016 05:41:15 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 1896 invoked from network); 29 Aug 2016 05:41:15 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=Aaj1XlxblzjiKcF4T7j1Or842eZREwOL08w/sXJhMTg=; b=YFJM5iZT9+Km69cqKRy102sbHx+DS3sllRhgVc4XKBGQBe5dmAyvWCNkeHsYGvFa8H ROWbU0wVpGAPm9z6KAspsa56RoJ3w6u/VTfZ1KLYZd7vUE4EFzfvsFMiR0gIjGdbdAWT UAFpU+oge9E117jbmBzQYWw6RWQwURKlwmu+vieXnzRoC6CDT6QnUhORFzd6/N9g6JQc iO261TxEGAB7+Irht+hIIfj+0Q9jUocNB27x6GtvJ+Vf60n1cen86L8NjDEZaYQxW89G YylNXB1yE/SOa0H+JqweLOhahgY394LJK3rebOWpraFUQMOyVRTtpg4lUiENSat8QcnQ d5gA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=Aaj1XlxblzjiKcF4T7j1Or842eZREwOL08w/sXJhMTg=; b=T6ZIcyiKAnfHNHkeyA6eVohRrPL8WsdNoGWfxBJlueLly8gRLrHpj8rMapPzBMlhp6 ZwZS4xCys4bI5RfxNO/5JwDA5Kdz6cd9jfg19GvdyOywdAGb0Gd9hRj4i2wMIzfziyKy QFO22ltZb37sOZMAO9GeFP2AoQp6YJWl6NVS139sKFAnoqRLusURI1+9Sc/ZX+oXD43k PRzl//j0zw03/JJe6ZYe7Pp/MJvYbha9/EDCntalb40p9NFmrAO0Yw8dBQBNNrq1Ihwc IOFU1H2UKz24yDUW2GBqSCcVVdYB1c8hKOYxVmgSYmilPMJv8ChmgekFeSuhKRH4adAk ueKQ== X-Gm-Message-State: AE9vXwOb8EM5TgIFq5vCAr0OIW1e0p0NFHjuucWt2QCmgVytZMPPhMeQvXXWHgDMXqG+/tY12WCh2yCGA4yUwg== X-Received: by 10.28.168.83 with SMTP id r80mr9356667wme.44.1472449263247; Sun, 28 Aug 2016 22:41:03 -0700 (PDT) In-Reply-To: <20160828170438.dhcbk2my2gkcp7ot@gordon> Xref: news.gmane.org gmane.linux.lib.musl.general:10408 Archived-At: --001a114cc2a04927de053b2f4fd5 Content-Type: text/plain; charset=UTF-8 On Sun, Aug 28, 2016 at 8:04 PM Bobby Bingham wrote: > On Sun, Aug 28, 2016 at 04:53:50PM +0000, Noam Meltzer wrote: > > On Sun, Aug 28, 2016 at 6:08 PM Rich Felker wrote: > > > > > On Sun, Aug 28, 2016 at 10:59:26AM +0000, Noam Meltzer wrote: > > > > > > > > > > > From bf23b7b8fd39eaca6a05173eaf543e1bce3319ab Mon Sep 17 00:00:00 > 2001 > > > > From: Noam Meltzer > > > > Date: Sun, 28 Aug 2016 13:53:24 +0300 > > > > Subject: [PATCH] fix strdupa evaulating expression twice > > > > > > > > calling strdupa with va_arg as its expression caused unexpected > > > > behaviour. now the expression is evaulated only once. > > > > --- > > > > include/string.h | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/include/string.h b/include/string.h > > > > index ff9badb..976faaf 100644 > > > > --- a/include/string.h > > > > +++ b/include/string.h > > > > @@ -85,7 +85,10 @@ size_t strlcpy (char *, const char *, size_t); > > > > #endif > > > > > > > > #ifdef _GNU_SOURCE > > > > -#define strdupa(x) strcpy(alloca(strlen(x)+1),x) > > > > +#define strdupa(x) (__extension__ ({ \ > > > > + const char *__xval = x; \ > > > > + strcpy(alloca(strlen(__xval)+1),__xval); \ > > > > + })) > > > > int strverscmp (const char *, const char *); > > > > int strcasecmp_l (const char *, const char *, locale_t); > > > > int strncasecmp_l (const char *, const char *, size_t, locale_t); > > > > > > The intent of the form as written is to be actual C (modulo use of > > > alloca) rather than "GNU C". Aside from that, strdupa is essentially > > > always-unsafe and should probably be removed or at least made into a > > > warning... > > > > > > > I understand what you're saying and I tend to agree. However: > > * The entire section of the code is wrapped with the _GNU_SOURCE test > > macro (so the __extension__ trick should work). > > * IMHO, if strdupa is to kept, it should at least provide expected > > behaviour. > > _GNU_SOURCE says that the program has requested glibc extensions, like > strdupa. It does not imply anything about the compiler being used and > whether it implements GCC extensions. > Thanks for clearing that out. > > That said, it looks like glibc only defines strdupa #if __GNUC__, so > this function is simply unavailable when using a compiler that doesn't > implement this extension. > > I'm not sure what the musl policy is here, but maybe we could do > something similar? > The __GNUC__ macros is being used in other parts of musl, so is there a reason not to move strdupa into a #if __GNUC__ macro? > > > > p.s. > > I spent about a day of work chasing a bug caused by the current > > implementation. So what I actually looking for is to save this pain from > > others one way or another. > > > > - Noam > --001a114cc2a04927de053b2f4fd5 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Sun= , Aug 28, 2016 at 8:04 PM Bobby Bingham <koorogi@koorogi.info> wrote:
On Sun, Aug 28, 2016 at 04:53:50PM +0000, Noam Meltzer wrote:
> On Sun, Aug 28, 2016 at 6:08 PM Rich Felker <dalias@libc.org> wrote:
>
> > On Sun, Aug 28, 2016 at 10:59:26AM +0000, Noam Meltzer wrote:
> > >
> >
> > > From bf23b7b8fd39eaca6a05173eaf543e1bce3319ab Mon Sep 17 00:= 00:00 2001
> > > From: Noam Meltzer <tsnoam@gmail.com>
> > > Date: Sun, 28 Aug 2016 13:53:24 +0300
> > > Subject: [PATCH] fix strdupa evaulating expression twice
> > >
> > > calling strdupa with va_arg as its expression caused unexpec= ted
> > > behaviour. now the expression is evaulated only once.
> > > ---
> > >=C2=A0 include/string.h | 5 ++++-
> > >=C2=A0 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/string.h b/include/string.h
> > > index ff9badb..976faaf 100644
> > > --- a/include/string.h
> > > +++ b/include/string.h
> > > @@ -85,7 +85,10 @@ size_t strlcpy (char *, const char *, siz= e_t);
> > >=C2=A0 #endif
> > >
> > >=C2=A0 #ifdef _GNU_SOURCE
> > > -#define=C2=A0 =C2=A0 =C2=A0 strdupa(x)=C2=A0 =C2=A0 =C2=A0 = strcpy(alloca(strlen(x)+1),x)
> > > +#define=C2=A0 =C2=A0 =C2=A0 strdupa(x) (__extension__ ({ \<= br> > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0const char = *__xval =3D x; \
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0strcpy(allo= ca(strlen(__xval)+1),__xval); \
> > > +=C2=A0 =C2=A0 =C2=A0}))
> > >=C2=A0 int strverscmp (const char *, const char *);
> > >=C2=A0 int strcasecmp_l (const char *, const char *, locale_t= );
> > >=C2=A0 int strncasecmp_l (const char *, const char *, size_t,= locale_t);
> >
> > The intent of the form as written is to be actual C (modulo use o= f
> > alloca) rather than "GNU C". Aside from that, strdupa i= s essentially
> > always-unsafe and should probably be removed or at least made int= o a
> > warning...
> >
>
> I understand what you're saying and I tend to agree. However:
>=C2=A0 * The entire section of the code is wrapped with the _GNU_SOURCE= test
> macro (so the __extension__ trick should work).
>=C2=A0 * IMHO, if strdupa is to kept, it should at least provide expect= ed
> behaviour.

_GNU_SOURCE says that the program has requested glibc extensions, like
strdupa.=C2=A0 It does not imply anything about the compiler being used and=
whether it implements GCC extensions.

T= hanks for clearing that out.
=C2=A0

That said, it looks like glibc only defines strdupa #if __GNUC__, so
this function is simply unavailable when using a compiler that doesn't<= br> implement this extension.

I'm not sure what the musl policy is here, but maybe we could do
something similar?

The __GNUC__ macros = is being used in other parts of musl, so is there a reason not to move strd= upa into a #if __GNUC__ macro?


>
> p.s.
> I spent about a day of work chasing a bug caused by the current
> implementation. So what I actually looking for is to save this pain fr= om
> others one way or another.
>
>=C2=A0 - Noam
--001a114cc2a04927de053b2f4fd5--