Bug 1594887

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>
Severity:	medium	Docs Contact:
Priority:	medium
Version:	7.5	CC:	atragler, bgalvani, fgiudici, lrintel, rkhan, sukulkar, thaller
Target Milestone:	rc
Target Release:	---
Hardware:	Unspecified
OS:	Unspecified
Whiteboard:
Fixed In Version:		Doc Type:	If docs needed, set a value
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2018-11-22 17:34:55 UTC	Type:	Bug
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:

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.