Re: ferror in ntpd (Re: bugs in printf(3))

[Todd C. Miller]

On Tue, 29 Dec 2015 13:25:16 +0100, =?utf-8?Q?J=C3=A9r=C3=A9mie_Courr=C3=A8ges-
Anglas?= wrote:

> I think it makes sense to try to recover, so calling clearerr() is
> needed.  But as said by millert you can't rely on fprintf to set the
> error indicator; the write might not be committed to disk, and the
> ftruncate and fsync calls below won't magically update the FILE's error
> indicator.
> 
> What about using "r = fflush(freqfp);" instead of ferror?

How does this look?

 - todd

Index: ntpd.c
===
RCS file: /cvs/src/usr.sbin/ntpd/ntpd.c,v
retrieving revision 1.101
diff -u -p -u -r1.101 ntpd.c
--- ntpd.c  19 Dec 2015 17:55:29 -  1.101
+++ ntpd.c  29 Dec 2015 17:51:56 -
@@ -552,12 +552,12 @@ writefreq(double d)
if (freqfp == NULL)
return 0;
rewind(freqfp);
-   fprintf(freqfp, "%.3f\n", d * 1e6); /* scale to ppm */
-   r = ferror(freqfp);
-   if (r != 0) {
+   r = fprintf(freqfp, "%.3f\n", d * 1e6); /* scale to ppm */
+   if (r < 0 || fflush(freqfp) != 0) {
if (warnonce) {
log_warnx("can't write %s", DRIFTFILE);
warnonce = 0;
+   clearerr(freqfp);
}
return 0;
}

Re: ferror in ntpd (Re: bugs in printf(3))

[Jérémie Courrèges-Anglas]

"Todd C. Miller"  writes:

> On Tue, 29 Dec 2015 13:25:16 +0100, 
> =?utf-8?Q?J=C3=A9r=C3=A9mie_Courr=C3=A8ges-
> Anglas?= wrote:
>
>> I think it makes sense to try to recover, so calling clearerr() is
>> needed.  But as said by millert you can't rely on fprintf to set the
>> error indicator; the write might not be committed to disk, and the
>> ftruncate and fsync calls below won't magically update the FILE's error
>> indicator.
>> 
>> What about using "r = fflush(freqfp);" instead of ferror?
>
> How does this look?
>
>  - todd
>
> Index: ntpd.c
> ===
> RCS file: /cvs/src/usr.sbin/ntpd/ntpd.c,v
> retrieving revision 1.101
> diff -u -p -u -r1.101 ntpd.c
> --- ntpd.c19 Dec 2015 17:55:29 -  1.101
> +++ ntpd.c29 Dec 2015 17:51:56 -
> @@ -552,12 +552,12 @@ writefreq(double d)
>   if (freqfp == NULL)
>   return 0;
>   rewind(freqfp);
> - fprintf(freqfp, "%.3f\n", d * 1e6); /* scale to ppm */
> - r = ferror(freqfp);
> - if (r != 0) {
> + r = fprintf(freqfp, "%.3f\n", d * 1e6); /* scale to ppm */
> + if (r < 0 || fflush(freqfp) != 0) {

Fine with me.

>   if (warnonce) {
>   log_warnx("can't write %s", DRIFTFILE);
>   warnonce = 0;
> + clearerr(freqfp);
>   }

Looking closer, the clearerr() call should probably be out of the
conditional.

With that fixed, ok jca@

>   return 0;
>   }
>

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: ferror in ntpd (Re: bugs in printf(3))

[Sebastian Benoit]

as jca@ says, the clearerr() should be out of the loop, so ok benno@ too.

J??r??mie Courr??ges-Anglas(j...@wxcvbn.org) on 2015.12.29 19:18:55 +0100:
> "Todd C. Miller"  writes:
> 
> > On Tue, 29 Dec 2015 13:25:16 +0100, 
> > =?utf-8?Q?J=C3=A9r=C3=A9mie_Courr=C3=A8ges-
> > Anglas?= wrote:
> >
> >> I think it makes sense to try to recover, so calling clearerr() is
> >> needed.  But as said by millert you can't rely on fprintf to set the
> >> error indicator; the write might not be committed to disk, and the
> >> ftruncate and fsync calls below won't magically update the FILE's error
> >> indicator.
> >> 
> >> What about using "r = fflush(freqfp);" instead of ferror?
> >
> > How does this look?
> >
> >  - todd
> >
> > Index: ntpd.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ntpd/ntpd.c,v
> > retrieving revision 1.101
> > diff -u -p -u -r1.101 ntpd.c
> > --- ntpd.c  19 Dec 2015 17:55:29 -  1.101
> > +++ ntpd.c  29 Dec 2015 17:51:56 -
> > @@ -552,12 +552,12 @@ writefreq(double d)
> > if (freqfp == NULL)
> > return 0;
> > rewind(freqfp);
> > -   fprintf(freqfp, "%.3f\n", d * 1e6); /* scale to ppm */
> > -   r = ferror(freqfp);
> > -   if (r != 0) {
> > +   r = fprintf(freqfp, "%.3f\n", d * 1e6); /* scale to ppm */
> > +   if (r < 0 || fflush(freqfp) != 0) {
> 
> Fine with me.
> 
> > if (warnonce) {
> > log_warnx("can't write %s", DRIFTFILE);
> > warnonce = 0;
> > +   clearerr(freqfp);
> > }
> 
> Looking closer, the clearerr() call should probably be out of the
> conditional.
> 
> With that fixed, ok jca@
> 
> > return 0;
> > }
> >
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 

-- 

Re: ferror in ntpd (Re: bugs in printf(3))

[Jérémie Courrèges-Anglas]

Sebastian Benoit  writes:

> Todd C. Miller(todd.mil...@courtesan.com) on 2015.12.28 10:46:08 -0700:
>> On Fri, 25 Dec 2015 00:30:29 +0100, Ingo Schwarze wrote:
>> 
>> > Besides, i don't see the point in messing with FILE flags at all
>> > in case of encoding errors.  As opposed to fgetwc(3) and fputwc(3),
>> > the manual doesn't document this fiddling, and POSIX doesn't ask
>> > for it.  No other conversions in printf(3) set the error indicator.
>> > It isn't required because printf(3) provides a proper error return
>> > (-1) in the first place.  Has anybody ever seen any code calling
>> > ferror(3) after printf(3)?  That just wouldn't make sense.
>> > Of course, printf(3) can result in the error indicator getting set,
>> > but only if the underlying low-level write calls fail.
>> 
>> You are correct that neither our man page nor ISO C document
>> setting the error indicator.  I get your expected output on Linux
>> and Solaris so I think this diff is correct.  I wonder if it is
>> worth documenting that the error indicator is not set in fprintf(3)?
>> 
>> We do have code in our tree that checks ferror() after doing writes
>> via fprintf.  One example src/usr.sbin/smtpd, but I have not done
>> an exhaustive check.
>
> ntpd is one of these.
>
> if the error is set, then ntpd has a bug i think, since it never calls
> clearerr()
>
> ok?

I think it makes sense to try to recover, so calling clearerr() is
needed.  But as said by millert you can't rely on fprintf to set the
error indicator; the write might not be committed to disk, and the
ftruncate and fsync calls below won't magically update the FILE's error
indicator.

What about using "r = fflush(freqfp);" instead of ferror?

> diff --git usr.sbin/ntpd/ntpd.c usr.sbin/ntpd/ntpd.c
> index cf88fe8..df7dedb 100644
> --- usr.sbin/ntpd/ntpd.c
> +++ usr.sbin/ntpd/ntpd.c
> @@ -558,6 +558,7 @@ writefreq(double d)
>   if (warnonce) {
>   log_warnx("can't write %s", DRIFTFILE);
>   warnonce = 0;
> + clearerr(freqfp);
>   }
>   return 0;
>   }



-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

ferror in ntpd (Re: bugs in printf(3))

[Sebastian Benoit]

Todd C. Miller(todd.mil...@courtesan.com) on 2015.12.28 10:46:08 -0700:
> On Fri, 25 Dec 2015 00:30:29 +0100, Ingo Schwarze wrote:
> 
> > Besides, i don't see the point in messing with FILE flags at all
> > in case of encoding errors.  As opposed to fgetwc(3) and fputwc(3),
> > the manual doesn't document this fiddling, and POSIX doesn't ask
> > for it.  No other conversions in printf(3) set the error indicator.
> > It isn't required because printf(3) provides a proper error return
> > (-1) in the first place.  Has anybody ever seen any code calling
> > ferror(3) after printf(3)?  That just wouldn't make sense.
> > Of course, printf(3) can result in the error indicator getting set,
> > but only if the underlying low-level write calls fail.
> 
> You are correct that neither our man page nor ISO C document
> setting the error indicator.  I get your expected output on Linux
> and Solaris so I think this diff is correct.  I wonder if it is
> worth documenting that the error indicator is not set in fprintf(3)?
> 
> We do have code in our tree that checks ferror() after doing writes
> via fprintf.  One example src/usr.sbin/smtpd, but I have not done
> an exhaustive check.

ntpd is one of these.

if the error is set, then ntpd has a bug i think, since it never calls
clearerr()

ok?

(sorry for hijacking the thread)

diff --git usr.sbin/ntpd/ntpd.c usr.sbin/ntpd/ntpd.c
index cf88fe8..df7dedb 100644
--- usr.sbin/ntpd/ntpd.c
+++ usr.sbin/ntpd/ntpd.c
@@ -558,6 +558,7 @@ writefreq(double d)
if (warnonce) {
log_warnx("can't write %s", DRIFTFILE);
warnonce = 0;
+   clearerr(freqfp);
}
return 0;
}