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?
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
