Re: PATCH: Plug some fd leaks in bin_print

[Mikael Magnusson <mikachu@xxxxxxxxx>]

On Wed, Jan 7, 2015 at 7:35 AM, Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> 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