[v1] nwfilter: Fix timeout data type reported by coverity

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

Posted by Erik Skultety 1 year, 8 months ago

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);
    }

This is a safe fix, since time_t is just long int and scales
automatically with platform (more specifically it's 64bit on all
platforms we care about).

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 src/nwfilter/nwfilter_dhcpsnoop.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
index a10a14cfc1..4133d4c672 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;
+    time_t timeout;
     /* timer list */
     virNWFilterSnoopIPLease *prev;
     virNWFilterSnoopIPLease *next;
@@ -1580,7 +1580,7 @@ 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("%lu %s %s %s\n", ipl->timeout, ifkey, ipstr, dhcpstr);
     len = strlen(lbuf);
 
     if (safewrite(lfd, lbuf, len) != len) {
@@ -1739,7 +1739,7 @@ virNWFilterSnoopLeaseFileLoad(void)
         }
         ln++;
         /* key len 54 = "VMUUID"+'-'+"MAC" */
-        if (sscanf(line, "%u %54s %15s %15s", &ipl.timeout,
+        if (sscanf(line, "%lu %54s %15s %15s", &ipl.timeout,
                    ifkey, ipstr, srvstr) < 4) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("virNWFilterSnoopLeaseFileLoad lease file "
-- 
2.37.2

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

Posted by Michal Prívozník 1 year, 8 months ago

On 8/19/22 10:41, 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);
>     }
> 
> This is a safe fix, since time_t is just long int and scales
> automatically with platform (more specifically it's 64bit on all
> platforms we care about).
> 
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
>  src/nwfilter/nwfilter_dhcpsnoop.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
> index a10a14cfc1..4133d4c672 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;
> +    time_t timeout;
>      /* timer list */
>      virNWFilterSnoopIPLease *prev;
>      virNWFilterSnoopIPLease *next;
> @@ -1580,7 +1580,7 @@ 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("%lu %s %s %s\n", ipl->timeout, ifkey, ipstr, dhcpstr);

Doesn't this use of ->timeout need to be typecasted? I mean, we are not
guaranteed that time_t is unsigned long. Elsewhere in the code we even
typecast to long long.

Michal

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

Posted by Daniel P. Berrangé 1 year, 8 months ago

On Tue, Aug 23, 2022 at 12:48:48PM +0200, Michal Prívozník wrote:
> On 8/19/22 10:41, 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);
> >     }
> > 
> > This is a safe fix, since time_t is just long int and scales
> > automatically with platform (more specifically it's 64bit on all
> > platforms we care about).
> > 
> > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > ---
> >  src/nwfilter/nwfilter_dhcpsnoop.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
> > index a10a14cfc1..4133d4c672 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;
> > +    time_t timeout;
> >      /* timer list */
> >      virNWFilterSnoopIPLease *prev;
> >      virNWFilterSnoopIPLease *next;
> > @@ -1580,7 +1580,7 @@ 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("%lu %s %s %s\n", ipl->timeout, ifkey, ipstr, dhcpstr);
> 
> Doesn't this use of ->timeout need to be typecasted? I mean, we are not
> guaranteed that time_t is unsigned long. Elsewhere in the code we even
> typecast to long long.

Yes, this would fail on 32-bit builds I believe. 


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|