PATCH (differently) Re: PATCH: Plug some fd leaks in bin_print

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Jan 6, 10:35pm, Bart Schaefer wrote:
}
} I have some doubts about some of this.  In fact I suspect that these are
} mostly false-positives because the option letters tested in the if()
} conditions preceding each of these goto's are (or should be) mutually
} exclusive.
} 
} [...] 
} 
} This needs to be looked at more closely.

I think this is at least mostly the right thing.  There are still some
fishy bits at the very end of the function (where fout is a temp file
or memory map for printf-ing with a format string) that seem to be cases
where a write error could be incorrectly reported, but that's a separate
problem from descriptor leaks.

Does coverity still find leaks after this?  What I've done is to move
option validity checking earlier in the function so that fout should
never get opened in the first place if we're not going to make proper
use of it later.


diff --git a/Src/builtin.c b/Src/builtin.c
index 5138c70..f32c29e 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -3724,7 +3724,7 @@ int
 bin_print(char *name, char **args, Options ops, int func)
 {
     int flen, width, prec, type, argc, n, narg, curlen = 0;
-    int nnl = 0, fmttrunc = 0, ret = 0, maxarg = 0;
+    int nnl = 0, fmttrunc = 0, ret = 0, maxarg = 0, nc = 0;
     int flags[5], *len;
     char *start, *endptr, *c, *d, *flag, *buf, spec[13], *fmt = NULL;
     char **first, **argp, *curarg, *flagch = "0+- #", save = '\0', nullstr = '\0';
@@ -3837,8 +3837,41 @@ bin_print(char *name, char **args, Options ops, int func)
 	}
     }
 
+    /* -o and -O -- sort the arguments */
+    if (OPT_ISSET(ops,'o') || OPT_ISSET(ops,'O')) {
+	int flags;
+
+	if (fmt && !*args) {
+	    if (fout != stdout)
+	        fclose(fout);
+	    return 0;
+	}
+	flags = OPT_ISSET(ops,'i') ? SORTIT_IGNORING_CASE : 0;
+	if (OPT_ISSET(ops,'O'))
+	    flags |= SORTIT_BACKWARDS;
+	strmetasort(args, flags, len);
+    }
+
+    /* -C -- number of columns */
+    if (!fmt && OPT_ISSET(ops,'C')) {
+	char *eptr, *argptr = OPT_ARG(ops,'C');
+	nc = (int)zstrtol(argptr, &eptr, 10);
+	if (*eptr) {
+	    zwarnnam(name, "number expcted after -%c: %s", 'C', argptr);
+	    return 1;
+	}
+	if (nc <= 0) {
+	    zwarnnam(name, "invalid number of columns: %s", argptr);
+	    return 1;
+	}
+    }
+
     /* -u and -p -- output to other than standard output */
-    if (OPT_HASARG(ops,'u') || OPT_ISSET(ops,'p')) {
+    if ((OPT_HASARG(ops,'u') || OPT_ISSET(ops,'p')) &&
+	/* rule out conflicting options -- historical precedence */
+	((!fmt && (OPT_ISSET(ops,'c') || OPT_ISSET(ops,'C'))) ||
+	!(OPT_ISSET(ops, 'z') ||
+	  OPT_ISSET(ops, 's') || OPT_ISSET(ops, 'S')))) {
 	int fdarg, fd;
 
 	if (OPT_ISSET(ops, 'p')) {
@@ -3877,24 +3910,9 @@ bin_print(char *name, char **args, Options ops, int func)
 	}
     }
 
-    /* -o and -O -- sort the arguments */
-    if (OPT_ISSET(ops,'o') || OPT_ISSET(ops,'O')) {
-	int flags;
-
-	if (fmt && !*args) {
-	    if (fout != stdout)
-	        fclose(fout);
-	    return 0;
-	}
-	flags = OPT_ISSET(ops,'i') ? SORTIT_IGNORING_CASE : 0;
-	if (OPT_ISSET(ops,'O'))
-	    flags |= SORTIT_BACKWARDS;
-	strmetasort(args, flags, len);
-    }
-
     /* -c -- output in columns */
     if (!fmt && (OPT_ISSET(ops,'c') || OPT_ISSET(ops,'C'))) {
-	int l, nc, nr, sc, n, t, i;
+	int l, nr, sc, n, t, i;
 #ifdef MULTIBYTE_SUPPORT
 	int *widths;
 
@@ -3965,19 +3983,9 @@ bin_print(char *name, char **args, Options ops, int func)
 #endif
 
 	if (OPT_ISSET(ops,'C')) {
-	    char *eptr, *argptr = OPT_ARG(ops,'C');
-	    nc = (int)zstrtol(argptr, &eptr, 10);
-	    if (*eptr) {
-		zwarnnam(name, "number expcted after -%c: %s", 'C', argptr);
-		return 1;
-	    }
-	    if (nc <= 0) {
-		zwarnnam(name, "invalid number of columns: %s", argptr);
-		return 1;
-	    }
 	    /*
 	     * n: number of elements
-	     * nc: number of columns
+	     * nc: number of columns (above)
 	     * nr: number of rows
 	     */
 	    n = arrlen(args);