[libvirt PATCH v2 0/2] nwfilter: Fix timeout data type reported by coverity
Since v1: - instead of time_t, store the timeout as unsigned long long and typecast any affected time_t variables on relevant places here's a successfull CI build: https://gitlab.com/eskultety/libvirt/-/pipelines/620752689 Erik Skultety (2): nwfilter: Replace a redundant 'now' variable with a direct time(0) call nwfilter: Fix timeout data type reported by coverity src/nwfilter/nwfilter_dhcpsnoop.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) -- 2.37.2
In this very instance having a variable is pointless since there is only a single time(0) call in the whole function. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/nwfilter/nwfilter_dhcpsnoop.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index a10a14cfc1..18812c0b20 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -1723,13 +1723,11 @@ virNWFilterSnoopLeaseFileLoad(void) char ipstr[INET_ADDRSTRLEN], srvstr[INET_ADDRSTRLEN]; virNWFilterSnoopIPLease ipl; virNWFilterSnoopReq *req; - time_t now; FILE *fp; int ln = 0, tmp; VIR_LOCK_GUARD lock = virLockGuardLock(&virNWFilterSnoopState.snoopLock); fp = fopen(LEASEFILE, "r"); - time(&now); while (fp && fgets(line, sizeof(line), fp)) { if (line[strlen(line)-1] != '\n') { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1746,7 +1744,7 @@ virNWFilterSnoopLeaseFileLoad(void) "line %d corrupt"), ln); break; } - if (ipl.timeout && ipl.timeout < now) + if (ipl.timeout && ipl.timeout < time(0)) continue; req = virNWFilterSnoopReqGetByIFKey(ifkey); if (!req) { -- 2.37.2
On Tue, Aug 23, 2022 at 18:22:36 +0200, Erik Skultety wrote:
In this very instance having a variable is pointless since there is only a single time(0) call in the whole function.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/nwfilter/nwfilter_dhcpsnoop.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index a10a14cfc1..18812c0b20 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c
[...]
@@ -1746,7 +1744,7 @@ virNWFilterSnoopLeaseFileLoad(void) "line %d corrupt"), ln); break; } - if (ipl.timeout && ipl.timeout < now) + if (ipl.timeout && ipl.timeout < time(0))
'time()' takes a pointer, so the argument should be 'NULL' instead.
continue; req = virNWFilterSnoopReqGetByIFKey(ifkey); if (!req) { -- 2.37.2
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@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; /* 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); 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 " -- 2.37.2
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@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?
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@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
On Wed, Aug 24, 2022 at 09:00:00 +0200, Erik Skultety wrote:
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@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).
Yes that's a bit weird, but that is also the reason I've suggested to use a temporary variable for sscanf which can be unsigned long long to have working formatters, but at the same time preserve 'time_t' where possible.
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.
Well I'm not sure it will help though. The man page for time() has the following disclaimer for linux: On Linux, a call to time() with tloc specified as NULL cannot fail with the error EOVERFLOW, even on ABIs where time_t is a signed 32-bit integer and the clock reaches or exceeds 2**31 seconds (2038-01-19 03:14:08 UTC, ignoring leap seconds). (POSIX.1 permits, but does not require, the EOVERFLOW error in the case where the seconds since the Epoch will not fit in time_t.) Instead, the behavior on Linux is undefined when the system time is out of the time_t range. Applications intended to run after 2038 should use ABIs with time_t wider than 32 bits. So once the sands of 32 bit time run out such boxes are doomed regardless whether we use a 64 bit variable or time_t. Anyways, once you switch to the correct way to pass a NULL pointer as argument in both patches you can add: Reviewed-by: Peter Krempa <pkrempa@redhat.com> I'd probably still prefer though to keep time_t where we are dealing with time and restrict the conversion only to sscanf/printf.
participants (2)
-
Erik Skultety -
Peter Krempa