From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4342 invoked from network); 15 Mar 2009 01:00:57 -0000 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-2.6 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.2.5 Received: from news.dotsrc.org (HELO a.mx.sunsite.dk) (130.225.247.88) by ns1.primenet.com.au with SMTP; 15 Mar 2009 01:00:57 -0000 Received-SPF: none (ns1.primenet.com.au: domain at sunsite.dk does not designate permitted sender hosts) Received: (qmail 46880 invoked from network); 15 Mar 2009 00:58:46 -0000 Received: from sunsite.dk (130.225.247.90) by a.mx.sunsite.dk with SMTP; 15 Mar 2009 00:58:46 -0000 Received: (qmail 25078 invoked by alias); 15 Mar 2009 00:58:41 -0000 Mailing-List: contact zsh-workers-help@sunsite.dk; run by ezmlm Precedence: bulk X-No-Archive: yes X-Seq: 26735 Received: (qmail 25067 invoked from network); 15 Mar 2009 00:58:41 -0000 Received: from bifrost.dotsrc.org (130.225.254.106) by sunsite.dk with SMTP; 15 Mar 2009 00:58:41 -0000 Received: from randymail-a2.g.dreamhost.com (balanced.mail.policyd.dreamhost.com [208.97.132.119]) by bifrost.dotsrc.org (Postfix) with ESMTP id AC06D80307F8 for ; Sun, 15 Mar 2009 01:58:36 +0100 (CET) Received: from blorf.net (dsl-74-220-65-6.cruzio.com [74.220.65.6]) by randymail-a2.g.dreamhost.com (Postfix) with ESMTP id 05FD0EE2EE for ; Sat, 14 Mar 2009 17:58:31 -0700 (PDT) Date: Sat, 14 Mar 2009 17:58:26 -0700 From: Wayne Davison To: zsh-workers@sunsite.dk Subject: Improving some return-value checking Message-ID: <20090315005826.GC18440@blorf.net> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="Nq2Wo0NMKNjxTN9z" Content-Disposition: inline User-Agent: Mutt/1.5.15+20070412 (2007-04-11) X-Virus-Scanned: ClamAV 0.92.1/9109/Sat Mar 14 23:35:08 2009 on bifrost X-Virus-Status: Clean --Nq2Wo0NMKNjxTN9z Content-Type: text/plain; charset=us-ascii Content-Disposition: inline The compiler on my newer Linux box warns about a lot of unchecked return values, so I'm starting to clean this up. Attached are a couple patches that make headway in this area: I hope the first in uncontroversial, simple improvements, and I'm going to check it in. The second is specific to checking the return value of pipe(), and there is some cleanup there that I'm not too sure about yet. For instance, I'm curious if my disabling of the WC_SUBLIST_COPROC bit on failure is a good idea or not. There's also a couple FIXME spots where I wasn't sure how to return an error which I will look at this again when I have more time (but feel free to chime in if you have some suggestions). This patch will not be committed as-is. Finally, I haven't yet patched the various read()/write()/fwrite() calls that are not checking their return values. I'm thinking about adding a helper function for each one that would iterate over its buffer as needed to handle partial read/write or a signal-interrupt. ..wayne.. --Nq2Wo0NMKNjxTN9z Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="return-check1.diff" index f22b23f..0b991c5 100644 --- a/Src/Modules/files.c +++ b/Src/Modules/files.c @@ -428,7 +428,8 @@ recursivecmd(char *nam, int opt_noerr, int opt_recurse, int opt_safe, if ((err & 2) && ds.dirfd >= 0 && restoredir(&ds) && zchdir(pwd)) { zsfree(pwd); pwd = ztrdup("/"); - chdir(pwd); + if (chdir(pwd) < 0) + zwarn("failed to chdir(%s): %e", pwd, errno); } if (ds.dirfd >= 0) close(ds.dirfd); index d825c7f..422c707 100644 --- a/Src/Modules/mapfile.c +++ b/Src/Modules/mapfile.c @@ -92,7 +92,8 @@ setpmmapfile(Param pm, char *value) * First we need to make sure the file is long enough for * when we msync. On AIX, at least, we just get zeroes otherwise. */ - ftruncate(fd, len); + if (ftruncate(fd, len) < 0) + zwarn("ftruncate failed: %e", errno); memcpy(mmptr, value, len); #ifndef MS_SYNC #define MS_SYNC 0 @@ -102,7 +103,8 @@ setpmmapfile(Param pm, char *value) * Then we need to truncate again, since mmap() always maps complete * pages. Honestly, I tried it without, and you need both. */ - ftruncate(fd, len); + if (ftruncate(fd, len) < 0) + zwarn("ftruncate failed: %e", errno); munmap(mmptr, len); } #else /* don't USE_MMAP */ index 12a9f0d..0dc9486 100644 --- a/Src/Modules/zftp.c +++ b/Src/Modules/zftp.c @@ -2043,8 +2043,9 @@ zfgetinfo(char *prompt, int noecho) fflush(stderr); } - fgets(instr, 256, stdin); - if (instr[len = strlen(instr)-1] == '\n') + if (fgets(instr, 256, stdin) == NULL) + instr[len = 0] = '\0'; + else if (instr[len = strlen(instr)-1] == '\n') instr[len] = '\0'; strret = dupstring(instr); index 050101f..95aca06 100644 --- a/Src/builtin.c +++ b/Src/builtin.c @@ -795,16 +795,16 @@ bin_cd(char *nam, char **argv, Options ops, int func) setjobpwd(); zsfree(pwd); pwd = metafy(zgetcwd(), -1, META_DUP); - } else if (stat(".", &st2) < 0) - chdir(unmeta(pwd)); - else if (st1.st_ino != st2.st_ino || st1.st_dev != st2.st_dev) { + } else if (stat(".", &st2) < 0) { + if (chdir(unmeta(pwd)) < 0) + zwarn("unable to chdir(%s): %e", pwd, errno); + } else if (st1.st_ino != st2.st_ino || st1.st_dev != st2.st_dev) { if (chasinglinks) { setjobpwd(); zsfree(pwd); pwd = metafy(zgetcwd(), -1, META_DUP); - } else { - chdir(unmeta(pwd)); - } + } else if (chdir(unmeta(pwd)) < 0) + zwarn("unable to chdir(%s): %e", pwd, errno); } unqueue_signals(); return 0; index ed7c087..4f15ebe 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -2722,7 +2722,8 @@ execcmd(Estate state, int input, int output, int how, int last1) #ifdef HAVE_NICE /* Check if we should run background jobs at a lower priority. */ if ((how & Z_ASYNC) && isset(BGNICE)) - nice(5); + if (nice(5) < 0) + zwarn("nice(5) failed: %e", errno); #endif /* HAVE_NICE */ } else if (is_cursh) { index 38ceac3..637976d 100644 --- a/Src/hist.c +++ b/Src/hist.c @@ -2291,9 +2291,9 @@ savehistfile(char *fn, int err, int writeflags) #ifdef HAVE_FCHMOD if (old_exists && out) { #ifdef HAVE_FCHOWN - fchown(fileno(out), sb.st_uid, sb.st_gid); + if (fchown(fileno(out), sb.st_uid, sb.st_gid) < 0) {} /* IGNORE FAILURE */ #endif - fchmod(fileno(out), sb.st_mode); + if (fchmod(fileno(out), sb.st_mode) < 0) {} /* IGNORE FAILURE */ } #endif } index 7a983d4..340ceb8 100644 --- a/Src/utils.c +++ b/Src/utils.c @@ -5422,7 +5422,8 @@ lchdir(char const *path, struct dirsav *d, int hard) } #ifdef HAVE_LSTAT if (*path == '/') - chdir("/"); + if (chdir("/") < 0) + zwarn("failed to chdir(/): %e", errno); for(;;) { while(*path == '/') path++; --Nq2Wo0NMKNjxTN9z Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="return-check2.diff" index 4f15ebe..bc7bfba 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -1327,11 +1327,19 @@ execpline(Estate state, wordcode slcode, int how, int last1) zclose(coprocin); zclose(coprocout); } - mpipe(ipipe); - mpipe(opipe); - coprocin = ipipe[0]; - coprocout = opipe[1]; - fdtable[coprocin] = fdtable[coprocout] = FDT_UNUSED; + if (mpipe(ipipe) < 0) { + coprocin = coprocout = -1; + slflags &= ~WC_SUBLIST_COPROC; + } else if (mpipe(opipe) < 0) { + close(ipipe[0]); + close(ipipe[1]); + coprocin = coprocout = -1; + slflags &= ~WC_SUBLIST_COPROC; + } else { + coprocin = ipipe[0]; + coprocout = opipe[1]; + fdtable[coprocin] = fdtable[coprocout] = FDT_UNUSED; + } } /* This used to set list_pipe_pid=0 unconditionally, but in things * like `ls|if true; then sleep 20; cat; fi' where the sleep was @@ -1438,16 +1446,17 @@ execpline(Estate state, wordcode slcode, int how, int last1) ((jn->stat & STAT_STOPPED) || (list_pipe_job && pline_level && (jobtab[list_pipe_job].stat & STAT_STOPPED)))) { - pid_t pid; + pid_t pid = 0; int synch[2]; struct timeval bgtime; - pipe(synch); - - if ((pid = zfork(&bgtime)) == -1) { + if (pipe(synch) < 0 || (pid = zfork(&bgtime)) == -1) { + if (pid < 0) { + close(synch[0]); + close(synch[1]); + } else + zerr("pipe failed: %e", errno); zleentry(ZLE_CMD_TRASH); - close(synch[0]); - close(synch[1]); fprintf(stderr, "zsh: job can't be suspended\n"); fflush(stderr); makerunning(jn); @@ -1568,7 +1577,9 @@ execpline2(Estate state, wordcode pcode, for (pc = state->pc; wc_code(code = *pc) == WC_REDIR; pc += WC_REDIR_WORDS(code)); - mpipe(pipes); + if (mpipe(pipes) < 0) { + /* FIXME */ + } /* if we are doing "foo | bar" where foo is a current * * shell command, do foo in a subshell and do the * @@ -1577,8 +1588,9 @@ execpline2(Estate state, wordcode pcode, int synch[2]; struct timeval bgtime; - pipe(synch); - if ((pid = zfork(&bgtime)) == -1) { + if (pipe(synch) < 0) { + zerr("pipe failed: %e", errno); + } else if ((pid = zfork(&bgtime)) == -1) { close(synch[0]); close(synch[1]); } else if (pid) { @@ -1995,7 +2007,9 @@ addfd(int forked, int *save, struct multio **mfds, int fd1, int fd2, int rflag, return; } mfds[fd1]->fds[1] = fdN; - mpipe(pipes); + if (mpipe(pipes) < 0) { + /* FIXME */ + } mfds[fd1]->pipe = pipes[1 - rflag]; redup(pipes[rflag], fd1); mfds[fd1]->ct = 2; @@ -2671,9 +2685,12 @@ execcmd(Estate state, int input, int output, int how, int last1) struct timeval bgtime; child_block(); - pipe(synch); - - if ((pid = zfork(&bgtime)) == -1) { + if (pipe(synch) < 0) { + zerr("pipe failed: %e", errno); + if (oautocont >= 0) + opts[AUTOCONTINUE] = oautocont; + return; + } else if ((pid = zfork(&bgtime)) == -1) { close(synch[0]); close(synch[1]); if (oautocont >= 0) @@ -3468,7 +3485,11 @@ getoutput(char *cmd, int qt) } return readoutput(stream, qt); } - mpipe(pipes); + if (mpipe(pipes) < 0) { + errflag = 1; + cmdoutpid = 0; + return NULL; + } child_block(); cmdoutval = 0; if ((cmdoutpid = pid = zfork(NULL)) == -1) { @@ -3728,7 +3749,8 @@ getproc(char *cmd, char **eptr) pnam = hcalloc(strlen(PATH_DEV_FD) + 6); if (!(prog = parsecmd(cmd, eptr))) return NULL; - mpipe(pipes); + if (mpipe(pipes) < 0) + return NULL; if ((pid = zfork(&bgtime))) { sprintf(pnam, "%s/%d", PATH_DEV_FD, pipes[!out]); zclose(pipes[out]); @@ -3782,7 +3804,8 @@ getpipe(char *cmd, int nullexec) zerr("invalid syntax for process substitution in redirection"); return -1; } - mpipe(pipes); + if (mpipe(pipes) < 0) + return -1; if ((pid = zfork(&bgtime))) { zclose(pipes[out]); if (pid == -1) { @@ -3806,12 +3829,16 @@ getpipe(char *cmd, int nullexec) /* open pipes with fds >= 10 */ /**/ -static void +static int mpipe(int *pp) { - pipe(pp); + if (pipe(pp) < 0) { + zerr("pipe failed: %e", errno); + return -1; + } pp[0] = movefd(pp[0]); pp[1] = movefd(pp[1]); + return 0; } /* --Nq2Wo0NMKNjxTN9z--