Skip to content
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

Memory leaks during parsing #64

Closed
sneves opened this issue Oct 8, 2020 · 4 comments
Closed

Memory leaks during parsing #64

sneves opened this issue Oct 8, 2020 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@sneves
Copy link

sneves commented Oct 8, 2020

Consider, say, the minimal input "a={\n" to toml::parse. We get a memory leak upon encountering the error Error while parsing inline table: expected key or closing '}', saw '\n'.

Looking at where this happens, parser::parse_inline_table, it is quite clear how this happens:

  • auto tab = new table{};
  • unexpected character found, so we run set_error_and_return_default("expected key or closing '}', saw '"sv, to_sv(*cp), "'"sv);

This leaves nobody to delete tab, and it is leaked. More importantly, this sort of pattern seems quite endemic to the parser code, so there are likely other places where leaks (or worse) happen.

@sneves sneves added the bug Something isn't working label Oct 8, 2020
@marzer
Copy link
Owner

marzer commented Oct 9, 2020

Thanks, I'll look into this.

this sort of pattern seems quite endemic to the parser code

Originally it used std::unique_ptr but that caused binary bloat of about 10% or so on some implementations so it was an easy win to refactor it out. I thought I was pretty thorough in ensuring there weren't any leaks introduced, though I guess I'll find out.

@marzer
Copy link
Owner

marzer commented Oct 9, 2020

Ah, can confirm that you're correct. Fewer leaks than you perhaps thought, though; one each on the error paths in parse_inline_table(), parse_array() and parse_key_value_pair_and_insert(). The actual impact of this was surprisingly low - 78 leaked objects from around 9100 generated during one run of the test suite. I was expecting it to be much worse!

@marzer marzer closed this as completed in fe0ac89 Oct 9, 2020
@marzer
Copy link
Owner

marzer commented Oct 9, 2020

There we go, all fixed. Thanks for the report @sneves!

@sneves
Copy link
Author

sneves commented Oct 9, 2020

Indeed, it looks fixed now. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants