Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/icc: Always close profile when loading fails #150

Merged

Conversation

jadahl
Copy link
Contributor

@jadahl jadahl commented Mar 3, 2023

cd_icc_load() always takes ownership of the passed profile, but will leak it if the initial condition fails and the funtion returns an error. Only if the actual profile loading fails, will the passed profile eventually not be leaked. Fix this by closing it immediately in the first case.


This might not be a good solution, but the API doesn't allow the caller to close the profile after calling the load function, so we need something to not leak here. The alternative is to always set the internal pointer to have it closed on finalization; that would avoid any user-after-free if the caller makes the assumption that the profiles is still valid (which might not be that far fetched since the API mentions it's kept around until finalize).

Another alternative is to treat it as a programming error to not have context ID (but I guess still set an error), and still leak in that case.

cd_icc_load() always takes ownership of the passed profile, but will
leak it if the initial condition fails and the funtion returns an error.
Only if the actual profile loading fails, will the passed profile
eventually not be leaked. Fix this by closing it immediately in the
first case.
@hughsie
Copy link
Owner

hughsie commented Mar 3, 2023

Would a return_val_if_fail be a better fix? i.e. #149

@jadahl
Copy link
Contributor Author

jadahl commented Mar 3, 2023

Would a return_val_if_fail be a better fix? i.e. #149

That's indeed an option and what I meant with "treat it as a programming error". The problem with that, if we don't also set an error, is that callers will now start crashing if they attempt to use the GError.

@hughsie
Copy link
Owner

hughsie commented Mar 3, 2023

Okay, lets do it your way for now, and re-assess if needed in the future. Thanks.

@hughsie hughsie merged commit 4a33e7c into hughsie:main Mar 3, 2023
@jadahl
Copy link
Contributor Author

jadahl commented Mar 3, 2023

What I'm not sure of here is whether there are users who assumed it'd stay alive until finalize.

@hughsie
Copy link
Owner

hughsie commented Mar 3, 2023

I think it's fine; we're not drowning in users here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants