[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH v2] echo: do not assume that free() leaves errno unmodified
From:       Denys Vlasenko <vda.linux () googlemail ! com>
Date:       2021-02-23 12:01:44
Message-ID: CAK1hOcPD8mVheoOSxUoj07nK8UEMRV3oeJ5h7Wk6NvZSzsCGiw () mail ! gmail ! com
[Download RAW message or body]

Applied, thank you.

On Tue, Feb 23, 2021 at 12:31 PM Natanael Copa <ncopa@alpinelinux.org> wrote:
>
> On Fri, 22 Jan 2021 08:35:55 +0100
> Natanael Copa <ncopa@alpinelinux.org> wrote:
>
> > musl libc's mallocng free() may modify errno if kernel does not support
> > MADV_FREE which causes echo to echo with error when it shouldn't.
> >
> > Future versions of POSIX[1] will require that free() leaves errno
> > unmodified but til then, do not rely free() implementation.
> >
> > Should fix downstream issues:
> > https://github.com/alpinelinux/docker-alpine/issues/134
> > https://gitlab.alpinelinux.org/alpine/aports/-/issues/12311
> >
> > Bloatcheck on x86_64:
> >
> > function                                             old     new   delta
> > echo_main                                            414     406      -8
> > ------------------------------------------------------------------------------
> > (add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-8)               Total: -8
> > bytes
> >    text          data     bss     dec     hex filename
> >     881114      15196    2000  898310   db506 busybox_old
> >      881106     15196    2000  898302   db4fe busybox_unstripped
> > ---
> >
> > v2: do the free after bb_simple_perror_msg so it prints the correct
> >     error message.
> >
> >  coreutils/echo.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/coreutils/echo.c b/coreutils/echo.c
> > index b3828894c..002832ead 100644
> > --- a/coreutils/echo.c
> > +++ b/coreutils/echo.c
> > @@ -97,6 +97,7 @@ int echo_main(int argc UNUSED_PARAM, char **argv)
> >  #else
> >       char nflag = 1;
> >       char eflag = 0;
> > +     int err;
> >
> >       while ((arg = *++argv) != NULL) {
> >               char n, e;
> > @@ -184,14 +185,12 @@ int echo_main(int argc UNUSED_PARAM, char **argv)
> >
> >   do_write:
> >       /* Careful to error out on partial writes too (think ENOSPC!) */
> > -     errno = 0;
> > -     /*r =*/ full_write(STDOUT_FILENO, buffer, out - buffer);
> > -     free(buffer);
> > -     if (/*WRONG:r < 0*/ errno) {
> > +     err = full_write(STDOUT_FILENO, buffer, out - buffer) != out - buffer;
> > +     if (err) {
> >               bb_simple_perror_msg(bb_msg_write_error);
> > -             return 1;
> >       }
> > -     return 0;
> > +     free(buffer);
> > +     return err;
> >  }
> >
> >  /*
>
> Ping. Any change to get this merged?
>
> It fixes a real bug and saves a few bytes.
>
> -nc
> _______________________________________________
> busybox mailing list
> busybox@busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]