Ticket #17 (closed defect: fixed)

Opened 2 years ago

Last modified 2 years ago

Various errors discovered using static analysis tool

Reported by: dmalcolm Owned by: twaugh
Priority: major Component: cups module
Version: 1.9.60 Keywords:
Cc: Blocked By:
Blocking:

Description

I attempted to compile pycups (as seen in the Fedora packages) using a static analysis tool I've written for CPython extension code:

http://gcc-python-plugin.readthedocs.org/en/latest/cpychecker.html

The signal:noise ratio of messages from the tool isn't yet as good as it could be (e.g. it tracebacks when analyzing some functions), but I believe I found some genuine bugs.

Specifically, there are quite a few places in the code where you do:

return Py_None;

which is usually wrong (as it doesn't increment the refcount on that return value); typically you want to use this macro:

Py_RETURN_NONE;

I will attach reports from the tool.

There are quite a few places where calls into Py_* functions could return NULL under low memory conditions, but the code assumes that the results are non-NULL (which will segfault). Are you aiming to make the code robust in this situation? (there are too many reports to sanely attach, alas)

Hope this is helpful

Attachments

cupsmodule.c.cups_require-refcount-errors.html (16.2 KB) - added by dmalcolm 2 years ago.
HTML report showing one example of an erroneous "return Py_None;"
cupsipp.c.cupsipp_iocb_read-refcount-errors.html (24.3 KB) - added by dmalcolm 2 years ago.
HTML report showing an execution path through cupsipp_iocb_read() that leaks a reference on the "result" object
cupsppd.c.PPD_emitString-refcount-errors.html (18.9 KB) - added by dmalcolm 2 years ago.
HTML report showing an execution path through PPD_emitString() which leaks a reference to "PyObject?* ret" (you already own a ref on it due to call to PyString_FromString, you don't have to INCREF it again)

Change History

Changed 2 years ago by dmalcolm

HTML report showing one example of an erroneous "return Py_None;"

Changed 2 years ago by dmalcolm

HTML report showing an execution path through cupsipp_iocb_read() that leaks a reference on the "result" object

Changed 2 years ago by dmalcolm

HTML report showing an execution path through PPD_emitString() which leaks a reference to "PyObject?* ret" (you already own a ref on it due to call to PyString_FromString, you don't have to INCREF it again)

comment:1 follow-up: ↓ 3 Changed 2 years ago by dmalcolm

Also emitted from the tool:

cupsipp.c: In function ‘IPPRequest_init’:
cupsipp.c:287:25: error: Mismatching type in call to PyArg_ParseTuple with format code "|i" [-fpermissive]
  argument 3 ("&op") had type
    "long unsigned int *" (pointing to 64 bits)
  but was expecting
    "int *" (pointing to 32 bits)
  for format code "i"

I suspect that the above will go badly wrong on 64-bit big-endian systems (e.g. ppc64 and s390x)

comment:2 Changed 2 years ago by dmalcolm

FWIW, this was done with pycups-1.9.60 as per Fedora's src.rpm packages in git as of today (aa20dc30d13903a15ea17f97330f1131dd30a906), using the latest version of the gcc-python-plugin from upstream (post 0.7: b2f11c1a312170a872dd09fd516a01446e0df7a0)

comment:3 in reply to: ↑ 1 Changed 2 years ago by twaugh

This looks like a great tool! I've applied some fixes upstream now.

Replying to dmalcolm:

There are quite a few places where calls into Py_* functions could return NULL

under low memory conditions, but the code assumes that the results are non-NULL

(which will segfault). Are you aiming to make the code robust in this situation? (there are too many reports to sanely attach, alas)

Ideally, yes, but I can see there is no handling for that at all at the moment.

Replying to dmalcolm:

Also emitted from the tool:

cupsipp.c: In function ‘IPPRequest_init’:
cupsipp.c:287:25: error: Mismatching type in call to PyArg_ParseTuple with format code "|i" [-fpermissive]
  argument 3 ("&op") had type
    "long unsigned int *" (pointing to 64 bits)
  but was expecting
    "int *" (pointing to 32 bits)
  for format code "i"

I suspect that the above will go badly wrong on 64-bit big-endian systems (e.g. ppc64 and s390x)

Should it be "l"? Here's what I have now:

{
  long op = -1;
  if (!PyArg_ParseTuple (args, "|l", &op))
    return -1;

  if (op < 0)
    self->ipp = ippNew ();
  else
    self->ipp = ippNewRequest ((unsigned long) op);

  return 0;
}

comment:4 Changed 2 years ago by twaugh

  • Resolution set to fixed
  • Status changed from new to closed
  • Version changed from 1.9.54 to 1.9.60

Not sure about that last one so I've left it unchanged for the moment.

Note: See TracTickets for help on using tickets.