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

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(a)redhat.com&gt; > --- > 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?