schwehr / libais

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

Fix memory leaks in python binding #209

Closed lsix closed 2 years ago

lsix commented 3 years ago

Hi,

We noticed memory leaks in libais that became to be problematic for us. Here is a series of patches that fixes those.

While looking at memory issues, I’ve also changed few manually managed raw pointers (new / delete) and changed them to std::unique_ptr (this is the first commit). I also changed some probably incorrect uses of reinterpred_cast to dynamic_cast (second commit).

I finally got to the crux of the problem which was in ais_py.cpp. This is where the rest of the patch series does changes:

PyDict_SetItem(dict, PyUnicode_FromString("foo"), foo);

where it should have been:

PyObject *foo_key = PyUnicode_FromString("foo");
assert(foo_key);
PyDict_SetItem(dict, foo_key, foo);
Py_DECREF(foo_key);

An equivalent solution us to use PyDict_SetItemString:

PyDict_SetItemString(dict, "foo", foo);

which is equivalent. This is done in patch 3.

If necessary, I can re-arrange patches / drop the 2 first ones if they do not belong in this PR.

schwehr commented 2 years ago

Thanks for the pull request! And sorry for the slow response. I don't track github very well. It's not my primary platform.

If you could break this into separate PRs that just do one thing at a time, that would be much better. I don't look at this code that often and the CI has been broken for a long time.

lsix commented 2 years ago

Hi, thanks for getting back. I’ll prepare separate PRs asap (everything is already split into multiple commits, this should be fairly quick to do).

lsix commented 2 years ago

Hi,

I have opened a set of separate PRs to replace this one (#221 #222 #223 #224 #225 and #226).

Some of the patches in those PRs have been updated in order to remove dependency between one and the other. As a result, there might be conflicts when trying to merge all of those PRs separately. I’ll update the PRs when they become un-mergable because of conflicts.

I close this PR.

schwehr commented 2 years ago

Thanks!!! I will try to get to the PRs in the next couple of days