PATCH: Plug some fd leaks in bin_print

PATCH: Plug some fd leaks in bin_print

[Mikael Magnusson]

Found by Coverity (Issue 439120).
---
 Src/builtin.c | 60 ++++++++++++++++++++++++-----------------------------------
 1 file changed, 24 insertions(+), 36 deletions(-)

diff --git a/Src/builtin.c b/Src/builtin.c
index 13c9a96..4a780bb 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -3783,7 +3783,7 @@ bin_print(char *name, char **args, Options ops, int func)
     /* compute lengths, and interpret according to -P, -D, -e, etc. */
     argc = arrlen(args);
     len = (int *) hcalloc(argc * sizeof(int));
-    for(n = 0; n < argc; n++) {
+    for (n = 0; n < argc; n++) {
 	/* first \ sequences */
 	if (fmt ||
 	    (!OPT_ISSET(ops,'e') &&
@@ -3850,7 +3850,7 @@ bin_print(char *name, char **args, Options ops, int func)
 	    char *argptr = OPT_ARG(ops,'u'), *eptr;
 	    /* Handle undocumented feature that -up worked */
 	    if (!strcmp(argptr, "p")) {
-		fdarg= coprocout;
+		fdarg = coprocout;
 		if (fdarg < 0) {
 		    zwarnnam(name, "-p: no coprocess");
 		    return 1;
@@ -3967,12 +3967,14 @@ bin_print(char *name, char **args, Options ops, int func)
 	    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;
+		zwarnnam(name, "number expected after -%c: %s", 'C', argptr);
+		ret = 1;
+		goto out_print;
 	    }
 	    if (nc <= 0) {
 		zwarnnam(name, "invalid number of columns: %s", argptr);
-		return 1;
+		ret = 1;
+		goto out_print;
 	    }
 	    /*
 	     * n: number of elements
@@ -4052,13 +4054,7 @@ bin_print(char *name, char **args, Options ops, int func)
 	    }
 	    fputc(OPT_ISSET(ops,'N') ? '\0' : '\n', fout);
 	}
-	/* Testing EBADF special-cases >&- redirections */
-	if ((fout != stdout) ? (fclose(fout) != 0) :
-	    (fflush(fout) != 0 && errno != EBADF)) {
-            zwarnnam(name, "write error: %e", errno);
-            ret = 1;
-	}
-	return ret;
+	goto out_print;
     }
 
     /* normal output */
@@ -4076,7 +4072,7 @@ bin_print(char *name, char **args, Options ops, int func)
 	    queue_signals();
 	    zpushnode(bufstack, sepjoin(args, NULL, 0));
 	    unqueue_signals();
-	    return 0;
+	    goto out_print;
 	}
 	/* -s option -- add the arguments to the history list */
 	if (OPT_ISSET(ops,'s') || OPT_ISSET(ops,'S')) {
@@ -4092,7 +4088,8 @@ bin_print(char *name, char **args, Options ops, int func)
 		    short *words;
 		    if (nwords > 1) {
 			zwarnnam(name, "option -S takes a single argument");
-			return 1;
+			ret = 1;
+			goto out_print;
 		    }
 		    words = NULL;
 		    wordsize = 0;
@@ -4134,13 +4131,7 @@ bin_print(char *name, char **args, Options ops, int func)
 	}
 	if (!(OPT_ISSET(ops,'n') || nnl))
 	    fputc(OPT_ISSET(ops,'N') ? '\0' : '\n', fout);
-	/* Testing EBADF special-cases >&- redirections */
-	if ((fout != stdout) ? (fclose(fout) != 0) :
-	    (fflush(fout) != 0 && errno != EBADF)) {
-            zwarnnam(name, "write error: %e", errno);
-            ret = 1;
-	}
-	return ret;
+	goto out_print;
     }
 
     /*
@@ -4150,6 +4141,9 @@ bin_print(char *name, char **args, Options ops, int func)
      */
 
     if (OPT_ISSET(ops,'z') || OPT_ISSET(ops,'s')) {
+	if (fout != stdout)
+	    fclose(fout);
+	fout = stdout;
 #ifdef HAVE_OPEN_MEMSTREAM
     	if ((fout = open_memstream(&buf, &mcount)) == NULL)
 	    zwarnnam(name, "open_memstream failed");
@@ -4200,9 +4194,8 @@ bin_print(char *name, char **args, Options ops, int func)
 		    if (narg > argc) {
 		    	zwarnnam(name, "%d: argument specifier out of range",
 				 narg);
-			if (fout != stdout)
-			    fclose(fout);
-			return 1;
+			ret = 1;
+			goto out_print;
 		    } else {
 		    	if (narg > maxarg) maxarg = narg;
 		    	curarg = *(first + narg - 1);
@@ -4233,9 +4226,8 @@ bin_print(char *name, char **args, Options ops, int func)
 		    	    zwarnnam(name,
 				     "%d: argument specifier out of range",
 				     narg);
-			    if (fout != stdout)
-				fclose(fout);
-			    return 1;
+			    ret = 1;
+			    goto out_print;
 			} else {
 		    	    if (narg > maxarg) maxarg = narg;
 		    	    argp = first + narg - 1;
@@ -4262,9 +4254,8 @@ bin_print(char *name, char **args, Options ops, int func)
 		    		zwarnnam(name,
 					 "%d: argument specifier out of range",
 					 narg);
-				if (fout != stdout)
-				    fclose(fout);
-				return 1;
+				ret = 1;
+				goto out_print;
 			    } else {
 		    		if (narg > maxarg) maxarg = narg;
 		    		argp = first + narg - 1;
@@ -4414,12 +4405,8 @@ bin_print(char *name, char **args, Options ops, int func)
 		}
 		zwarnnam(name, "%s: invalid directive", start);
 		if (*c) c[1] = save;
-		/* Testing EBADF special-cases >&- redirections */
-		if ((fout != stdout) ? (fclose(fout) != 0) :
-		    (fflush(fout) != 0 && errno != EBADF)) {
-		    zwarnnam(name, "write error: %e", errno);
-		}
-		return 1;
+		ret = 1;
+		goto out_print;
 	    }
 
 	    if (type > 0) {
@@ -4536,6 +4523,7 @@ bin_print(char *name, char **args, Options ops, int func)
 	unqueue_signals();
     }
 
+out_print:
     /* Testing EBADF special-cases >&- redirections */
     if ((fout != stdout) ? (fclose(fout) != 0) :
 	(fflush(fout) != 0 && errno != EBADF)) {
-- 
2.2.0.GIT

Re: PATCH: Plug some fd leaks in bin_print

[Bart Schaefer]

On Jan 7,  1:25am, Mikael Magnusson wrote:
} Subject: PATCH: Plug some fd leaks in bin_print
}
} Found by Coverity (Issue 439120).

