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

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(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? 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).

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.