Skip to content

Fix memory leak in ValidateDTD m_dtd #2471

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
Closed

Fix memory leak in ValidateDTD m_dtd #2471

wants to merge 1 commit into from

Conversation

martinhsv
Copy link
Contributor

@martinhsv martinhsv commented Dec 7, 2020

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

The function call in this line of code allocates heap memory. The second and subsequent calls to this function resulted in the the m_dtd pointer simply being assigned a new value, which meant that the heap memory previously pointed-to was leaked on a per-transaction basis.

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

With this new code, before the new parse-and-assign, we'll check if m_dtd is not null, and if so do the proper cleanup.

(I have addressed the leak in this pull request, but a larger issue we may want to consider for later implementation: Do we really want to be calling xmlParseDTD for every transaction? Granted, if we only want to do this once rather than per-transaction, we would need to consider the use case where the resource content changes over time -- particularly if it is an internet resource. We could decide we don't care, or perhaps adopt a cacheing-and-periodic-update strategy)

@martinhsv martinhsv added this to the v3.1.0 milestone Dec 7, 2020
@zimmerle zimmerle force-pushed the v3/dev/3.1-experimental branch from 5f38e37 to 6464b9a Compare December 9, 2020 21:44
@zimmerle
Copy link
Contributor

As mentioned on #2473 I am afraid that the leak of const utilization on the libxml may be an impediment to actually have those optimized on run-time (evaluate) as oppose to configuration time (init). The plan is to have evaluate as const in the operator context. Lets have a chat on #2473 and we get back to this one.

@zimmerle zimmerle force-pushed the v3/dev/3.1-experimental branch 6 times, most recently from 0606332 to 9c23d61 Compare December 10, 2020 17:44
@martinhsv
Copy link
Contributor Author

Closing this in favour of #2478

@martinhsv martinhsv closed this Dec 15, 2020
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 this pull request may close these issues.

2 participants