schwehr / libais

C++ decoder for Automatic Identification System for tracking ships and decoding maritime information
Other
215 stars 94 forks source link

ais_py.cpp: Prefer PyDict_SetItemString over PyDict_SetItem #222

Closed lsix closed 2 years ago

lsix commented 2 years ago

All insertions into python dictionaries are done using PyDict_SetItem. This function requires a PyObject* as the KEY (second) argument.

In few places a temporary PyObject* is created when inserting in the dict but is not cleaned (i.e Py_DECREF'ed). This is done like so:

   PyDict_SetItem(dict, PyUnicode_FromString("cargos"), cargo_list);

This allocates a PyObject* from the "cargos" string, and gives it directly to PyDict_SetItem. The problem is the following:

When the containing dict is eventually deleted, its reference to the key object is decremented, leaving its refcount to 1. This leads to a leak of the key object.

The correct usage would have been to use:

   PyObject *key = PyUnicode_FromString("cargos");
   assert(key);
   PyDict_SetItem(dict, key, cargo_list);
   Py_DECREF(key);

This use case is quite common, leading cpython to provide a helper function that does just that: PyDict_SetItemString. This function takes a const char * argument as KEY argument and handles the refcounting properly. The previous example then becomes:

   PyDict_SetItemString(dict, "cargos", cargo_list);

This patch proposes to use PyDict_SetItemString instead of PyDict_SetItem wherever possible.

schwehr commented 2 years ago

You'll need to merge with head for this and your other PRs. I've also started cleanup with using std::string for all strings.

lsix commented 2 years ago

Hi @schwehr,

I have rebased this branch onto master.

I have also rephrased the commit message (and PR comment). When re reading the original one, I did not find it very clear. I hope it now is easier to understand.