Fix memory leak in ValidateSchema

[martinhsv]

This pull request addresses the memory leak listed as 3) in #2469 .

This is analogous to #2471 .

Prior to this change, second and subsequent calls to this object's evaluate function would cause a memory leak, when m_validCtx was assigned a new pointer value before the previously-pointed-to memory had been free'd.

[zimmerle]

This issue is an exciting one. Let me highlight some pieces of code -

Looking at the code

The variable instantiation happens here -
https://github.com/SpiderLabs/ModSecurity/blob/6464b9a1c570161cae7cffc93807c2a340967a2b/src/operators/validate_schema.cc#L85-L93

In the same function, we have some cleanups -
https://github.com/SpiderLabs/ModSecurity/blob/6464b9a1c570161cae7cffc93807c2a340967a2b/src/operators/validate_schema.cc#L131-L132

The variable m_validCtx is not being held on the cleanups. That is odd; I guess it was forgotten, and it is working like that for a while.

My 2 cents on the fix

I am afraid I would prefer to have a different approach on the fix -

1. Having xmlSchemaFreeValidCtxt(m_validCtx); altogether with the rest of the cleanups. About line 131-132.
2. Changing the scope of the variables listed in (A) to be contiguous in the function. There is no need for those variables to be class members.

As suggested in the patch, the cleanup happens on the subsequent request, not on the current one. I am not saying that the library is thread-safe at this level, but I think it hurts the best practices having this as is. The problem preceding @martinhsv' patch: the scope is too broad.

I understand that who was implementing this operator a while ago - myself? 🤦 - was wondering about having those initializations optimized on the operator initialization. I guess, because m_validCtx is not const in xmlSchemaValidateDoc [1] it is impractical to have it in long term. Eventually, evaluate(...) will become const in the operator context.

I understand the overhead of setting up on the evaluation as oppose to instantiate on the initialization. Nevertheless, not having the parameter as const on the library will demand a cast to 'constify' it. Consequently, not gentle and could lead to a race condition very annoying for debugging 😥 in a not so popular functionality.

Further study can be done to identify whatever those are treated a const or not by libxml. Moreover, the rationale is valid for all parameters.

(A) Variables that I think we should reduce the scope

https://github.com/SpiderLabs/ModSecurity/blob/6464b9a1c570161cae7cffc93807c2a340967a2b/src/operators/validate_schema.h#L136-L138

Well notice @martinhsv!! Thanks!

[martinhsv]

I agree that this pull request is problematic. Also agree that, if we still want to call xmlSchemaNewValidCtxt() on each call of evaluate(), that it makes sense to get rid of m_validCtx as a member variable and use a stack variable within the evaluate() function instead.

[martinhsv]

Closing in favour or #2479