From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/12429 Path: news.gmane.org!.POSTED!not-for-mail From: Alexander Monakov Newsgroups: gmane.linux.lib.musl.general Subject: [PATCH] faccessat: fix error code on setreXid failure Date: Tue, 30 Jan 2018 23:32:37 +0300 Message-ID: <20180130203237.4580-1-amonakov@ispras.ru> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org X-Trace: blaine.gmane.org 1517344277 21107 195.159.176.226 (30 Jan 2018 20:31:17 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Tue, 30 Jan 2018 20:31:17 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-12445-gllmg-musl=m.gmane.org@lists.openwall.com Tue Jan 30 21:31:13 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 1egcYK-0003un-UK for gllmg-musl@m.gmane.org; Tue, 30 Jan 2018 21:30:49 +0100 Original-Received: (qmail 13603 invoked by uid 550); 30 Jan 2018 20:32:50 -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 13542 invoked from network); 30 Jan 2018 20:32:48 -0000 X-Mailer: git-send-email 2.11.0 Xref: news.gmane.org gmane.linux.lib.musl.general:12429 Archived-At: 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 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: - 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; - 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; - 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. 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; -- 2.11.0