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 leak when exception is thrown in adl_serializer::to_json #3881

Closed
1 of 2 tasks
mjerabek opened this issue Dec 14, 2022 · 2 comments · Fixed by #3901
Closed
1 of 2 tasks

Memory leak when exception is thrown in adl_serializer::to_json #3881

mjerabek opened this issue Dec 14, 2022 · 2 comments · Fixed by #3901
Labels
kind: bug solution: proposed fix a fix for the issue has been proposed and waits for confirmation

Comments

@mjerabek
Copy link

Description

Throwing an exception in serializer function after the output json object is already initialized results in a memory leak (detected with both ASan and valgrind):

void adl_serializer<Foo>::to_json(json& j, Foo const& f) {
    j["a"] = 42;
    throw std::runtime_error("error");
}

Reproduction steps

Reproducer here: https://godbolt.org/z/dvorMExWh

Expected vs. actual results

Expected: no memory leak

Minimal code example

No response

Error messages

No response

Compiler and operating system

linux, clang 15, gcc 12.2

Library version

3.11.2, godbolt trunk, 7f72eed

Validation

@gregmarr
Copy link
Contributor

This is throwing inside the constructor. When the constructor doesn't complete without exception, then the destructor is not run, meaning that m_value.destroy(m_type); is not called. Cleaning up from this is going to be tricky.
https://isocpp.org/wiki/faq/exceptions#selfcleaning-members

Off the top of my head, the constructor code that calls out to the adl serializer could detect an exception and call m_value.destroy(m_type); manually before rethrowing.

@barcode
Copy link
Contributor

barcode commented Dec 27, 2022

This can be solved by either using a base class or a member var to do the cleanup.
I would prefer one of those solutions, since they solve the problem of leaking memory permanently.

The stl uses base classes for containers managing memory (https://github.com/gcc-mirror/gcc/blob/7c755fd9018821b79ddc32ee507897860510986c/libstdc%2B%2B-v3/include/bits/stl_vector.h#L423).
If we use a base class, we do not need to change the variable names. It would be enough to move some code into this base class.

I will implement a solution using a base class in #3901.

/edit
I went with the solution using a member var, since it requires less changes to the existing code and the resulting code is simpler to understand.

barcode pushed a commit to barcode/json that referenced this issue Dec 28, 2022
barcode pushed a commit to barcode/json that referenced this issue Dec 28, 2022
barcode pushed a commit to barcode/json that referenced this issue Dec 28, 2022
barcode pushed a commit to barcode/json that referenced this issue Dec 28, 2022
barcode pushed a commit to barcode/json that referenced this issue Jan 31, 2023
@nlohmann nlohmann linked a pull request Feb 7, 2023 that will close this issue
7 tasks
@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Feb 7, 2023
@nlohmann nlohmann added this to the Release 3.11.3 milestone Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants