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

pam_exec: arggv not free'd on error #444

Closed
setharnold opened this issue Feb 23, 2022 · 4 comments · Fixed by #448
Closed

pam_exec: arggv not free'd on error #444

setharnold opened this issue Feb 23, 2022 · 4 comments · Fixed by #448

Comments

@setharnold
Copy link

arggv = calloc (argc + 4, sizeof (char *));

Hello, the arggv array and referenced strings aren't free'd on any of the error paths. This is probably not a big deal but does seem out of character given other things are free'd in the same error paths.

Thanks

@ldv-alt
Copy link
Member

ldv-alt commented Feb 23, 2022

Thanks for reporting.
I think freeing memory right before pam_syslog() followed by _exit(ENOMEM) is rather useless,
so wouldn't it be better to remove all those free(envlist) calls instead?

What's worse is the unchecked return value of strdup in

arggv[i] = strdup(argv[i+optargc]);

@setharnold
Copy link
Author

Heh, I thought about that, too, but it's nice to clean up memory before exit() to keep valgrind reports short and sweet. It's up to you what you'd rather do, but I'd argue for consistency either way.

I can't believe I looked right past that unchecked strdup() though. Sigh.

Thanks

@ldv-alt ldv-alt changed the title arggv not free'd on error pam_exec: arggv not free'd on error Feb 23, 2022
@ldv-alt
Copy link
Member

ldv-alt commented Feb 23, 2022

@thkukuk Thorsten, I think it's your code, would you prefer freeing arggv[] array before _exit() calls, or removing free(envlist) calls?

@thkukuk
Copy link
Contributor

thkukuk commented Feb 23, 2022

It's not only arggv which is not free'd, but freeing envlist also leaves a lot of unreachable allocations behind.
So before we write long, complex code to free all kind of memory, just remove free(envlist).

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 a pull request may close this issue.

3 participants