[musl] [PATCH] remove strdupa

[musl] [PATCH] remove strdupa

[Ismael Luceno]

There's no portable way to implement strdupa without double evaluation
of it's parameter, and it's use leads to vulnerabilities, since there's
no chance to check for stack overruns.

Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
---
 include/string.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/string.h b/include/string.h
index 43ad0942edd5..65fe0d503004 100644
--- a/include/string.h
+++ b/include/string.h
@@ -88,7 +88,6 @@ void explicit_bzero (void *, size_t);
 #endif
 
 #ifdef _GNU_SOURCE
-#define	strdupa(x)	strcpy(alloca(strlen(x)+1),x)
 int strverscmp (const char *, const char *);
 char *strchrnul(const char *, int);
 char *strcasestr(const char *, const char *);
-- 
2.38.1

Re: [musl] [PATCH] remove strdupa

[Rich Felker]

On Sat, Oct 22, 2022 at 03:57:23PM +0200, Ismael Luceno wrote:
> There's no portable way to implement strdupa without double evaluation
> of it's parameter, and it's use leads to vulnerabilities, since there's
> no chance to check for stack overruns.
> 
> Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
> ---
>  include/string.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/string.h b/include/string.h
> index 43ad0942edd5..65fe0d503004 100644
> --- a/include/string.h
> +++ b/include/string.h
> @@ -88,7 +88,6 @@ void explicit_bzero (void *, size_t);
>  #endif
>  
>  #ifdef _GNU_SOURCE
> -#define	strdupa(x)	strcpy(alloca(strlen(x)+1),x)
>  int strverscmp (const char *, const char *);
>  char *strchrnul(const char *, int);
>  char *strcasestr(const char *, const char *);
> -- 
> 2.38.1

Does anyone have strong opinions one way or the other on this --
especially distro folks who'd need to deal with the fallout?

Rich

Re: [musl] [PATCH] remove strdupa

[Szabolcs Nagy]

* Rich Felker <dalias@libc.org> [2022-10-23 01:46:22 -0400]:
> On Sat, Oct 22, 2022 at 03:57:23PM +0200, Ismael Luceno wrote:
> > There's no portable way to implement strdupa without double evaluation
> > of it's parameter, and it's use leads to vulnerabilities, since there's
> > no chance to check for stack overruns.
> > 
> > Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
> > ---
> >  include/string.h | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/include/string.h b/include/string.h
> > index 43ad0942edd5..65fe0d503004 100644
> > --- a/include/string.h
> > +++ b/include/string.h
> > @@ -88,7 +88,6 @@ void explicit_bzero (void *, size_t);
> >  #endif
> >  
> >  #ifdef _GNU_SOURCE
> > -#define	strdupa(x)	strcpy(alloca(strlen(x)+1),x)
> >  int strverscmp (const char *, const char *);
> >  char *strchrnul(const char *, int);
> >  char *strcasestr(const char *, const char *);
> > -- 
> > 2.38.1
> 
> Does anyone have strong opinions one way or the other on this --
> especially distro folks who'd need to deal with the fallout?

debian code search finds strdupa in 126 packages,
so it is widely used and not trivial to manually fix up,
i'd expect distros to just readd that definition to avoid breakage.

Re: [musl] [PATCH] remove strdupa

[Rich Felker]

On Sun, Oct 23, 2022 at 04:10:46PM +0200, Szabolcs Nagy wrote:
> * Rich Felker <dalias@libc.org> [2022-10-23 01:46:22 -0400]:
> > On Sat, Oct 22, 2022 at 03:57:23PM +0200, Ismael Luceno wrote:
> > > There's no portable way to implement strdupa without double evaluation
> > > of it's parameter, and it's use leads to vulnerabilities, since there's
> > > no chance to check for stack overruns.
> > > 
> > > Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
> > > ---
> > >  include/string.h | 1 -
> > >  1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/include/string.h b/include/string.h
> > > index 43ad0942edd5..65fe0d503004 100644
> > > --- a/include/string.h
> > > +++ b/include/string.h
> > > @@ -88,7 +88,6 @@ void explicit_bzero (void *, size_t);
> > >  #endif
> > >  
> > >  #ifdef _GNU_SOURCE
> > > -#define	strdupa(x)	strcpy(alloca(strlen(x)+1),x)
> > >  int strverscmp (const char *, const char *);
> > >  char *strchrnul(const char *, int);
> > >  char *strcasestr(const char *, const char *);
> > > -- 
> > > 2.38.1
> > 
> > Does anyone have strong opinions one way or the other on this --
> > especially distro folks who'd need to deal with the fallout?
> 
> debian code search finds strdupa in 126 packages,
> so it is widely used and not trivial to manually fix up,
> i'd expect distros to just readd that definition to avoid breakage.

