Skip to content

Fixed memory leak in UTMP processing #1329

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.

Sign up for GitHub

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

Jump to bottom
Closed
Karlson2k wants to merge 3 commits into from
Closed

Fixed memory leak in UTMP processing #1329

Karlson2k wants to merge 3 commits into from

Conversation

@Karlson2k
Copy link
Contributor

@Karlson2k Karlson2k commented Aug 10, 2025

Obvious bug. Nothing to comment.

@Karlson2k
lib/utmp.c: Fix memory leak
53d2939 
Fixes: f40bdfa (02-08-2023; "libmisc: implement `get_session_host()`")
Signed-off-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev>
@Karlson2k
Copy link
Contributor Author

Karlson2k commented Aug 10, 2025 •
edited
Loading

Seems to be a copy of a3667fd from PR #1290

alejandro-colomar
alejandro-colomar reviewed Aug 10, 2025
View reviewed changes
lib/utmp.c
#if defined(HAVE_STRUCT_UTMPX_UT_HOST)
if ((ut != NULL) && (ut->ut_host[0] != '\0')) {
*out = XSTRNDUP(ut->ut_host);
free (ut);
Copy link
Collaborator

@alejandro-colomar alejandro-colomar Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is being fixed already: a3667fd

Copy link
Contributor Author

@Karlson2k Karlson2k Aug 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, noticed this too after making of this PR.
There is a second commit in this PR.

Copy link
Contributor Author

@Karlson2k Karlson2k Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably it worth to merge the fix separately from the larger PR as it looks like independent?

Copy link
Collaborator

@alejandro-colomar alejandro-colomar Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe; I'll think about it.

Copy link
Collaborator

@alejandro-colomar alejandro-colomar Aug 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this patch set. It adds [[gnu::malloc(free)]], which I didn't, and that helps prevent this bug in the future. I'll merge it. Thanks!

Copy link
Contributor Author

@Karlson2k Karlson2k Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are welcome.
I'll update it based on your comments.

@Karlson2k
Copy link
Contributor Author

Karlson2k commented Aug 10, 2025

Related idea.
Instead of all these allocations/freeings, would it be better to use a static buffer for utmpx entry in get_current_utmp()?
The size of the entry is not large and suits quite well for a static buffer.
get_current_utmp() would still return a pointer, but there would be no need to free it.
This approach, however, will make get_current_utmp() thread-unsafe, but the underlying getutxent() is not thread-safe anyway (even now, if get_current_utmp() is called in two threads simultaneously, the result will be incorrect).

The same approach is used in glibc.

@Karlson2k Karlson2k mentioned this pull request Aug 10, 2025
Closed
@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Aug 10, 2025

Related idea. Instead of all these allocations/freeings, would it be better to use a static buffer for utmpx entry in get_current_utmp()? The size of the entry is not large and suits quite well for a static buffer. get_current_utmp() would still return a pointer, but there would be no need to free it. This approach, however, will make get_current_utmp() thread-unsafe, but the underlying getutxent() is not thread-safe anyway (even now, if get_current_utmp() is called in two threads simultaneously, the result will be incorrect).

The same approach is used in glibc.

I prefer avoiding static in general. It makes it more difficult to reason about the code. It's like global variables, after all.

@Karlson2k
Copy link
Contributor Author

Karlson2k commented Aug 10, 2025

I prefer avoiding static in general. It makes it more difficult to reason about the code. It's like global variables, after all.

I personally prefer avoiding global variables (as the scope is not controlled), but a small static buffer inside the function is perfectly fine (the scope is fully controlled), it can make code better, faster, smaller, easier-to-read and safer at the same time.
However, here it is up to you. I'll keep allocations.

alejandro-colomar
alejandro-colomar approved these changes Aug 10, 2025
View reviewed changes
Copy link
Collaborator

@alejandro-colomar alejandro-colomar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

Reviewed-by: Alejandro Colomar <alx@kernel.org>

alejandro-colomar
alejandro-colomar reviewed Aug 10, 2025
View reviewed changes
@Karlson2k
lib/utmp.c: Add ATTR_MALLOC(free) attribute
3fdefc3 
Signed-off-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev>
@Karlson2k
lib/utmp.c: Add explicit include <stdlib.h> for free() and other func…
822fe7f 
…tions

Signed-off-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev>
@Karlson2k Karlson2k force-pushed the utmp_fix_mem_leak_01 branch from ba8c330 to 822fe7f Compare August 11, 2025 00:50
@Karlson2k Karlson2k mentioned this pull request Aug 11, 2025
Merged
@Karlson2k
Copy link
Contributor Author

Karlson2k commented Aug 11, 2025

Updated as requested.
However, I suggest to use PR #1332 instead.

@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Aug 11, 2025

Updated as requested. However, I suggest to use PR #1332 instead.

Thanks! I merged #1332.

@Karlson2k Karlson2k deleted the utmp_fix_mem_leak_01 branch August 11, 2025 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alejandro-colomar alejandro-colomar alejandro-colomar approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Karlson2k @alejandro-colomar