On Sat, Mar 07, 2020 at 08:35:39AM +0100, Jeremie Courreges-Anglas wrote: > On Fri, Mar 06 2020, "Theo de Raadt" <[email protected]> wrote: > >> Jeremie Courreges-Anglas <[email protected]> wrote: > >> > > >> > > >> > Checking the return value of fprintf is good but not enough to ensure > >> > that data has properly been written to the file without an error. To do > >> > that we can use fflush(3) in a single place. > [redacted] > >> How about checking the return value of fclose() in output_finish() instead? > > Oh you want to reach the error-reporting code... > > And the cleanup-on-error code. Doing it here looks more appealing than > adding > if (fflush(out) != 0) > return -1; > > at the end of all the output_* functions. > > If you're careful about write errors you can avoid feeding an > incomplete/garbage file to your BGP daemon. The code was already > careful, but did not account for buffering. This is what this patch > tries to address. > > >> > Build-tested only. ok? > >> > > >> > Bonus: in output_finish(), "out = NULL;" is pointless, so zap it. > >> > I suspect it's a remnant from a time where the output FILE * was > >> > a global. That would be a separate commit. > > Diff below again for convenience, > > > Index: output.c > =================================================================== > RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v > retrieving revision 1.6 > diff -u -p -p -u -r1.6 output.c > --- output.c 6 Mar 2020 17:36:42 -0000 1.6 > +++ output.c 6 Mar 2020 23:04:18 -0000 > @@ -71,7 +71,7 @@ outputfiles(struct vrp_tree *v) > rc = 1; > continue; > } > - if ((*outputs[i].fn)(fout, v) != 0) { > + if ((*outputs[i].fn)(fout, v) != 0 || fflush(fout) != 0) { > logx("output for %s format failed", outputs[i].name); > fclose(fout); > output_cleantmp(); > @@ -112,8 +112,6 @@ void > output_finish(FILE *out) > { > fclose(out); > - out = NULL; > - > rename(output_tmpname, output_name); > output_tmpname[0] = '\0'; > } >
I think it would be better to pick up the fclose error in output_finish() and while doing that also check for rename() errors. At least those errors should be logged. -- :wq Claudio
