-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 memory leak in the example #2578
base: v3/master
Are you sure you want to change the base?
Conversation
error string is created via strdup and it must be passed to free to avoid a memory leak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @bsulmanas,
Please see the review inline.
@@ -70,6 +70,7 @@ int main (int argc, char **argv) | |||
end: | |||
msc_rules_cleanup(rules); | |||
msc_cleanup(modsec); | |||
free(error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @bsulmanas,
The variable error is indeed filled if there is an error. Otherwise, it may point towards NULL. Considering the former case and the suggested patch, it will lead to a double-free-ish mistake. The variable error can be sefely freed on lines 45 and 54, where its usage is known.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, zimmerle's suggestion is a good solution.
To keep it at the 'end' label as your code is right now, you would need to wrap it inside a if ( error != NULL )
because reaching the end label doesn't mean there was an error, and if there was no error the code would try to do a free( NULL );
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I'm fairly sure that the PR is safe as it stands. Calling free on a NULL pointer has been considered safe from very early days.
See, for example 7.20.3.2 from https://www.open-std.org/JTC1/SC22/wg14/www/docs/n1124.pdf
"If ptr is a null pointer, no action occurs"
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
error
string is created viastrdup
and it must be passed tofree()
to avoid a memory leak.Not really important in this particular case, but if would be consistent with
msc_rules_cleanup(rules)
andmsc_cleanup(modsec)