Distros can of course add it with an ugly -D'strdupa(x)=...' in CFLAGS
too, or with a one-line patch to the affected packages to add the
#define. I would kinda expect some of these (likely any using
autotools or especially gnulib) already handle the case where it's not
defined and define their own, so maybe it wouldn't actually be that
much breakage (but of course this would also be a reduction in
bug-catching).

I think a reasonable (but unsatisfying) outcome of this thread might
end up being "do nothing until there's action to greatly reduce the
number of packages using strdupa" (or at least evaluate the situation
and determine that most would not break). I would really like to avoid
"improving" interfaces that are harmful and slated for removal, but I
also don't want to make unnecessary new burden on
distros/integrators/users.

Rich

Re: [musl] [PATCH] remove strdupa

[Rich Felker]

On Sun, Oct 23, 2022 at 11:13:42AM -0400, Rich Felker wrote:
> On Sun, Oct 23, 2022 at 04:10:46PM +0200, Szabolcs Nagy wrote:
> > * Rich Felker <dalias@libc.org> [2022-10-23 01:46:22 -0400]:
> > > On Sat, Oct 22, 2022 at 03:57:23PM +0200, Ismael Luceno wrote:
> > > > There's no portable way to implement strdupa without double evaluation
> > > > of it's parameter, and it's use leads to vulnerabilities, since there's
> > > > no chance to check for stack overruns.
> > > > 
> > > > Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
> > > > ---
> > > >  include/string.h | 1 -
> > > >  1 file changed, 1 deletion(-)
> > > > 
> > > > diff --git a/include/string.h b/include/string.h
> > > > index 43ad0942edd5..65fe0d503004 100644
> > > > --- a/include/string.h
> > > > +++ b/include/string.h
> > > > @@ -88,7 +88,6 @@ void explicit_bzero (void *, size_t);
> > > >  #endif
> > > >  
> > > >  #ifdef _GNU_SOURCE
> > > > -#define	strdupa(x)	strcpy(alloca(strlen(x)+1),x)
> > > >  int strverscmp (const char *, const char *);
> > > >  char *strchrnul(const char *, int);
> > > >  char *strcasestr(const char *, const char *);
> > > > -- 
> > > > 2.38.1
> > > 
> > > Does anyone have strong opinions one way or the other on this --
> > > especially distro folks who'd need to deal with the fallout?
> > 
> > debian code search finds strdupa in 126 packages,
> > so it is widely used and not trivial to manually fix up,
> > i'd expect distros to just readd that definition to avoid breakage.
> 
> Distros can of course add it with an ugly -D'strdupa(x)=...' in CFLAGS
> too, or with a one-line patch to the affected packages to add the
> #define. I would kinda expect some of these (likely any using
> autotools or especially gnulib) already handle the case where it's not
> defined and define their own, so maybe it wouldn't actually be that
> much breakage (but of course this would also be a reduction in
> bug-catching).
> 
> I think a reasonable (but unsatisfying) outcome of this thread might
> end up being "do nothing until there's action to greatly reduce the
> number of packages using strdupa" (or at least evaluate the situation
> and determine that most would not break). I would really like to avoid
> "improving" interfaces that are harmful and slated for removal, but I
> also don't want to make unnecessary new burden on
> distros/integrators/users.

Extra data points from discussion on #alpine-devel: at least MacOS and
FreeBSD lack a strdupa macro. This means pretty much anything using it
is either non-portable OS-specific software or already has checks and
fallback cases for when it doesn't exist. So the breakage from
removing it might be very low.

Rich

Re: [musl] [PATCH] remove strdupa

[Konstantin P.]

[-- Attachment #1: Type: text/plain, Size: 2957 bytes --]

systemd is using strdupa widely, and it is main reason why it should
considered to stay.

On Sun, Oct 23, 2022 at 8:03 PM Rich Felker <dalias@libc.org> wrote:

> On Sun, Oct 23, 2022 at 11:13:42AM -0400, Rich Felker wrote:
> > On Sun, Oct 23, 2022 at 04:10:46PM +0200, Szabolcs Nagy wrote:
> > > * Rich Felker <dalias@libc.org> [2022-10-23 01:46:22 -0400]:
> > > > On Sat, Oct 22, 2022 at 03:57:23PM +0200, Ismael Luceno wrote:
> > > > > There's no portable way to implement strdupa without double
> evaluation
> > > > > of it's parameter, and it's use leads to vulnerabilities, since
> there's
> > > > > no chance to check for stack overruns.
> > > > >
> > > > > Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
> > > > > ---
> > > > >  include/string.h | 1 -
> > > > >  1 file changed, 1 deletion(-)
> > > > >
> > > > > diff --git a/include/string.h b/include/string.h
> > > > > index 43ad0942edd5..65fe0d503004 100644
> > > > > --- a/include/string.h
> > > > > +++ b/include/string.h
> > > > > @@ -88,7 +88,6 @@ void explicit_bzero (void *, size_t);
> > > > >  #endif
> > > > >
> > > > >  #ifdef _GNU_SOURCE
> > > > > -#define        strdupa(x)      strcpy(alloca(strlen(x)+1),x)
> > > > >  int strverscmp (const char *, const char *);
> > > > >  char *strchrnul(const char *, int);
> > > > >  char *strcasestr(const char *, const char *);
> > > > > --
> > > > > 2.38.1
> > > >
> > > > Does anyone have strong opinions one way or the other on this --
> > > > especially distro folks who'd need to deal with the fallout?
> > >
> > > debian code search finds strdupa in 126 packages,
> > > so it is widely used and not trivial to manually fix up,
> > > i'd expect distros to just readd that definition to avoid breakage.
> >
> > Distros can of course add it with an ugly -D'strdupa(x)=...' in CFLAGS
> > too, or with a one-line patch to the affected packages to add the
> > #define. I would kinda expect some of these (likely any using
> > autotools or especially gnulib) already handle the case where it's not
> > defined and define their own, so maybe it wouldn't actually be that
> > much breakage (but of course this would also be a reduction in
> > bug-catching).
> >
> > I think a reasonable (but unsatisfying) outcome of this thread might
> > end up being "do nothing until there's action to greatly reduce the
> > number of packages using strdupa" (or at least evaluate the situation
> > and determine that most would not break). I would really like to avoid
> > "improving" interfaces that are harmful and slated for removal, but I
> > also don't want to make unnecessary new burden on
> > distros/integrators/users.
>
> Extra data points from discussion on #alpine-devel: at least MacOS and
> FreeBSD lack a strdupa macro. This means pretty much anything using it
> is either non-portable OS-specific software or already has checks and
> fallback cases for when it doesn't exist. So the breakage from
> removing it might be very low.
>
> Rich
>

[-- Attachment #2: Type: text/html, Size: 4016 bytes --]

Re: [musl] [PATCH] remove strdupa

[Joakim Sindholt]

On Mon, 24 Oct 2022 10:58:22 +0300, "Konstantin P." <ria.freelander@gmail.com> wrote:
> systemd is using strdupa widely, and it is main reason why it should
> considered to stay.

At git HEAD it only uses strdupa 5 times, all of them in
src/test/test-string-util.c. The rest are their own str[n]dupa_safe
macro that uses alloca directly.

Re: [musl] [PATCH] remove strdupa

[Rich Felker]

On Mon, Oct 24, 2022 at 10:58:22AM +0300, Konstantin P. wrote:
> systemd is using strdupa widely, and it is main reason why it should
> considered to stay.

This is not a reason for it to be considered to stay. Even aside from
systemd intentionally not supporting anything but glibc and therefore
always requiring patching to build, avoiding a one-line patch to a
single package regardless of it's importance is not going to be a
reason. The motivating factor here is the scale of work imposed on
people preparing distributions or their own system integrations --
whether they'd be able to take care of the problem with a handful of
easy patches, or fighting with many tens or hundreds of packages
needing attention to get them building again.

Rich