Memory leak and incorrect behaviour of generated bindings

[Matthijs]

Happening on all release of sip from 4.16.8 until 4.19.20, so Xenial and newer are all affected.

I have tested this on both Xenial(sip 4.17) and Bionic(4.19.7).

My request is to update all affected releases to 4.19.21.

[Impact]
Since the release of sip 4.16.8, but especially commit https://www.riverbankcomputing.com/hg/sip/rev/11a92ebd4840, sip contains two bugs. The code being generated doesn't keep the correct references and a memory leak.

The reference problem:
(generateVariableGetter in sipgen/gencode.c)
In the get function of a class member, there isn't a reference kept to the containing class. Therefore if no other references are kept to the containing class, the containing class in cpp is deleted, also if there are still references to the member. This causes also the member in cpp to be deleted, while in python there is still a reference to the member. Which can't be accessed anymore.
Fixed in:
https://www.riverbankcomputing.com/hg/sip/rev/137b9be794a1
https://www.riverbankcomputing.com/hg/sip/rev/6a057b2d8537
https://www.riverbankcomputing.com/hg/sip/rev/699facc95914

The memory leak problem:
(sip_api_get_reference in siplib/siplib.c)
A new reference to "key_obj" is obtained from PyLong_FromLong/PyInt_FromLong. but the reference count is not decreased. Therefore "key_obj" stays alive and produces a memory leak.
Fixed in:
https://www.riverbankcomputing.com/hg/sip/rev/82ec38dc0e63

Both problems are fixed in 4.19.21. Also the entire changelog between 4.19.20 and 4.19.21 is related to fixing only these two problems.

The converstation between me and the develop of sip can be read here: http://python.6.x6.nabble.com/sip-Accessing-members-results-in-incorrect-missing-data-td5266521.html, which might provide some more insight.

[Test Case]
As sip is a library to generate python bindings. So my test case will be an "applied" test case. My test case is focused on the 'python_orocos_kdl', https://github.com/orocos/orocos_kinematics_dynamics, library.

For this test case, please use the "test/sip_bug" branch of my fork of the library, https://github.com/MatthijsBurgh/orocos_kinematics_dynamics.

To test the reference problem, follow the steps from travis config. (Of course skipping the installation of python-sip-dev if testing with other sip versions.)

To test the memory leak, run the following code (After running the steps from the travis config):
```
import PyKDL as kdl

F = kdl.Frame(
    kdl.Rotation.Quaternion(0, 1, 0, 0),
    kdl.Vector(1, 2, 3))

while True:
    v = F.p
```
See http://python.6.x6.nabble.com/sip-Accessing-members-results-in-incorrect-missing-data-tp5266521p5267154.html including plot of the memory usage, http://python.6.x6.nabble.com/file/t383425/mem_leak.png.

[Regression Potential]
Different versions of SIP have been released on multiple distributions. No bugs have been reported of these versions. I have tested multiple versions of SIP in the affected range on both Xenial and Bionic. I didn't experience any bugs other than the two reported here.

[Other Info]
Already mentioned, but the conversation between me and the developer, http://python.6.x6.nabble.com/sip-Accessing-members-results-in-incorrect-missing-data-td5266521.html.

[Dmitry Shachnev (mitya57)]

Hi Matthijs!

For 20.04 (Focal), this is now fixed, so I am changing the bug status accordingly.

For 18.04 (Bionic), I prepared a test package in https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/3941. Can you please test if it works fine for you?

For 16.04 (Xenial), the patches do not apply cleanly. If you managed to backport the fixes, can you attach your backported patch?

[Matthijs (matthijsburgh)]

Hi Dmitry,

Please check the attached patch compared to the 4.17 tag. This should provide the solution for 16.04. The diff is in the original mercurial sip repo, https://www.riverbankcomputing.com/hg/sip. But is should be applicable to the git repo.

[Matthijs (matthijsburgh)]

Hi Dmitry,

Also the provided test package for 18.04 does fix the problems for me.

[Matthijs (matthijsburgh)]

Hi Dmitry,

