Re: [PATCH 0/9] Fix issues detected by clang analyzer.
Willy,
Am 24.06.19 um 11:07 schrieb Willy Tarreau:
> On Mon, Jun 24, 2019 at 10:03:29AM +0200, Tim Düsterhus wrote:
>>> Well, some there seem to be real bugs, but others are more a matter of
>>> style than correctness and I disagree with a bogus compiler dictating
>>> what coding style rules we must respect. Because that compiler is bogus.
>>
>> To be clear: This is not a compiler. It's a static analyzer. clang !=
>> clang analyzer.
>
> OK maybe but the point remains, we're relying on a tool complaining about
> *style* and not about code correctness. History has proven multiple times
> that whenever we fall into that trap, we end up with :
> - confusing code which is less and less maintainable over time,
> due to the extra checks that lead to nothing and that become very
> difficult to adjust.
>
> - new bugs due to the extra complexity involved in certain checks
> or ways to write code.
>
> - regressions in stable branch by failure to adapt the mangled code
> to work properly there.
>
> Yes I'm in favor of fixing bogus code or to adapt fragile constructs,
> but not to work around compiler (or analyzer) bugs or stupidities if
> they add wrong assumptions, confusion, or suspicion in the code.
I agree here, that's why I only created patches for a subset of the
reported issues.
>>> Please note that I *do* prefer the sizeof(*foo) for readability and long
>>> term correctness, it's just that we must not claim a bug in the code when
>>> the bug is in the compiler itself (or whatever claims there is a bug).
>>
>> Feel free to re-tag that one as MINOR :-)
>
> In my opinion it's a cleanup : it doesn't change the emitted code at all,
> and makes the code more robust against changes, and saves one round trip
> to the include file for the casual reader trying to figure what caused a
> bug to happen.
Agreed.
>>> Also I refuse to use BUG_ON() to silence the compiler. The sole purpose
>>> of BUG_ON() is to help the *developer*. It should be seen as proven
>>> documentation that helps figuring branches when analysing a bug. While
>>> the assertion in a comment may have become wrong over time, the assertion
>>> in a BUG_ON() is provably true.
>>
>> I agree with any *large* changes to make the analyzer happy. In the two
>> patches I made it was a minimal change to make. The proxy_parse_declare
>> probably could also be changed to:
>>
>>> --- a/src/proxy.c
>>> +++ b/src/proxy.c
>>> @@ -498,7 +498,8 @@ static int proxy_parse_declare(char **args, int
>>> section, struct proxy *curpx,
>>> hdr->index = curpx->nb_req_cap++;
>>> curpx->req_cap = hdr;
>>> }
>>> - if (strcmp(args[2], "response") == 0) {
>>> + else {
>>> + BUG_ON(strcmp(args[2], "response") != 0);
>>> hdr->next = curpx->rsp_cap;
>>> hdr->index = curpx->nb_rsp_cap++;
>>> curpx->rsp_cap = hdr;
>>
>> which might be acceptable?
>
> No because that BUG_ON() above is redundant, just put an "else if (strcmp...)"
> if you want but I really fail to see the problem there and we're not going
> to add confusion in the code to please a shitty compiler or analyzer which
> finds bugs where they are not. What you're doing above is mangling the code
> to *temporarily* shut up the analyzer. It has nothing to do with addressing
> any real issue. Either the analyzer is right and the bug is somewhere else
> and must be fixed, or it's wrong and we'd rather ignore it. Otherwise we
IMO silencing is better than ignoring, because then a the report does
not need to be evaluated again when checking again. Of course I
recognize that silencing potentially comes at a cost to the human and
that one should not make any huge changes, just to please the machine.
For the changes I made I'd argue that they actually might be helpful to
the human as well to make sure that the locations in question do not
diverge. Less so for the `proxy_parse_declare` one, more for
`h2_make_htx_(request|response)` one. I find the latter incredibly
non-obvious, but that might be just my view.
May I suggest this one for the latter then (I believe it captures the
intent better than the current one):
--- a/src/h2.c
+++ b/src/h2.c
@@ -689,7 +689,7 @@ int h2_make_htx_request(struct http_hdr *list,
struct htx *htx, unsigned int *ms
goto fail;
/* Let's dump the request now if not yet emitted. */
- if (!(fields & H2_PHDR_FND_NONE)) {
+ if (!sl) {
sl = h2_prepare_htx_reqline(fields, phdr_val, htx, msgf);
if (!sl)
goto fail;
@@ -913,7 +913,7 @@ int h2_make_htx_response(struct http_hdr *list,
struct htx *htx, unsigned int *m
goto fail;
/* Let's dump the request now if not yet emitted. */
- if (!(fields & H2_PHDR_FND_NONE)) {
+ if (!sl) {
sl = h2_prepare_htx_stsline(fields, phdr_val, htx, msgf);
Re: [PATCH 0/9] Fix issues detected by clang analyzer.
On 6/24/19 10:03 AM, Tim Düsterhus wrote: Willy, Am 24.06.19 um 07:33 schrieb Willy Tarreau: Hi Tim, [snipped] If it emits an error when using sizeof(int) instead of sizeof(*foo) where foo is an pointer to unsigned int, it is bogus. I challenge you to present me a relevant architecture where unsigned changes the allocated size!r For C11 it is guaranteed that there is none according to this: https://stackoverflow.com/a/13169473/782822 This is guaranteed since the beginning of C. Not only for C11. :) Fred.
Re: [PATCH 0/9] Fix issues detected by clang analyzer.
On Mon, Jun 24, 2019 at 10:03:29AM +0200, Tim Düsterhus wrote:
> > Well, some there seem to be real bugs, but others are more a matter of
> > style than correctness and I disagree with a bogus compiler dictating
> > what coding style rules we must respect. Because that compiler is bogus.
>
> To be clear: This is not a compiler. It's a static analyzer. clang !=
> clang analyzer.
OK maybe but the point remains, we're relying on a tool complaining about
*style* and not about code correctness. History has proven multiple times
that whenever we fall into that trap, we end up with :
- confusing code which is less and less maintainable over time,
due to the extra checks that lead to nothing and that become very
difficult to adjust.
- new bugs due to the extra complexity involved in certain checks
or ways to write code.
- regressions in stable branch by failure to adapt the mangled code
to work properly there.
Yes I'm in favor of fixing bogus code or to adapt fragile constructs,
but not to work around compiler (or analyzer) bugs or stupidities if
they add wrong assumptions, confusion, or suspicion in the code.
> There were a *much* larger number of mistakes reported. I picked the
> ones that either were valid (BUG/MINOR) or that were easily silenced
> (MINOR) and I carefully read the code to make sure that they are valid.
OK but one more reason then to drop any other tendencious ones if it
doesn't even achieve a 100% cleaning!
> > If it emits an error when using sizeof(int) instead of sizeof(*foo) where
> > foo is an pointer to unsigned int, it is bogus. I challenge you to present
> > me a relevant architecture where unsigned changes the allocated size!r
>
> For C11 it is guaranteed that there is none according to this:
> https://stackoverflow.com/a/13169473/782822
Not surprised at all :-)
> > Please note that I *do* prefer the sizeof(*foo) for readability and long
> > term correctness, it's just that we must not claim a bug in the code when
> > the bug is in the compiler itself (or whatever claims there is a bug).
>
> Feel free to re-tag that one as MINOR :-)
In my opinion it's a cleanup : it doesn't change the emitted code at all,
and makes the code more robust against changes, and saves one round trip
to the include file for the casual reader trying to figure what caused a
bug to happen.
> > Also I refuse to use BUG_ON() to silence the compiler. The sole purpose
> > of BUG_ON() is to help the *developer*. It should be seen as proven
> > documentation that helps figuring branches when analysing a bug. While
> > the assertion in a comment may have become wrong over time, the assertion
> > in a BUG_ON() is provably true.
>
> I agree with any *large* changes to make the analyzer happy. In the two
> patches I made it was a minimal change to make. The proxy_parse_declare
> probably could also be changed to:
>
> > --- a/src/proxy.c
> > +++ b/src/proxy.c
> > @@ -498,7 +498,8 @@ static int proxy_parse_declare(char **args, int
> > section, struct proxy *curpx,
> > hdr->index = curpx->nb_req_cap++;
> > curpx->req_cap = hdr;
> > }
> > - if (strcmp(args[2], "response") == 0) {
> > + else {
> > + BUG_ON(strcmp(args[2], "response") != 0);
> > hdr->next = curpx->rsp_cap;
> > hdr->index = curpx->nb_rsp_cap++;
> > curpx->rsp_cap = hdr;
>
> which might be acceptable?
No because that BUG_ON() above is redundant, just put an "else if (strcmp...)"
if you want but I really fail to see the problem there and we're not going
to add confusion in the code to please a shitty compiler or analyzer which
finds bugs where they are not. What you're doing above is mangling the code
to *temporarily* shut up the analyzer. It has nothing to do with addressing
any real issue. Either the analyzer is right and the bug is somewhere else
and must be fixed, or it's wrong and we'd rather ignore it. Otherwise we
can as well use this awesome analyzer and try to fix its reports :
$ for i in src/*.c; do for j in $(seq 1 $((RANDOM%5+1))); do echo
"$i:$((RANDOM%2000+1)): variable is used uninitialized in this
function";done;done
Don't get me wrong, I'm not dismissing everything such analysers can find,
I'm saying that these tools should be used as a help an not as a goal. Each
report needs to be carefully evaluated. In the strcmp() example above,
nothing in your patch addresses a real issue since the code is just a
repetition of something done 5 lines above. If the analyzer is wrong there,
be it!
> > For the crash that clang doesn't detect that the code will stop there,
>
> Just to make sure: clang *analyzer*. It's nothing an off-the-shelf clang
> will report.
One more reason for not sticking too hard to such wrong reports then!
Thanks,
Willy
Re: [PATCH 0/9] Fix issues detected by clang analyzer.
Willy,
Am 24.06.19 um 07:33 schrieb Willy Tarreau:
> Hi Tim,
>
> On Sun, Jun 23, 2019 at 10:10:08PM +0200, Tim Duesterhus wrote:
>> Willy,
>>
>> these patches are the result of running
>>
>> scan-build make -j4 all TARGET=linux-glibc DEBUG=-DDEBUG_STRICT=1
>>
>> While it spits out a bunch of false negatives that are quite convoluted these
>> patches either:
>>
>> 1. Make it easier for clang analyzer to understand the code by making a few
>> *small*
>>adjustments. The one in `proxy_parse_declare` might be questionable,
>> though.
>> 2. Actually fix an issue I could reproduce with a carefully crafted example
>>configuration.
>
> Well, some there seem to be real bugs, but others are more a matter of
> style than correctness and I disagree with a bogus compiler dictating
> what coding style rules we must respect. Because that compiler is bogus.
To be clear: This is not a compiler. It's a static analyzer. clang !=
clang analyzer.
There were a *much* larger number of mistakes reported. I picked the
ones that either were valid (BUG/MINOR) or that were easily silenced
(MINOR) and I carefully read the code to make sure that they are valid.
> If it emits an error when using sizeof(int) instead of sizeof(*foo) where
> foo is an pointer to unsigned int, it is bogus. I challenge you to present
> me a relevant architecture where unsigned changes the allocated size!r
For C11 it is guaranteed that there is none according to this:
https://stackoverflow.com/a/13169473/782822
> Please note that I *do* prefer the sizeof(*foo) for readability and long
> term correctness, it's just that we must not claim a bug in the code when
> the bug is in the compiler itself (or whatever claims there is a bug).
Feel free to re-tag that one as MINOR :-)
> Also I refuse to use BUG_ON() to silence the compiler. The sole purpose
> of BUG_ON() is to help the *developer*. It should be seen as proven
> documentation that helps figuring branches when analysing a bug. While
> the assertion in a comment may have become wrong over time, the assertion
> in a BUG_ON() is provably true.
I agree with any *large* changes to make the analyzer happy. In the two
patches I made it was a minimal change to make. The proxy_parse_declare
probably could also be changed to:
> --- a/src/proxy.c
> +++ b/src/proxy.c
> @@ -498,7 +498,8 @@ static int proxy_parse_declare(char **args, int section,
> struct proxy *curpx,
> hdr->index = curpx->nb_req_cap++;
> curpx->req_cap = hdr;
> }
> - if (strcmp(args[2], "response") == 0) {
> + else {
> + BUG_ON(strcmp(args[2], "response") != 0);
> hdr->next = curpx->rsp_cap;
> hdr->index = curpx->nb_rsp_cap++;
> curpx->rsp_cap = hdr;
which might be acceptable?
> For the crash that clang doesn't detect that the code will stop there,
Just to make sure: clang *analyzer*. It's nothing an off-the-shelf clang
will report.
> I understand the problem but I think that fixing it with an ifdef to
> detect this specific analyzer isn't the right way to do it because
> we'll get it in the face again soon for any other code analyzer. I
> think we should move this to an inline function marked with
> __attribute__((noreturn)) instead.
>
> I'll have a deeper look, because I think we need some updated tools. If
> compilers get smarter at purposely annoying us, we'll need to more
> ostensibly shut them up when relevant by not telling them what we're
> doing since they appear to have difficulties reading the code they were
> made to compile in the first place...
>
> I'm having a few ideas that could actually all derive from the
> ALREADY_CHECKED() hack. For example we could use something like this
> to block funny checks and disable certain absurd warnings that made
> us introduce bugs in the past :
>
> #define FORCE(x) ({ typeof(x) __x__ = (x); asm("" : "=rm"(__x__) :
> "0"(__x__)); __x__; })
>
> We could also use __builtin_trap() when defined, or __builtin_unreachable()
> after an asm statement, optionally replaced by a loop when not defined.
>
> I may propose you a few things to try later today.
Best regards
Tim Düsterhus
Re: [PATCH 0/9] Fix issues detected by clang analyzer.
Hi Tim,
On Sun, Jun 23, 2019 at 10:10:08PM +0200, Tim Duesterhus wrote:
> Willy,
>
> these patches are the result of running
>
> scan-build make -j4 all TARGET=linux-glibc DEBUG=-DDEBUG_STRICT=1
>
> While it spits out a bunch of false negatives that are quite convoluted these
> patches either:
>
> 1. Make it easier for clang analyzer to understand the code by making a few
> *small*
>adjustments. The one in `proxy_parse_declare` might be questionable,
> though.
> 2. Actually fix an issue I could reproduce with a carefully crafted example
>configuration.
Well, some there seem to be real bugs, but others are more a matter of
style than correctness and I disagree with a bogus compiler dictating
what coding style rules we must respect. Because that compiler is bogus.
If it emits an error when using sizeof(int) instead of sizeof(*foo) where
foo is an pointer to unsigned int, it is bogus. I challenge you to present
me a relevant architecture where unsigned changes the allocated size!r
Please note that I *do* prefer the sizeof(*foo) for readability and long
term correctness, it's just that we must not claim a bug in the code when
the bug is in the compiler itself (or whatever claims there is a bug).
Also I refuse to use BUG_ON() to silence the compiler. The sole purpose
of BUG_ON() is to help the *developer*. It should be seen as proven
documentation that helps figuring branches when analysing a bug. While
the assertion in a comment may have become wrong over time, the assertion
in a BUG_ON() is provably true.
For the crash that clang doesn't detect that the code will stop there,
I understand the problem but I think that fixing it with an ifdef to
detect this specific analyzer isn't the right way to do it because
we'll get it in the face again soon for any other code analyzer. I
think we should move this to an inline function marked with
__attribute__((noreturn)) instead.
I'll have a deeper look, because I think we need some updated tools. If
compilers get smarter at purposely annoying us, we'll need to more
ostensibly shut them up when relevant by not telling them what we're
doing since they appear to have difficulties reading the code they were
made to compile in the first place...
I'm having a few ideas that could actually all derive from the
ALREADY_CHECKED() hack. For example we could use something like this
to block funny checks and disable certain absurd warnings that made
us introduce bugs in the past :
#define FORCE(x) ({ typeof(x) __x__ = (x); asm("" : "=rm"(__x__) : "0"(__x__));
__x__; })
We could also use __builtin_trap() when defined, or __builtin_unreachable()
after an asm statement, optionally replaced by a loop when not defined.
I may propose you a few things to try later today.
Cheers,
Willy
[PATCH 0/9] Fix issues detected by clang analyzer.
Willy, these patches are the result of running scan-build make -j4 all TARGET=linux-glibc DEBUG=-DDEBUG_STRICT=1 While it spits out a bunch of false negatives that are quite convoluted these patches either: 1. Make it easier for clang analyzer to understand the code by making a few *small* adjustments. The one in `proxy_parse_declare` might be questionable, though. 2. Actually fix an issue I could reproduce with a carefully crafted example configuration. Cc'd William on patch 4 (worker) and Christopher on 5 (spoe). Tim Duesterhus (9): BUG/MINOR: cfgparse: Pass correct type to `calloc` BUG/MINOR: log: Detect missing sampling ranges in config BUG/MINOR: cfgparse: Stop passing NULL to memcpy BUG/MINOR: mworker: Fix segmentation fault during cfgparse BUG/MINOR: spoe: Fix memory leak if failing to allocate memory BUG/MINOR: debug: Silence warning about ignored return value MINOR: debug: Make ABORT_NOW() emit abort() for clang analyzer MINOR: proxy: Restructure code to assert that `proxy_parse_declare` does not leak MINOR: h2: Assert that a status line exists in h2_make_htx_(request|response) include/common/debug.h | 6 +- src/cfgparse.c | 5 +++-- src/flt_spoe.c | 1 + src/h2.c | 8 src/log.c | 5 + src/mworker-prog.c | 30 -- src/proxy.c| 6 +- 7 files changed, 43 insertions(+), 18 deletions(-) -- 2.21.0