Adding a whole bunch of "goto" is really the best way to fix this?

Re: PATCH: Plug some fd leaks in bin_print

[Mikael Magnusson]

On Wed, Jan 7, 2015 at 2:21 AM, Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jan 7,  1:25am, Mikael Magnusson wrote:
> } Subject: PATCH: Plug some fd leaks in bin_print
> }
> } Found by Coverity (Issue 439120).
>
> Adding a whole bunch of "goto" is really the best way to fix this?

It seemed nicer than copying that four line mess of ternary operators
to 10 different places.

-- 
Mikael Magnusson

Re: PATCH: Plug some fd leaks in bin_print

[Bart Schaefer]

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.

On Jan 7,  1:25am, Mikael Magnusson wrote:
}
}  	    if (*eptr) {
} -		zwarnnam(name, "number expcted after -%c: %s", 'C', argptr);
} -		return 1;
} +		zwarnnam(name, "number expected after -%c: %s", 'C', argptr);
} +		ret = 1;
} +		goto out_print;
}  	    }
}  	    if (nc <= 0) {
}  		zwarnnam(name, "invalid number of columns: %s", argptr);
} -		return 1;
} +		ret = 1;
} +		goto out_print;
}  	    }

I'm not sure what coverity had to say here, but these can't be right.  The
descriptor should NOT be fflush()d on this error.

}  	    queue_signals();
}  	    zpushnode(bufstack, sepjoin(args, NULL, 0));
}  	    unqueue_signals();
} -	    return 0;
} +	    goto out_print;

This can't be right either; there shouldn't be any output when pushing on
the buffer stack, so again we shouldn't be fflush()ing.


}  			zwarnnam(name, "option -S takes a single argument");
} -			return 1;
} +			ret = 1;
} +			goto out_print;

Same complaint.

}      if (OPT_ISSET(ops,'z') || OPT_ISSET(ops,'s')) {
} +	if (fout != stdout)
} +	    fclose(fout);
} +	fout = stdout;

Why is that needed when the very next couple of lines are going to over-
write fout with a different open?

I'm just going to quit complaining about the rest of these; I think
they're all incorrect.  If nothing else, they shouldn't introduce the
possibility of printing BOTH an argument-parsing error AND a write-
failed error.

This needs to be looked at more closely.

-- 
Barton E. Schaefer

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

[Bart Schaefer]

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

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

[Bart Schaefer]

