Re: [PATCH] fix strdupa evaulating expression twice

[Bobby Bingham <koorogi@koorogi.info>]

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

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?

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