Bug 1594887 - nm_utils_parse_variant_attributes() returns floating GVariants
Summary: nm_utils_parse_variant_attributes() returns floating GVariants
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: NetworkManager
Version: 7.5
Hardware: Unspecified
OS: Unspecified
medium
medium
Target Milestone: rc
: ---
Assignee: Beniamino Galvani
QA Contact: Desktop QE
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-06-25 15:43 UTC by Beniamino Galvani
Modified: 2019-02-26 15:56 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-11-22 17:34:55 UTC
Target Upstream Version:


Attachments (Terms of Use)

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 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.


Note You need to log in before you can comment on or make changes to this bug.