On Wed, Aug 24, 2022 at 09:00:00 +0200, Erik Skultety wrote:
> On Tue, Aug 23, 2022 at 06:43:26PM +0200, Peter Krempa wrote:
> > On Tue, Aug 23, 2022 at 18:22:37 +0200, Erik Skultety wrote:
> > > Coverity reports:
> > > virNWFilterSnoopIPLeaseUpdate(virNWFilterSnoopIPLease *ipl,
> > > time_t timeout)
> > > {
> > > if (timeout < ipl->timeout)
> > > return; /* no take-backs */
> > >
> > > virNWFilterSnoopIPLeaseTimerDel(ipl);
> > > >>> CID 396747: High impact quality (Y2K38_SAFETY)
> > > >>> A "time_t" value is stored in an integer with too few bits
> > > to accommodate it. The expression "timeout" is cast to
> > > "unsigned int".
> > > ipl->timeout = timeout;
> > > virNWFilterSnoopIPLeaseTimerAdd(ipl);
> > > }
> > >
> > > Given that it doesn't make sense for a timeout to be negative, let's
> > > store it as unsigned long long and typecast all affected time_t
> > > occurrences accordingly. Since the affected places only ever get the
> > > current time which is not going to be negative (unless it's year 2038
> > > and a 32bit architecture) we can safely cast them.
> > >
> > > Signed-off-by: Erik Skultety <eskultet@xxxxxxxxxx>
> > > ---
> > > src/nwfilter/nwfilter_dhcpsnoop.c | 12 +++++++-----
> > > 1 file changed, 7 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
> > > index 18812c0b20..0977951be1 100644
> > > --- a/src/nwfilter/nwfilter_dhcpsnoop.c
> > > +++ b/src/nwfilter/nwfilter_dhcpsnoop.c
> > > @@ -146,7 +146,7 @@ struct _virNWFilterSnoopIPLease {
> > > virSocketAddr ipAddress;
> > > virSocketAddr ipServer;
> > > virNWFilterSnoopReq * snoopReq;
> > > - unsigned int timeout;
> > > + unsigned long long timeout;
> >
> > [1]
> >
> > > /* timer list */
> > > virNWFilterSnoopIPLease *prev;
> > > virNWFilterSnoopIPLease *next;
> > > @@ -415,7 +415,8 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLease *ipl,
> > > * virNWFilterSnoopIPLeaseUpdate - update the timeout on an IP lease
> > > */
> > > static void
> > > -virNWFilterSnoopIPLeaseUpdate(virNWFilterSnoopIPLease *ipl, time_t timeout)
> > > +virNWFilterSnoopIPLeaseUpdate(virNWFilterSnoopIPLease *ipl,
> > > + unsigned long long timeout)
> > > {
> > > if (timeout < ipl->timeout)
> > > return; /* no take-backs */
> > > @@ -447,7 +448,7 @@ virNWFilterSnoopIPLeaseGetByIP(virNWFilterSnoopIPLease *start,
> > > static unsigned int
> > > virNWFilterSnoopReqLeaseTimerRun(virNWFilterSnoopReq *req)
> > > {
> > > - time_t now = time(0);
> > > + unsigned long long now = time(0);
> >
> > This should also use NULL instead of 0.
> >
> > > bool is_last = false;
> > >
> > > /* protect req->start */
> > > @@ -1580,7 +1581,8 @@ virNWFilterSnoopLeaseFileWrite(int lfd, const char *ifkey,
> > > return -1;
> > >
> > > /* time intf ip dhcpserver */
> > > - lbuf = g_strdup_printf("%u %s %s %s\n", ipl->timeout, ifkey, ipstr, dhcpstr);
> > > + lbuf = g_strdup_printf("%llu %s %s %s\n",
> > > + ipl->timeout, ifkey, ipstr, dhcpstr);
> > > len = strlen(lbuf);
> > >
> > > if (safewrite(lfd, lbuf, len) != len) {
> > > @@ -1737,7 +1739,7 @@ virNWFilterSnoopLeaseFileLoad(void)
> > > }
> > > ln++;
> > > /* key len 54 = "VMUUID"+'-'+"MAC" */
> > > - if (sscanf(line, "%u %54s %15s %15s", &ipl.timeout,
> > > + if (sscanf(line, "%llu %54s %15s %15s", &ipl.timeout,
> > > ifkey, ipstr, srvstr) < 4) {
> > > virReportError(VIR_ERR_INTERNAL_ERROR,
> > > _("virNWFilterSnoopLeaseFileLoad lease file "
> >
> > It feels to me that using a temporary unsigned long long variable to do
> > the sscanf and store everything in a time_t [1] would look more
> > appropriate as there shouldn't be a problem with time_t itself, right?
>
> There actually is (and I neglected the fact in v1) - the C standard only
> defines time_t as a signed type which can be either an integer type or even a
> float, but nothing else. Since there isn't a type formatter for time_t,
> compilers and coverity might complain in the future about signedness again (I'm
> especially worried about scanf given its nature).
Yes that's a bit weird, but that is also the reason I've suggested to
use a temporary variable for sscanf which can be unsigned long long to
have working formatters, but at the same time preserve 'time_t' where
possible.
> with unsigned long long because it's always predictable and always 64bit. To me
> storing the timeout as something else than time_t with enough bit space seems
> like a better solution.
Well I'm not sure it will help though. The man page for time() has the
following disclaimer for linux:
On Linux, a call to time() with tloc specified as NULL cannot fail
with the error EOVERFLOW, even on ABIs where time_t is a signed
32-bit integer and the clock reaches or exceeds 2**31 seconds
(2038-01-19 03:14:08 UTC, ignoring leap seconds). (POSIX.1 permits,
but does not require, the EOVERFLOW error in the case where the
seconds since the Epoch will not fit in time_t.) Instead, the
behavior on Linux is undefined when the system time is out of the
time_t range. Applications intended to run after 2038 should use
ABIs with time_t wider than 32 bits.
So once the sands of 32 bit time run out such boxes are doomed
regardless whether we use a 64 bit variable or time_t.
Anyways, once you switch to the correct way to pass a NULL pointer as
argument in both patches you can add:
Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
I'd probably still prefer though to keep time_t where we are dealing
with time and restrict the conversion only to sscanf/printf.
