* [PATCH] fix strdupa evaulating expression twice @ 2016-08-28 10:59 Noam Meltzer 2016-08-28 15:08 ` Rich Felker 0 siblings, 1 reply; 6+ messages in thread From: Noam Meltzer @ 2016-08-28 10:59 UTC (permalink / raw) To: musl [-- Attachment #1.1: Type: text/plain, Size: 1 bytes --] [-- Attachment #1.2: Type: text/html, Size: 26 bytes --] [-- Attachment #2: 0001-fix-strdupa-evaulating-expression-twice.patch --] [-- Type: text/x-patch, Size: 978 bytes --] 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); -- 2.7.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix strdupa evaulating expression twice 2016-08-28 10:59 [PATCH] fix strdupa evaulating expression twice Noam Meltzer @ 2016-08-28 15:08 ` Rich Felker 2016-08-28 16:53 ` Noam Meltzer 0 siblings, 1 reply; 6+ messages in thread From: Rich Felker @ 2016-08-28 15:08 UTC (permalink / raw) To: musl 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... Rich ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix strdupa evaulating expression twice 2016-08-28 15:08 ` Rich Felker @ 2016-08-28 16:53 ` Noam Meltzer 2016-08-28 17:04 ` Bobby Bingham 0 siblings, 1 reply; 6+ messages in thread From: Noam Meltzer @ 2016-08-28 16:53 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 1920 bytes --] 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. 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 [-- Attachment #2: Type: text/html, Size: 2581 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix strdupa evaulating expression twice 2016-08-28 16:53 ` Noam Meltzer @ 2016-08-28 17:04 ` Bobby Bingham 2016-08-29 5:40 ` Noam Meltzer 0 siblings, 1 reply; 6+ messages in thread From: Bobby Bingham @ 2016-08-28 17:04 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix strdupa evaulating expression twice 2016-08-28 17:04 ` Bobby Bingham @ 2016-08-29 5:40 ` Noam Meltzer 2016-08-30 21:03 ` Rich Felker 0 siblings, 1 reply; 6+ messages in thread From: Noam Meltzer @ 2016-08-29 5:40 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 2883 bytes --] 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 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 > [-- Attachment #2: Type: text/html, Size: 4136 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix strdupa evaulating expression twice 2016-08-29 5:40 ` Noam Meltzer @ 2016-08-30 21:03 ` Rich Felker 0 siblings, 0 replies; 6+ messages in thread From: Rich Felker @ 2016-08-30 21:03 UTC (permalink / raw) To: musl On Mon, Aug 29, 2016 at 05:40:52AM +0000, Noam Meltzer wrote: > > 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? For the most part it's used internally for optimization purposes, not for making the external API vary by compiler capability. However being that alloca need not even work or be defined on non-GNUC compilers, and that this is a nasty error-prone function that shouldn't be used to begin with, I really have no objection to having it only work on GNUC. On the other hand I don't think we even need to put it in #ifdef __GNUC__; if the non-GCC compiler does not accept the syntax, it will just produce an error anyway. Perhaps we should add strndupa too? It's been requested but IIRC it could not be implemented without GNUC extensions. Rich ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-08-30 21:03 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-28 10:59 [PATCH] fix strdupa evaulating expression twice Noam Meltzer 2016-08-28 15:08 ` Rich Felker 2016-08-28 16:53 ` Noam Meltzer 2016-08-28 17:04 ` Bobby Bingham 2016-08-29 5:40 ` Noam Meltzer 2016-08-30 21:03 ` Rich Felker
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).