[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 30 Jan 2018 16:33:53 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] faccessat: fix error code on setreXid failure
On Tue, Jan 30, 2018 at 11:32:37PM +0300, Alexander Monakov wrote:
> Commit 316d6741b68b485205d7233c98bd6c795bb80370 changed one use of
> SYS_exit in 'checker' without changing another just three lines above,
> and then commit f9fb20b42da0e755d93de229a5a737d79a0e8f60 changed the
> meaning of return value, causing EACCES to be reported instead of EBUSY
> if preparatory setregid/setreuid fail.
This looks right.
> This is the minimal fix for the issue, but it appears there's another:
> collecting checker's exit code and reaping the zombie is implemented as
>
> int status;
> do {
> __syscall(SYS_wait4, pid, &status, __WCLONE, 0);
> } while (!WIFEXITED(status) && !WIFSIGNALED(status));
>
> but I don't understand why this retry loop is required and correct:
AFAIK wait4 can also return due to Stopped status or trace-related
reasons, not just exit. That was the motivation I think.
> - if another thread won the race to collect the zombie by doing something
> like waitpid(-1, 0, __WALL), it fails to check syscall's return value and
> uses uninitialized 'status', possibly causing an infinite loop or OOB access
> in the parent;
Yes, I think that should be fixed even though I think it's broken
usage.
> - the code seems to assume that the zombie will not be auto-collected even if
> SIGCHLD disposition is set to SIG_IGN; this sounds logical, but not explicitly
> documented as far as I can tell;
Indeed, I'm not sure, but I don't know any good fix.
> - if the two problems above don't arise, I don't see how the test in while ()
> condition can fail; we have signals blocked, so waitpid can only return when
> the child no longer exists.
>
> Plus, using CLONE_VM | CLONE_VFORK would help conserve resources.
I'm not sure if it's safe -- having tasks sharing vm with different
permissions is usually a very bad thing. It might be ok here but I'm
not sure.
>
> Alexander
>
> src/unistd/faccessat.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/unistd/faccessat.c b/src/unistd/faccessat.c
> index 33478959..954cbdb4 100644
> --- a/src/unistd/faccessat.c
> +++ b/src/unistd/faccessat.c
> @@ -25,7 +25,7 @@ static int checker(void *p)
> int i;
> if (__syscall(SYS_setregid, __syscall(SYS_getegid), -1)
> || __syscall(SYS_setreuid, __syscall(SYS_geteuid), -1))
> - __syscall(SYS_exit, 1);
> + return sizeof errors/sizeof *errors - 1;
> ret = __syscall(SYS_faccessat, c->fd, c->filename, c->amode, 0);
> for (i=0; i < sizeof errors/sizeof *errors - 1 && ret!=errors[i]; i++);
> return i;
Looks ok except it encodes an assumption that EBUSY is last. It might
make more sense to goto the errno-searching loop.
Rich
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.
