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

Fix memleak when process policies #3301

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

JetXujing
Copy link
Contributor

Fix memleak when process policies

@pmatilai pmatilai requested review from pmatilai and removed request for pmatilai September 19, 2024 08:02
@pmatilai pmatilai self-assigned this Sep 19, 2024
@pmatilai
Copy link
Member

This will fix a leak on early exit or case of single policy, but it should additionally be freed in the for-loop. But... I seriously doubt the policy does anything meaningful these days and the more merciful solution would be be just axe the whole thing.

Are you actually trying to use the SELinux policy stuff for something, or is this just something spotted by static analysis?

@JetXujing
Copy link
Contributor Author

This is what I found by static analysis of the code.

@JetXujing
Copy link
Contributor Author

This will fix a leak on early exit or case of single policy, but it should additionally be freed in the for-loop.

Yes, it should be freed in the for-loop

@pmatilai
Copy link
Member

You don't need to test against NULL, and you can just assign like this, it always returns NULL for this very purpose:
optCon = poptFreeContext(optCon)

@JetXujing
Copy link
Contributor Author

You don't need to test against NULL, and you can just assign like this, it always returns NULL for this very purpose: optCon = poptFreeContext(optCon)

Thank you for your review comments. The poptFreeContext function has no return value, is the usage of optCon = poptFreeContext(optCon) reasonable?

poptFreeContext declares as follows:
void poptFreeContext(poptContext con);

@pmatilai
Copy link
Member

What version of popt is that? Oh, at least the man page is wrong on that, it claims void but the actual header (on popt 1.19) is:

/**
 * Destroy context.
 * @param con           context
 * @return              NULL always
 */
poptContext poptFreeContext( poptContext con);

@pmatilai
Copy link
Member

Okay, it returns NULL at least back to popt 1.13 and we certainly don't care about older than that.

build/policies.c Outdated
@@ -298,6 +299,9 @@ static rpmRC processPolicies(rpmSpec spec, Package pkg, int test)
free(name);
free(types);

if (optCon)
Copy link
Member

Choose a reason for hiding this comment

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

Don't bother checking for the NULL, poptFreeContext() does it anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@JetXujing
Copy link
Contributor Author

What version of popt is that? Oh, at least the man page is wrong on that, it claims void but the actual header (on popt 1.19) is:

/**
 * Destroy context.
 * @param con           context
 * @return              NULL always
 */
poptContext poptFreeContext( poptContext con);

The code has been updated.
The man page I looked at earlier. I reconfirmed the code and found that the return value had changed.

@pmatilai pmatilai merged commit 937e725 into rpm-software-management:master Sep 19, 2024
1 check passed
@pmatilai
Copy link
Member

Thanks for the patch.

It was also useful to spot the error in popt manual, submitted a fix for that: rpm-software-management/popt#125

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