v3.1-experimental branch -- multiple memory leaks

[martinhsv]

Within current v3/dev/3.1-experimental, I believe I have identified some additional memory leaks that have not yet been addressed:

1. set_env.cc:41
2. modsec_build.cc:30
3. validate_dtd.cc:52 (this one may be a false positive -- needs more investigation)

[zimmerle]

set_env

I believe you are referring to:

https://github.com/SpiderLabs/ModSecurity/blob/afefda53c69bb17e2ec16d24dd6fbc2c8fd7d063/src/actions/set_env.cc#L40

Is it a consequence of strdup? that was used to set the variable. It is gone when the process exit.

modsec_bulid

Duplicate as the memory leak reported on #2376. Fixed.

Here the variable is generated:
https://github.com/SpiderLabs/ModSecurity/blob/afefda53c69bb17e2ec16d24dd6fbc2c8fd7d063/src/variables/modsec_build.cc#L31

Here is the constructor for VariableValue:
https://github.com/SpiderLabs/ModSecurity/blob/5f38e37f73f151db49e3f34981adc96d03633889/headers/modsecurity/variable_value.h#L98-L108

validate_dtd

https://github.com/SpiderLabs/ModSecurity/blob/afefda53c69bb17e2ec16d24dd6fbc2c8fd7d063/src/operators/validate_dtd.cc#L51-L53

Is it the right place?

[martinhsv]

set_env.cc: Perhaps I'm misunderstanding you but waiting for the process to terminate doesn't help here. Because the memory allocation is occurring in the execute function, this is a per-transaction leak.

[martinhsv]

modsec_build.cc: Yes, the heap memory allocation is happening in that particular VariableValue constructor. And it is due to the line with that push_back in modsec_build.cc.

But, like the previous case, because this is occurring within the evaluate function, this is a per-process leak.

[martinhsv]

validate_dtd:

No, it's at validate_dtd.cc:52 (line number from 3.1-experimental):

m_dtd = xmlParseDTD(NULL, (const xmlChar *)m_resource.c_str());

I haven't fully confirmed this one, but I believe this is the same sort of issue; this line of code is in the evaluate function which means it runs every transaction and overwrites the the previous value of the pointer m_dtd. Which means that the memory that had been pointed to is leaked on a per-transaction basis.

[martinhsv]

None of the above were already fixed in 3.1-experimental when I ran the tests on Friday -- I did make sure to run against the current revision of 3.1-experimental as of that date.

[WGH-]

In glibc, setenv seems to behave better wrt memory management, as it tries to reuse the KEY=VALUE pairs if possible. But it will still leak memory if called with different arguments, and nothing can be done to prevent this.

What's the use case for the setenv operator, anyway? Manipulating the global process state from transaction handling code seems like a bad idea.

[tomasdeml]

FYI #2381 (comment)

[martinhsv]

Regarding the 2nd of 3 leaks reported in this issue (i.e. modsec_build), as stated

modsec_bulid

Duplicate as the memory leak reported on #2376. Fixed.

Confirmed. No further work required for this one.

[martinhsv]

Adding an additional one:

4. validate_schema.cc:85

This is structurally similar to the previous-reported validate_dtd.cc leak, and is likewise a per-transaction memory leak

    m_validCtx = xmlSchemaNewValidCtxt(m_schema);

[martinhsv]

I'm going to recommend we leave aside the leak identified above as '1) set_env.cc:41'

If and when we want to improve this, a likely avenue (as suggested by @WGH- ) would be to use setenv() instead of putenv().

But for now:

• this issue is not new to 3.1 -- it is pre-existing in 3.0.x, which means taking time to deal with this now is of doubtful immediate value
• no one in the community has complained about it so far, so I expect that the ModSecurity setenv action is rarely used (or if it is used at all, it is not used on a per-transaction basis)

[zimmerle]

Summarize:

• set_env.cc
• modsec_build.cc
• validate_dtd.cc

It is essential to mention that those are issues regarding specific functionality - One may face it upon utilizing those resources' unitedness.

set_env.cc

There is interesting information on the subject here: StackOverFlow.

Indeed, setenv seems to be a better choice than putenv. Thank you, @martinhsv and @WGH-, for raising this. I knew that putenv was not so gentle; the alternative of using setenv seems good. I was studying the differences and I end up changing the code here - 6464b9a

modsec_build.cc

It is fixed already. Likely to be a dup.

validate_dtd.cc

Still have to look in this one. I do have some concerns about a possible race conditions. This possible race condition was the motivation to initialize this member (m_dtd) on ever evaluation. Back then, I did not notice the leak. I will have a look at it tomorrow and update here accordingly.

Thanks @martinhsv nice effort!

[martinhsv]

Just so it doesn't get forgotten ...

validate_schema.cc was added later as a fourth item. It is comparable to the validate_dtd.cc issue and has a similar pull request available.

[zimmerle]

Last update -

• set_env.cc
• modsec_build.cc
• validate_dtd.cc
• validate_schema.cc

All issues are now closed!