RHEL Engineering is moving the tracking of its product development work on RHEL 6 through RHEL 9 to Red Hat Jira (issues.redhat.com). If you're a Red Hat customer, please continue to file support cases via the Red Hat customer portal. If you're not, please head to the "RHEL project" in Red Hat Jira and file new tickets here. Individual Bugzilla bugs in the statuses "NEW", "ASSIGNED", and "POST" are being migrated throughout September 2023. Bugs of Red Hat partners with an assigned Engineering Partner Manager (EPM) are migrated in late September as per pre-agreed dates. Bugs against components "kernel", "kernel-rt", and "kpatch" are only migrated if still in "NEW" or "ASSIGNED". If you cannot log in to RH Jira, please consult article #7032570. That failing, please send an e-mail to the RH Jira admins at rh-issues@redhat.com to troubleshoot your issue as a user management inquiry. The email creates a ServiceNow ticket with Red Hat. Individual Bugzilla bugs that are migrated will be moved to status "CLOSED", resolution "MIGRATED", and set with "MigratedToJIRA" in "Keywords". The link to the successor Jira issue will be found under "Links", have a little "two-footprint" icon next to it, and direct you to the "RHEL project" in Red Hat Jira (issue links are of type "https://issues.redhat.com/browse/RHEL-XXXX", where "X" is a digit). This same link will be available in a blue banner at the top of the page informing you that that bug has been migrated.
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: 2021-10-17 19:40 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:
Embargoed:

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 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.
Note You need to log in before you can comment on or make changes to this bug.