[Python-Dev] remaining issues from Klocwork static analysis

["Martin v. Löwis"]

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

> #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.

> # 28 Modules/_sre.c:987   Array Index Out of Bounds
> 
>     Buffer overflow, array index of 'mark' may be outside the
>     bounds. Array 'mark' of size 200 declared at sre.h:77 may use
>     index values 0..536870911. Also there are 3 similar errors on
>     lines 1006, 1225, 1237.  (Try limiting mark on line 589?)

ISTM that SRE has a limit of 100 MARK opcodes, meaning a maximum
of 100 groups per expression (so you need 200 mark pointers).
This can't overrun as sre_compile refuses to compile expressions
with more groups.

Of course, a malicious application could craft the opcodes itself
(bypassing sre_compile), in which case you could get a buffer
overrun.

The right solution is to have a dynamic marks array.

> #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)

Looking at the code, I now wonder why decomp_data can't be
"unsigned short", instead of "unsigned int" (the upper byte is the
decomposition length, and it can't be larger than 256, either).

> # 36 Modules/cPickle.c:3404   Memory Leak
> 
>     Memory leak. Dynamic memory stored in 's' allocated through
>     function 'pystrndup' at line 3384 is lost at line 3404.
> 
>     s should not be freed on line 3407, but earlier.
>     PDATA_PUSH can return on error and s will not be freed.

Correct. We should not use macros with embedded return statements.

> # 61 Modules/_sqlite/cursor.c:599  Null pointer may be dereferenced
> 
>     Null pointer 'self->statement' that comes from line 674 may be
>     dereferenced by passing argument 1 to function
>     'statement_mark_dirty' at line 599.

Looks like a problem. Maybe a break is missing after line 674?

Regards,
Martin