Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sun, 21 Feb 2016 12:51:30 +0300 (MSK)
From: Alexander Monakov <amonakov@...ras.ru>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] slim down and avoid undefined behavior in
 unsetenv

... aaand the OCD kicks in :)

On Sat, 20 Feb 2016, Alexander Monakov wrote:
> diff --git a/src/env/unsetenv.c b/src/env/unsetenv.c
> index 3569335..f0f369f 100644
> --- a/src/env/unsetenv.c
> +++ b/src/env/unsetenv.c
[snip]
>  	size_t l = strlen(name);
>  
> -	if (!*name || strchr(name, '=')) {
> +	if (!*name || memchr(name, '=', l)) {

Here I could have changed '!*name' to '!l' for a small cleanup as well.

>  		errno = EINVAL;
>  		return -1;
>  	}

Some places in musl tail-call to __syscall_ret(-Exxx) to set errno. I wonder
if it's accidental or there's a guideline for using one style or the other?
The only place I imagine the tailcall style might be undesired is sem_trywait,
where returning failure is not expected to be rare.

What do you think about a change that introduces __set_errno that accepts
positive errno and returns -1L? With that change __syscall_ret can become

    return r < -4095UL ? r : __set_errno(-r);

> -again:
[snip]
> +	for (char **e = __environ; *e; )
> +		if (!memcmp(name, *e, l) && l[*e] == '=') {

Here the usage of memcmp requires that it scans buffers left-to-right and
stops on first mismatch. As I understand the standards do not guarantee that,
but musl's current implementation does, and is not interposable. Still, a
gotcha.

Alexander

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.