From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/12430 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] faccessat: fix error code on setreXid failure Date: Tue, 30 Jan 2018 16:33:53 -0500 Message-ID: <20180130213353.GM1627@brightrain.aerifal.cx> References: <20180130203237.4580-1-amonakov@ispras.ru> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: blaine.gmane.org 1517347929 32192 195.159.176.226 (30 Jan 2018 21:32:09 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Tue, 30 Jan 2018 21:32:09 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-12446-gllmg-musl=m.gmane.org@lists.openwall.com Tue Jan 30 22:32:05 2018 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1egdVc-00080R-3b for gllmg-musl@m.gmane.org; Tue, 30 Jan 2018 22:32:04 +0100 Original-Received: (qmail 11722 invoked by uid 550); 30 Jan 2018 21:34:06 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 11699 invoked from network); 30 Jan 2018 21:34:06 -0000 Content-Disposition: inline In-Reply-To: <20180130203237.4580-1-amonakov@ispras.ru> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:12430 Archived-At: 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