Re: bash-4.3 bug report

[Eric Blake]

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

signature.asc
Description: OpenPGP digital signature