|Summary:||nm_utils_parse_variant_attributes() returns floating GVariants|
|Product:||Red Hat Enterprise Linux 7||Reporter:||Beniamino Galvani <bgalvani>|
|Component:||NetworkManager||Assignee:||Beniamino Galvani <bgalvani>|
|Status:||CLOSED NOTABUG||QA Contact:||Desktop QE <desktop-qa-list>|
|Version:||7.5||CC:||atragler, bgalvani, fgiudici, lrintel, rkhan, sukulkar, thaller|
|Fixed In Version:||Doc Type:||If docs needed, set a value|
|Doc Text:||Story Points:||---|
|Last Closed:||2018-11-22 17:34:55 UTC||Type:||Bug|
|oVirt Team:||---||RHEL 7.3 requirements from Atomic Host:|
|Cloudforms Team:||---||Target Upstream Version:|
Description Beniamino Galvani 2018-06-25 15:43:47 UTC
The GVariants in the hash table returned by nm_utils_parse_variant_attributes() are floating. This is unexpected in my opinion. We should at least document the current behavior, and also introduce a new API with the correct one (GVariant have one reference and are not floating). See also https://bugzilla.redhat.com/show_bug.cgi?id=1555013#c9
Comment 2 Beniamino Galvani 2018-06-27 08:23:02 UTC
Pushed branch bg/parse-var-attr-ref-rh1594887 for review.
Comment 3 Thomas Haller 2018-07-01 09:08:45 UTC
> clients: fix memory leak when parsing routes why was this not caught be unit-tests and valgind? Could you extend the unit-tests, so that the leak shows up? I know, we discussed this already, and there was a consensus. Fair enough, if everybody else agrees with this approach. For the record, I wouldn't do this, because: Too bad we missed 1.12.0 with these fixes. So, by ~fixing~ API now, we not only break behaviour for 1.10.x, but also 1.12.0. I don't like that. I a library introduced a bug, it's first and foremost the responsibility of the library (developers) to take the burden to improve the situation. Especially as in this case. The previous behaviour -- while odd -- was indeed used correctly (by callers sinking the reference). If this would be a bug, which unavoidable lead to leak/memory-corruption, than of course the API can only be fixed. But the old API did not result in buggy behaviour, that's why I think it would be lazy of us, to just change behaviour and call it a fix. We should pride ourself of not changing behaviour unless unavoidable. That doesn't seem to be the case here.
Comment 4 Thomas Haller 2018-09-14 15:30:35 UTC
See upstream commits: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=31bda1b8373c71ecb41d8208aa2bc9c88999d988 https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=e645aeb12ccefb1fbe18bb33d046ee649de86da0 The first, is cherry-picked (and modified) from bg/parse-var-attr-ref-rh1594887 branch.
Comment 5 Thomas Haller 2018-09-19 12:31:34 UTC
This missed rhel-7.6. It also missed rhel-8.0 (aside the bits from comment 4).
Comment 6 Beniamino Galvani 2018-11-22 17:34:55 UTC
So the only reasonable approach would to add a new API with the new (better?) behavior, but it seems a waste of effort as the current API is fine and its behavior well documented. I'm going to close this.