[Python-Dev] remaining issues from Klocwork static analysis (original) (raw)

Neal Norwitz nnorwitz at gmail.com
Wed Jul 26 06:47:08 CEST 2006

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 (PyTupleCheck(closure)) { PyXINCREF(closure); } looks indeed suspicious: Why do we check for NULL (XINCREF) when we know closure can't be NULL (TupleCheck). 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, tbootstrap 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 'decompprefix' may be outside the > bounds. Array 'decompprefix' of size 18 declared at > unicodedatadb.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

More information about the Python-Dev mailing list