Please check the attached patch compared to the 4.17 tag. This should provide the solution for 16.04. The diff is in the original mercurial sip repo, https://www.riverbankcomputing.com/hg/sip. But is should be applicable to the git repo.

The previous patch missed 2 lines. Therefore the update

[Dmitry Shachnev (mitya57)]

Thanks!

I have now uploaded the packages to Ubuntu, they are waiting for review in unapproved queues:

- https://launchpad.net/ubuntu/xenial/+queue?queue_state=1
- https://launchpad.net/ubuntu/bionic/+queue?queue_state=1

When they are accepted into -proposed, you will be asked to test the packages from there. Testing is needed before the packages will be accepted into official -updates.

I have also uploaded the 16.04 (Xenial) package into the PPA mentioned above, in case you want to test it earlier.

[Robie Basak (racb)]

Thank you for working on this.

In the proposed upload for Xenial, siplib/sip.h.in reorders a struct but there is no major version bump as suggested required by the comment in that file. This gives me concern that there will be some API or ABI breakage with this change, since there is currently no plan to rebuild reverse dependencies.

I don't see any discussion of possible API or ABI breakages in the bug so far. Please could you look into this possibility, both for Xenial and for Bionic? If it's definitely not an issue then that's fine; I'd just like to see an explanation from someone familiar with this package.

[Dmitry Shachnev (mitya57)]

Hi Robie, thanks for noticing that!

That reordering is not needed for this fix, it is a backporting error. When the reordering happened in upstream 4.19 release, the ABI version was bumped from 11.3 to 12.0, so it is a breaking change.

Please reject the current Xenial upload, I will do another upload without any changes to sip.h.in.

The Bionic upload does not have that change, so it may be accepted.

[Dmitry Shachnev (mitya57)]

I have uploaded an updated Xenial package to https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/3941.

Matthijs, if you have time, please test whether it still works for you.

[Matthijs (matthijsburgh)]

Hi Dmitry,

I have tested the updated Xenial package. Still works!

[Brian Murray (brian-murray)]

Hello Matthijs, or anyone else affected,

Accepted sip4 into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/sip4/4.19.7+dfsg-1ubuntu0.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Brian Murray (brian-murray)]

Hello Matthijs, or anyone else affected,

Accepted sip4 into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/sip4/4.17+dfsg-1ubuntu0.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-xenial to verification-done-xenial. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-xenial. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Matthijs (matthijsburgh)]

I have tested the following versions:

Xenial: amd64 4.17+dfsg-1ubuntu0.1
Bionic: amd64 4.19.7+dfsg-1ubuntu0.1

Both distributions are now fixed.

For reference, my CI setup:
Xenial fixed: https://travis-ci.com/github/MatthijsBurgh/orocos_kinematics_dynamics/jobs/308050115
Bionic fixed: https://travis-ci.com/github/MatthijsBurgh/orocos_kinematics_dynamics/jobs/308050672
Xenial before fix: https://travis-ci.com/github/MatthijsBurgh/orocos_kinematics_dynamics/jobs/307568631

[Brian Murray (brian-murray)]

The verification of the Stable Release Update for sip4 has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[Launchpad Janitor (janitor)]

This bug was fixed in the package sip4 - 4.19.7+dfsg-1ubuntu0.1

---------------
sip4 (4.19.7+dfsg-1ubuntu0.1) bionic; urgency=medium

  * Fixed issues with C/C++ member variable getters and setters. This
    restores the keeping of a reference to the containing object in the
    instance of a contained class (LP: #1861601).

 -- Dmitry Shachnev <email address hidden> Mon, 02 Mar 2020 20:04:07 +0300

[Launchpad Janitor (janitor)]

This bug was fixed in the package sip4 - 4.17+dfsg-1ubuntu0.1

---------------
sip4 (4.17+dfsg-1ubuntu0.1) xenial; urgency=medium

  * Fixed issues with C/C++ member variable getters and setters. This
    restores the keeping of a reference to the containing object in the
    instance of a contained class (LP: #1861601).

 -- Dmitry Shachnev <email address hidden> Sun, 15 Mar 2020 19:23:54 +0300