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.

Sign up for GitHub

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

Jump to bottom

Prevent memory leak when exception is thrown in adl_serializer::to_json #3901

Merged
merged 7 commits into from
Mar 8, 2023
Merged

Prevent memory leak when exception is thrown in adl_serializer::to_json #3901

merged 7 commits into from
Mar 8, 2023

Conversation

barcode
Copy link
Contributor

@barcode barcode commented Dec 27, 2022 •
edited
Loading

(problem described in issue #3881)

There is a memory leak when throwing inside an adl serializers to_json method.

  • Add unit test
  • Implement fix
  • Fix Ci

Pull request checklist

Read the Contribution Guidelines for detailed information.

  • Changes are described in the pull request, or an existing issue is referenced.
  • The test suite compiles and runs without error.
  • Code coverage is 100%. Test cases can be added by editing the test suite.
  • The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header files single_include/nlohmann/json.hpp and single_include/nlohmann/json_fwd.hpp. The whole process is described here.

@barcode barcode mentioned this pull request Dec 27, 2022
Closed
2 tasks
@github-actions
Copy link

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated.

@coveralls
Copy link

coveralls commented Dec 27, 2022 •
edited
Loading

Coverage Status

Coverage: 100.0%. Remained the same when pulling 79bae8a on barcode:fix_mem_leak_in_adl_serializer into b230614 on nlohmann:develop.

@barcode barcode force-pushed the fix_mem_leak_in_adl_serializer branch 4 times, most recently from f8cf474 to db58287 Compare December 28, 2022 07:55
@barcode
Copy link
Contributor Author

barcode commented Dec 28, 2022

The test detects the memleak: https://github.com/nlohmann/json/actions/runs/3792554026/jobs/6448842970#step:5:316

@github-actions github-actions bot added L and removed M labels Dec 28, 2022
@barcode
Copy link
Contributor Author

barcode commented Dec 28, 2022

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 barcode force-pushed the fix_mem_leak_in_adl_serializer branch 2 times, most recently from 4018619 to df39768 Compare December 29, 2022 07:02
@gregmarr
Copy link
Contributor

Since this keeps most of the handling in the json class rather than the new data class, such as the copy constructor and move constructor, should the assert invariant function stay there as well? Also, should the copy and move constructors of this class be defaulted? Seems like they should be deleted.

@barcode
Copy link
Contributor Author

barcode commented Dec 30, 2022 •
edited
Loading 

should the assert invariant function stay there as well?

in that case the assert invariant method is not called if an exception in the ctor occurred. Then we do not need to adapt that method to deal with broken objects. Since the dtor of an element is called after the objects dtor finishes, the call order would be OK as well (assert before destroy).

Also, should the copy and move constructors of this class be defaulted? Seems like they should be deleted.

we need at least the move ctor

json/include/nlohmann/json.hpp

Line 1196 in df39768

m_data(std::move(other.m_data))

The others we could delete.

@barcode barcode force-pushed the fix_mem_leak_in_adl_serializer branch from 580c36e to 0e2f01a Compare January 3, 2023 07:08
gregmarr
gregmarr reviewed Jan 3, 2023
View reviewed changes
include/nlohmann/json.hpp Outdated Show resolved Hide resolved
@barcode barcode force-pushed the fix_mem_leak_in_adl_serializer branch from 0e2f01a to cd0a103 Compare January 3, 2023 22:35
gregmarr
gregmarr reviewed Jan 3, 2023
View reviewed changes
tests/src/unit-regression2.cpp Outdated
@@ -36,7 +36,7 @@ using ordered_json = nlohmann::ordered_json;
#include <variant>
#endif

#ifdef JSON_HAS_CPP_20
#ifdef JSON_HAS_CPP_20 && __has_include(<span>)
Copy link
Contributor

@gregmarr gregmarr Jan 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proper form for this is #if defined(JSON_HAS_CPP_20) && ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I should sleep instead of writing code while tired...

@barcode barcode force-pushed the fix_mem_leak_in_adl_serializer branch 2 times, most recently from 0ed7496 to e9e1543 Compare January 4, 2023 06:36
@barcode
Copy link
Contributor Author

barcode commented Jan 4, 2023

I looked a bit at the ci_test_compilers_clang (10) issue.
This PR did not change the affected test and it worked in earlier builds.

It seems the compiler and changed to a newer version.

old:

Compiler: clang version 10.0.0; Target: x86_64-unknown-linux-gnu; Thread model: posix; InstalledDir: /usr/local/bin

new:

Compiler: Debian clang version 10.0.1-++20210313014605+ef32c611aa21-1~exp1~20210313125208.190; Target: x86_64-pc-linux-gnu; Thread model: posix; InstalledDir: /usr/bin

Since this issue has nothing to do with this PR. it probably should be fixed in a separate PR.

barcode added 4 commits January 31, 2023 19:29
Add unit test for leaking memory when throwing inside of an adl seria…
aadeb72 
…lizer

(problem described in issue nlohmann#3881)
Fix leaking memory when throwing inside of an adl serializer
5a1a575
Fix ci issues
96f75d2 
* Add checks for uninitialized m_value (this can happen due to exceptions in the ctor)
* Add missing noexcept
* Fix clang tidy errors
Move assert_invariant to surrounding basic_json class
3613fa6
@barcode barcode force-pushed the fix_mem_leak_in_adl_serializer branch from e9e1543 to 1210c3e Compare January 31, 2023 18:30
@barcode barcode force-pushed the fix_mem_leak_in_adl_serializer branch from 8e99a23 to 79bae8a Compare February 1, 2023 17:24
@barcode
Copy link
Contributor Author

barcode commented Feb 2, 2023

Work on this PR is done and it only needs a review.

nlohmann
nlohmann approved these changes Feb 7, 2023
View reviewed changes
Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@nlohmann nlohmann linked an issue Feb 7, 2023 that may be closed by this pull request
Memory leak when exception is thrown in adl_serializer::to_json #3881
Closed
2 tasks
@nlohmann nlohmann added this to the Release 3.11.3 milestone Feb 7, 2023
@nlohmann nlohmann merged commit bbe337c into nlohmann:develop Mar 8, 2023
@nlohmann
Copy link
Owner

nlohmann commented Mar 8, 2023

Thanks!

@raphael-grimm raphael-grimm deleted the fix_mem_leak_in_adl_serializer branch August 4, 2023 07:21
@nlohmann nlohmann mentioned this pull request Nov 28, 2023
Merged
@gmyers18 gmyers18 mentioned this pull request Sep 5, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregmarr gregmarr gregmarr left review comments

@nlohmann nlohmann nlohmann approved these changes

Assignees
No one assigned
Labels
L tests
Projects
None yet
Milestone
Release 3.11.3
Development

Successfully merging this pull request may close these issues.

Memory leak when exception is thrown in adl_serializer::to_json
4 participants
@barcode @coveralls @gregmarr @nlohmann