Improving some return-value checking

Improving some return-value checking

[Wayne Davison]

[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]

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..

[-- Attachment #2: return-check1.diff --]
[-- Type: text/x-diff, Size: 3633 bytes --]

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++;

[-- Attachment #3: return-check2.diff --]
[-- Type: text/x-diff, Size: 4248 bytes --]

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;
 }
 
 /*

Re: Improving some return-value checking

[Wayne Davison]

[-- Attachment #1: Type: text/plain, Size: 811 bytes --]

On Sat, Mar 14, 2009 at 4:58 PM, Wayne Davison <wayned@users.sourceforge.net
> wrote:

> The second is specific to checking the return value of pipe() [...]
> I haven't yet patched the various read()/write()/fwrite() calls
> that are not checking their return values
>

I finally got back to this, and checked in some cleanup for both the pipe()
calls and the read()/write() calls that were causing compiler warnings.
 This gets rid of all the "ignoring return value" compiler warnings, and
should make some of the read/write calls slightly safer (since they now
handle EINTR).  There is one remaining FIXME (in exec.c) where I didn't
error-handle an mpipe() call.  Since it wasn't error-handled before, it will
be fine for now, but we may want to figure out how to do an error exit from
that point.

..wayne..