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

http-support.c: Fix memory leak from Avahi #49

Closed
wants to merge 1 commit into from
Closed

http-support.c: Fix memory leak from Avahi #49

wants to merge 1 commit into from

Conversation

zdohnal
Copy link
Member

@zdohnal zdohnal commented Nov 23, 2020

Avahi library allocates memory for 'value' within 'avahi_string_list_get_pair()' function - this allocated memory is returned as a pointer via parameter to 'http_resolve_cb()' function scope, so it needs to be freed.

It can be reproduced via cups-browsed if the printer/CUPS server is within local network and it is discoverable by Avahi - then if cups-browsed is run within valgrind, it will report the following issue:

==4931== 21 bytes in 1 blocks are definitely lost in loss record 356 of 1,326
==4931==    at 0x4839809: malloc (vg_replace_malloc.c:307)
==4931==    by 0x48539B4: UnknownInlinedFun (malloc.c:68)
==4931==    by 0x48539B4: avahi_malloc (malloc.c:107)
==4931==    by 0x4853EC9: avahi_memdup (malloc.c:252)
==4931==    by 0x485522A: avahi_string_list_get_pair (strlst.c:513)
==4931==    by 0x4CA24D7: http_resolve_cb (http-support.c:2602)
==4931==    by 0x486A44A: avahi_service_resolver_event (resolver.c:146)
==4931==    by 0x486B3BC: filter_func (client.c:256)
==4931==    by 0x4F19384: dbus_connection_dispatch (in /usr/lib64/libdbus-1.so.3.19.13)
==4931==    by 0x4863BDF: dispatch_timeout_callback (dbus-watch-glue.c:105)
==4931==    by 0x4856577: avahi_simple_poll_dispatch (simple-watch.c:570)
==4931==    by 0x4CA8C37: _httpResolveURI (http-support.c:2045)
==4931==    by 0x4C02C59: resolve_uri (ipp.c:88)

The report is gone if the fix is applied.

Would you mind merging it into the project?

Thank you in advance!

Avahi library allocates memory for 'value' within 'avahi_string_list_get_pair()' function - this allocated memory is returned as a pointer via parameter to 'http_resolve_cb()' function scope, so it needs to be freed.
michaelrsweet added a commit that referenced this pull request Nov 24, 2020
@michaelrsweet
Copy link
Member

@zdohnal Looks like we need to use avahi_free, not free. Also, I noticed another spot (where we get the UUID) where the value wasn't freed...

[master e38d6dc] Fix memory leak (Issue #49)

@michaelrsweet michaelrsweet self-assigned this Nov 24, 2020
@michaelrsweet michaelrsweet added bug Something isn't working priority-high labels Nov 24, 2020
@michaelrsweet michaelrsweet added this to the v2.3.3op1 milestone Nov 24, 2020
@zdohnal
Copy link
Member Author

zdohnal commented Nov 24, 2020

I haven't checked whether avahi_free() is in Avahi public API after finding generic free() works as well, sorry :( .
It was a pure luck free() worked as well :D - because avahi library doesn't set allocator in our case, so avahi_free() just calls a simple free().

You're right, avahi_free() is better since it is in public API and we cannot depend on allocator not being defined.

Thanks for checking!

@michaelrsweet
Copy link
Member

Yeah, when I saw this I checked up on the API to make sure I understood the memory allocation requirements (“did I forget to do this or is this an Avahi bug?”) and noticed the note about using avahi_free. Then I looked for other uses of that function (though I should probably look in the other DNS-SD code in CUPS too...)

@zdohnal
Copy link
Member Author

zdohnal commented Nov 24, 2020

That *get_pair function is called only twice, which you covered, so I think we are clear from this leak.

@michaelrsweet
Copy link
Member

@zdohnal Agreed, I don't see any other API usage that requires us to call avahi_free.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority-high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants