[Python-Dev] remaining issues from Klocwork static analysis

[Neal Norwitz]

On 7/25/06, "Martin v. Löwis" <martin at v.loewis.de> wrote:
> Neal Norwitz wrote:
> > # 74 Object/funcobject.c:143    Suspicious deref of ptr before NULL check
>
> Not quite sure what it is complaining about, but
>
>         else if (PyTuple_Check(closure)) {
>                 Py_XINCREF(closure);
>         }
>
> looks indeed suspicious: Why do we check for NULL (XINCREF) when
> we know closure can't be NULL (Tuple_Check). Drop the X, and see
> if the warning goes away

Yes, I definitely think dropping the X would make the warning go away.
 Do we want to check for a NULL pointer and raise an exception?  The
docs don't address the issue, so I think if we added a check, ie:  if
(closure && PyTuple_Check(closure)) and got rid of the X that would be
fine as well.

> > #169 Modules/threadmodule.c:497 Memory Leak
>
> Does it say what memory is leaking? Perhaps it complains about
> boot not being released if ident is not -1, however, in that case,
> t_bootstrap will release the memory.

I believe you are right, I never traced through t_bootstrap.   I think
this is a false positive.  There is some memory being leaked on thread
creation as reported by valgrind IIRC.  This doesn't seem to be it
though.

> > #174 Modules/unicodedata.c:432   Array Index Out of Bounds
> >
> >     Buffer overflow, array index of 'decomp_prefix' may be outside the
> >     bounds. Array 'decomp_prefix' of size 18 declared at
> >     unicodedata_db.h:529 may use index values 18..255. Also there is one
> >     similar error on line 433.
>
> This limit is enforced by Tools/unicode/makeunicodedata.py. There are
> only 18 decomposition prefixes at the moment, yet we use 8 bits for
> the decomposition prefix (makeunicodedata checks that prefix < 256)

Just to make sure I understand.  The code in question is accessing
decomp_prefix like this:
    decomp_prefix[decomp_data[index] & 255]

So decomp_prefix will be accessed with the result of:
    decomp_data[index] & 255

The first line of data is (fro unicodedata_db.h) is:

static unsigned int decomp_data[] = {
    0, 257, 32, 514, 32, 776, 259, 97, 514, 32, 772, 259, 50, 259, 51, 514,

If index == 2 (or 3-5, 7-10, etc), we have:
    decomp_prefix[decomp_data[2] & 255]
    decomp_prefix[32 & 255]
    decomp_prefix[32]

which is larger than the max size of decomp_prefix (18).  But from
what I think you stated above, index can't equal those values and the
code that prevents it is calculated a few lines above:

        index = decomp_index1[(code>>DECOMP_SHIFT)];
        index = decomp_index2[(index<<DECOMP_SHIFT)+
                             (code&((1<<DECOMP_SHIFT)-1))];

Is that correct?  If so, would it be correct to add:

    unsigned short prefix_index = decomp_data[index] & 255;
    assert(prefix_index < (sizeof(decomp_prefix)/sizeof(*decomp_prefix)));

n