[libvirt PATCH v2 2/2] nwfilter: Fix timeout data type reported by coverity

[Peter Krempa]

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 at redhat.com>
> > > ---
> > >  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 at redhat.com>

I'd probably still prefer though to keep time_t where we are dealing
with time and restrict the conversion only to sscanf/printf.