Re: byteswap side-effect defect report from Coverity
On Mon, May 20, 2024 at 5:43 PM Bruno Haible wrote: > Jeffrey Walton wrote: > > I think this is a valid finding. It will operate one way in release > builds > > (-DNDEBUG), and another way in debug builds (no NDEBUG). > > No. Like Coverity, you are assuming that 'ASSERT' works like 'assert'. > But it does not. The raison d'être of ASSERT is to work independently > of NDEBUG (and to produce a better diagnostic). > See tests/macros.h line 67. > That macro has been traditionally called VERIFY. It is effectively an assert without the gyrations around NDEBUG. Jeff
Re: byteswap side-effect defect report from Coverity
Jeffrey Walton wrote: > I think this is a valid finding. It will operate one way in release builds > (-DNDEBUG), and another way in debug builds (no NDEBUG). No. Like Coverity, you are assuming that 'ASSERT' works like 'assert'. But it does not. The raison d'être of ASSERT is to work independently of NDEBUG (and to produce a better diagnostic). See tests/macros.h line 67. Bruno
Re: byteswap side-effect defect report from Coverity
On 5/20/24 14:29, Jeffrey Walton wrote: I think this is a valid finding. It will operate one way in release builds (-DNDEBUG), and another way in debug builds (no NDEBUG). When NDEBUG is in effect, the code `bswap_16 (value_1++) == 0` will be removed. How so? The test program uses 'ASSERT' not 'assert', so 'NDEBUG' should be irrelevant. Can you reproduce the problem with: ./gnulib-tool --test byteswap ?
Re: byteswap side-effect defect report from Coverity
On Mon, May 20, 2024 at 12:13 PM Paul Eggert wrote:
> Bruno and I get defect reports from Coverity Scan for Gnulib. The most
> recent one has this new complaint:
>
> > *** CID 1588680: Incorrect expression (ASSERT_SIDE_EFFECT)
> > /gltests/test-byteswap.c: 43 in test_bswap_eval_once()
> > 37
> > 38 /* Test that the bswap functions evaluate their arguments once.
> */
> > 39 static void
> > 40 test_bswap_eval_once (void)
> > 41 {
> > 42 uint_least16_t value_1 = 0;
> >>> CID 1588680: Incorrect expression (ASSERT_SIDE_EFFECT)
> >>> Argument "value_1++" of ASSERT() has a side effect. The
> containing function might work differently in a non-debug build.
> > 43 ASSERT (bswap_16 (value_1++) == 0);
>
> I checked the glibc sources, and long ago glibc indeed had a bug in this
> area: bswap_16 and related function-like macros evaluated their
> arguments more than once. This bug was fixed in the following glibc
> commit dated Sat Aug 8 20:02:34 1998 +, and the fix appears in glibc
> 2.1 (1999).
>
>
> https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=7ce241a03e2c0b49482d9d05c8ddb765e89a01d9
>
> Gnulib does not support glibc 2.1.x and older, so this should not be a
> problem when porting to glibc. However, I worried that other platforms
> might have the bug, until I noticed that m4/byteswap.m4 already
> inadvertently tests for it. I installed the attached patch to document
> the test.
>
> We can ignore this Coverity defect report as a false positive, just as
> we ignore many other defect reports.
I think this is a valid finding. It will operate one way in release builds
(-DNDEBUG), and another way in debug builds (no NDEBUG).
When NDEBUG is in effect, the code `bswap_16 (value_1++) == 0` will be
removed.
Maybe the test should be rewritten to something like:
uint16_t x = bswap_16 (value_1++);
ASSERT(x == 0);
Jeff
Re: byteswap side-effect defect report from Coverity
Hi Bruno, On 5/20/24 12:40 PM, Bruno Haible wrote: >> Interesting. I just learned what a Coverity scan is. Do I have to have >> permission to view the defects? > > I think one needs permission to view and classify these defects, yes. > But it's more boring than anything else, since more than 90% are false > alarms. So, if you don't mind, it's sufficient if Paul and I view and > classify these defects. I see. That is fine with me. I can see that a lot of them are "CWE-676: Use of Potentially Dangerous Function", which seems more annoying then helpful. I imagine it is just a bunch of functions that are mostly fine. > If you really want to do something boring, you could review > 'gcc -fanalyzer' reports (which is something Paul and I occasionally > do as well) or 'clang -fanalyzer' reports (which neither of us has done > so far, AFAIK). I use 'gcc -fanalyzer' occasionally. I wasn't aware that clang supported it too. Collin
Re: byteswap side-effect defect report from Coverity
Hi Collin, > Interesting. I just learned what a Coverity scan is. Do I have to have > permission to view the defects? I think one needs permission to view and classify these defects, yes. But it's more boring than anything else, since more than 90% are false alarms. So, if you don't mind, it's sufficient if Paul and I view and classify these defects. If you really want to do something boring, you could review 'gcc -fanalyzer' reports (which is something Paul and I occasionally do as well) or 'clang -fanalyzer' reports (which neither of us has done so far, AFAIK). Bruno
Re: byteswap side-effect defect report from Coverity
On 5/20/24 12:15, Collin Funk wrote: I just learned what a Coverity scan is. Do I have to have permission to view the defects? I think anybody can sign up, here: https://scan.coverity.com/projects/gnu-gnulib
Re: byteswap side-effect defect report from Coverity
Hi Paul, On 5/20/24 9:13 AM, Paul Eggert wrote: > Gnulib does not support glibc 2.1.x and older, so this should not be a > problem when porting to glibc. However, I worried that other platforms might > have the bug, until I noticed that m4/byteswap.m4 already inadvertently tests > for it. I installed the attached patch to document the test. I was worried about other systems too. After you mentioned that macros don't work on double arguments correctly I used it in byteswap.m4 since it seemed like a nice compile-time check. I probably should have commented it though, thanks. > We can ignore this Coverity defect report as a false positive, just as we > ignore many other defect reports. Interesting. I just learned what a Coverity scan is. Do I have to have permission to view the defects? Collin
byteswap side-effect defect report from Coverity
Bruno and I get defect reports from Coverity Scan for Gnulib. The most
recent one has this new complaint:
*** CID 1588680: Incorrect expression (ASSERT_SIDE_EFFECT)
/gltests/test-byteswap.c: 43 in test_bswap_eval_once()
37
38 /* Test that the bswap functions evaluate their arguments once. */
39 static void
40 test_bswap_eval_once (void)
41 {
42 uint_least16_t value_1 = 0;
>>> CID 1588680: Incorrect expression (ASSERT_SIDE_EFFECT)
>>> Argument "value_1++" of ASSERT() has a side effect. The containing
function might work differently in a non-debug build.
43 ASSERT (bswap_16 (value_1++) == 0);
I checked the glibc sources, and long ago glibc indeed had a bug in this
area: bswap_16 and related function-like macros evaluated their
arguments more than once. This bug was fixed in the following glibc
commit dated Sat Aug 8 20:02:34 1998 +, and the fix appears in glibc
2.1 (1999).
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=7ce241a03e2c0b49482d9d05c8ddb765e89a01d9
Gnulib does not support glibc 2.1.x and older, so this should not be a
problem when porting to glibc. However, I worried that other platforms
might have the bug, until I noticed that m4/byteswap.m4 already
inadvertently tests for it. I installed the attached patch to document
the test.
We can ignore this Coverity defect report as a false positive, just as
we ignore many other defect reports.From 6dbbada2b05bdc5efd0a5592a7805c92fe0531c6 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Mon, 20 May 2024 09:08:27 -0700
Subject: [PATCH] * m4/byteswap.m4: Add comment about broken C libraries.
---
m4/byteswap.m4 | 4
1 file changed, 4 insertions(+)
diff --git a/m4/byteswap.m4 b/m4/byteswap.m4
index 3f5ef45cfe..3185bab763 100644
--- a/m4/byteswap.m4
+++ b/m4/byteswap.m4
@@ -15,6 +15,10 @@ AC_DEFUN([gl_BYTESWAP],
AC_CACHE_CHECK([for working bswap_16, bswap_32, bswap_64],
[gl_cv_header_working_byteswap_h],
[gl_cv_header_working_byteswap_h=no
+ dnl Check that floating point arguments work.
+ dnl This also checks C libraries with implementations like
+ dnl '#define bswap_16(x) (((x) >> 8 & 0xff) | (((x) & 0xff) << 8))'
+ dnl that mistakenly evaluate their arguments multiple times.
AC_COMPILE_IFELSE(
[AC_LANG_PROGRAM(
[[#include
--
2.40.1
