On Mon, Mar 09, 2020 at 09:41:37AM +0100, Jeremie Courreges-Anglas wrote:
> On Sat, Mar 07 2020, Claudio Jeker <[email protected]> wrote:
> > 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.
>
> Agreed. Here's an updated diff that tests the return value of
> output_finish(). Suggestions welcome for the error message in this
> case.
>
> It also drops the extra "out = NULL;", and replaces logx() calls with
> warn(3) in this file. I see no reason to drop those messages and errno
> information. logx() seems used mostly for statistics.
>
> Thoughts, tests / oks?
Works for me. Ok claudio@
PS: I think some of the output_* prototypes could be moved to output.c
since those functions are only used internally.
> Index: extern.h
> ===================================================================
> RCS file: /d/cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.24
> diff -u -p -r1.24 extern.h
> --- extern.h 6 Mar 2020 17:36:42 -0000 1.24
> +++ extern.h 9 Mar 2020 08:19:20 -0000
> @@ -372,7 +372,7 @@ extern char* outputdir;
> int outputfiles(struct vrp_tree *v);
> FILE *output_createtmp(char *);
> void output_cleantmp(void);
> -void output_finish(FILE *);
> +int output_finish(FILE *);
> int output_bgpd(FILE *, struct vrp_tree *);
> int output_bird1v4(FILE *, struct vrp_tree *);
> int output_bird1v6(FILE *, struct vrp_tree *);
> Index: output.c
> ===================================================================
> RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 output.c
> --- output.c 6 Mar 2020 17:36:42 -0000 1.6
> +++ output.c 9 Mar 2020 08:20:39 -0000
> @@ -67,18 +67,23 @@ outputfiles(struct vrp_tree *v)
>
> fout = output_createtmp(outputs[i].name);
> if (fout == NULL) {
> - logx("cannot create %s", outputs[i].name);
> + warn("cannot create %s", outputs[i].name);
> rc = 1;
> continue;
> }
> if ((*outputs[i].fn)(fout, v) != 0) {
> - logx("output for %s format failed", outputs[i].name);
> + warn("output for %s format failed", outputs[i].name);
> fclose(fout);
> output_cleantmp();
> rc = 1;
> continue;
> }
> - output_finish(fout);
> + if (output_finish(fout) != 0) {
> + warn("finish for %s format failed", outputs[i].name);
> + output_cleantmp();
> + rc = 1;
> + continue;
> + }
> }
>
> return rc;
> @@ -108,14 +113,15 @@ output_createtmp(char *name)
> return f;
> }
>
> -void
> +int
> output_finish(FILE *out)
> {
> - fclose(out);
> - out = NULL;
> -
> - rename(output_tmpname, output_name);
> + if (fclose(out) != 0)
> + return -1;
> + if (rename(output_tmpname, output_name) == -1)
> + return -1;
> output_tmpname[0] = '\0';
> + return 0;
> }
>
> void
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
>
--
:wq Claudio
