Skip to content

Fix memory leak in msc_rules_* C APIs #1765

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

Closed
wants to merge 1 commit into from

Conversation

weliu
Copy link

@weliu weliu commented May 3, 2018

When msc_rules_* APIs fail, the error string will return to indicate the reason. It was a duplicate of an internal string. However, it will be duplicated again after those calls in Nginx connector. For example:

In ngx_http_modsecurity_merge_loc_conf():

rules = msc_rules_merge(c->rules_set, p->rules_set, &error);
if (rules < 0) {
return strdup(error);
}

Since the type of error is 'const char **', I believe that we should not duplicate it inside those APIs and let the caller decide how to handle it.

@zimmerle zimmerle self-requested a review May 3, 2018 18:36
@zimmerle zimmerle self-assigned this May 3, 2018
@zimmerle zimmerle added the 3.x Related to ModSecurity version 3.x label May 3, 2018
@p0pr0ck5
Copy link
Contributor

p0pr0ck5 commented May 3, 2018

Perhaps we should remove the duplicate strdup from the Nginx connector instead? The Apache connector does not call such a duplicate: https://github.com/SpiderLabs/ModSecurity-apache/blob/4e8854c46bd3c216d7834ab94605f45114794791/src/msc_config.c#L169-L183.

@weliu
Copy link
Author

weliu commented May 3, 2018

@p0pr0ck5 Since the Apache connector does not free the returned error string, there still exists memory leak if we only remove duplicate strdup from the Nginx connector.

zimmerle pushed a commit that referenced this pull request May 24, 2018
@zimmerle
Copy link
Contributor

Merged! Thanks!

@zimmerle zimmerle closed this May 24, 2018
@weliu
Copy link
Author

weliu commented May 31, 2018

@zimmerle Yes, I made a mistake, since the getParserError() return a temporary string, it is not safe for us to return its c_str. Sorry for the trouble I have made.

@zimmerle
Copy link
Contributor

No problem. I dint notice at first sight as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Related to ModSecurity version 3.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants