[PATCH v2] echo: do not assume that free() leaves errno unmodified

Natanael Copa ncopa at alpinelinux.org
Tue Feb 23 11:31:08 UTC 2021


On Fri, 22 Jan 2021 08:35:55 +0100
Natanael Copa <ncopa at 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


More information about the busybox mailing list