[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: bash-4.3 bug report

From: Eric Blake
Subject: Re: bash-4.3 bug report
Date: Mon, 14 Apr 2014 10:40:21 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0

On 04/14/2014 10:22 AM, David Binderman wrote:
> Hello there,
> ----------------------------------------
>> But my point remains to the original poster: a patch
>> without justification is unlikely to be applied. Document WHY you think
>> the existing code is a bug, not just HOW to fix it, for your patch to be
>> usefully considered.
> Standard software engineering practice is to look before leaping.
> This means always check the array index before use.
> The static analyser implements that standard practice.

Therefore, the static analyzer (_which_ static analyzer? you didn't
state that) has exposed a false positive, and the WHY for the patch is
to silence the false positive of the static analyzer.  But have you also
filed a bug against the analyzer about its false positive?

> The code in question, independent of whether it works ok or not,
> does it's work in a non-standard way when the standard way
> is easy to achieve and has some possible benefits for robustness,
> as well as being easier on the eye to the experienced code reviewer.

Indeed, reading JUST the code present in this thread, I did not realize
that invokers[] was 1. always NULL-terminated, and 2. possibly longer
than 5 elements; I also did not see that this loop wants to stop at the
first instance of either end-of-list or capped at 5 (stopping early on
an array is indeed an unusual construct).  Reading the code in full
context, I now see that, which is why I conclude that the static
analyzer reported a false positive.  Had you provided your patch with
context lines, rather than just the one line being modified, it might
have also saved some reviewer time.

> Anyone experienced looking at the code will always need to examine it
> more closely to find out why it's a good idea in this case to use an array
> index and *then* sanity check it's value.

So use that as the justification for the patch, rather than expecting us
to figure it out your intentions on our own.

Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]