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

[Peter Krempa]

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?