On Jan 6, 11:08pm, Bart Schaefer wrote:
}
} 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.

Oops, forgot to delete a couple of lines from a copy-paste:

} +	if (fmt && !*args) {
} +	    if (fout != stdout)
} +	        fclose(fout);
} +	    return 0;
} +	}

fout is always stdout there, so that can just be

+	if (fmt && !*args)
+	    return 0;

I won't resend the entire diff unless somebody needs it.

Re: PATCH: Plug some fd leaks in bin_print

[Mikael Magnusson]

On Wed, Jan 7, 2015 at 7:35 AM, Bart Schaefer <schaefer@brasslantern.com> 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.
>
> On Jan 7,  1:25am, Mikael Magnusson wrote:
> }
> }           if (*eptr) {
> } -             zwarnnam(name, "number expcted after -%c: %s", 'C', argptr);
> } -             return 1;
> } +             zwarnnam(name, "number expected after -%c: %s", 'C', argptr);
> } +             ret = 1;
> } +             goto out_print;
> }           }
> }           if (nc <= 0) {
> }               zwarnnam(name, "invalid number of columns: %s", argptr);
> } -             return 1;
> } +             ret = 1;
> } +             goto out_print;
> }           }
>
> I'm not sure what coverity had to say here, but these can't be right.  The
> descriptor should NOT be fflush()d on this error.

Coverity only complains about the missing close, I figured fflushing
stdout was harmless and unifying the error cleanup paths was neater.

> }           queue_signals();
> }           zpushnode(bufstack, sepjoin(args, NULL, 0));
> }           unqueue_signals();
> } -         return 0;
> } +         goto out_print;
>
> This can't be right either; there shouldn't be any output when pushing on
> the buffer stack, so again we shouldn't be fflush()ing.
>
>
> }                       zwarnnam(name, "option -S takes a single argument");
> } -                     return 1;
> } +                     ret = 1;
> } +                     goto out_print;
>
> Same complaint.
>
> }      if (OPT_ISSET(ops,'z') || OPT_ISSET(ops,'s')) {
> } +     if (fout != stdout)
> } +         fclose(fout);
> } +     fout = stdout;
>
> Why is that needed when the very next couple of lines are going to over-
> write fout with a different open?

Because if they fail to open their thing, fout will continue to point
to something that was closed?

> I'm just going to quit complaining about the rest of these; I think
> they're all incorrect.  If nothing else, they shouldn't introduce the
> possibility of printing BOTH an argument-parsing error AND a write-
> failed error.
>
> This needs to be looked at more closely.

Okay, I'll resend a version that just does the fclose when things are
actually leaking, and leave the others alone. I didn't realize
fflush()ing stdout was somehow dangerous sometimes.

-- 
Mikael Magnusson

Re: PATCH: Plug some fd leaks in bin_print

[Mikael Magnusson]

On Wed, Jan 7, 2015 at 9:04 AM, Mikael Magnusson <mikachu@gmail.com> wrote:
> On Wed, Jan 7, 2015 at 7:35 AM, Bart Schaefer <schaefer@brasslantern.com> wrote:
>> This needs to be looked at more closely.
>
> Okay, I'll resend a version that just does the fclose when things are
> actually leaking, and leave the others alone. I didn't realize
> fflush()ing stdout was somehow dangerous sometimes.

I should have looked at all new mails before replying to this one,
oops, never mind :).

-- 
Mikael Magnusson

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

[Mikael Magnusson]

On Wed, Jan 7, 2015 at 8:14 AM, Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jan 6, 11:08pm, Bart Schaefer wrote:
> }
> } 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.

Only Clint and Oliver have admin access, so I can't actually upload
new code to coverity to check. Maybe if either of them see this they
could add me/us as admins too?

-- 
Mikael Magnusson

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

[Bart Schaefer]

On Jan 7,  9:26am, Mikael Magnusson wrote:
} Subject: Re: PATCH (differently) Re: PATCH: Plug some fd leaks in bin_prin
}
} On Wed, Jan 7, 2015 at 8:14 AM, Bart Schaefer <schaefer@brasslantern.com> wrote:
} > On Jan 6, 11:08pm, Bart Schaefer wrote:
} > }
} > } 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.
} 
} Only Clint and Oliver have admin access, so I can't actually upload
} new code to coverity to check. Maybe if either of them see this they
} could add me/us as admins too?

I can push this to the master git a bit later today if nobody sees any
obvious problems with it.  "make check" does still pass.

You can see from that one hunk that the precedence of options is a bit
wacky.  -c / -C win if there is no format string but -z / -s win if
there is one.  I guess that sort of makes sense because columnation of
formatted output isn't supported.