Re: [PATCH] Improve execl* functions

Federico Terraneo <[email protected]> Mon, 27 Jan 2025 15:36:45 +0100

Newsgroups	gmane.comp.lib.newlib
Message-ID	<PAXP251MB0317B746787EFDF4C89A9C3FF9EC2@PAXP251MB0317.EURP251.PROD.OUTLOOK.COM>

Hi Corinna,

On 27/01/25 14:21, Corinna Vinschen wrote:
> Adding ARG_NUM_MAX this way requires all targets to define their own
> ARG_NUM_MAX if they want to support more than 256 args.  Right now,
> there's no such limitation.

As far as I can see, right now there is this limitation in newlib.
Let's have a look at the current code of one of these functions:

https://sourceware.org/git?p=newlib-cygwin.git;a=blob_plain;f=newlib/libc/posix/execl.c

What I see is a

const char *argv[256];

declared on the stack, and a do..while loop copying args into that array 
with no bound checking. Thus, attempting to pass more than 256 arguments 
will silently corrupt the stack.

Attached please find a test case that when compiled with the address 
sanitizer fails due to the stack buffer overflow when more than 256 
arguments are passed to the execl code above.

The purpose of my patch is to:
- avoid the stack buffer overflow in all cases
- allow targets to override the default 256 arguments limit if they so 
desire, either lowering to reduce stack use or increasing it if 256 is 
not enough

Let me know if I'm missing something.

Best regards,
Federico Terraneo

testcase.c (text/x-csrc, 1.5 KB)

//compile with gcc -fsanitize=address -o testcase testcase.c

#include <unistd.h>

int main()
{
    execl("/usr/bin/echo", "echo",
    "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a",
    "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a",
    "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a",
    "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a",
    "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a",
    "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a",
    "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a",
    "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a",
    "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a",
    "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a",
    "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a",
    "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a",
    "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a",
    "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a",
    "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a",
    "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a",
    "a", //<--- if this is uncommented overflows buffer in stack
    NULL);
    return 1;
}