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

[Erik Skultety]

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). Therefore I decided to go
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.

Erik