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

Possible memory leak on push_back #1971

Closed
davide9 opened this issue Mar 4, 2020 · 5 comments
Closed

Possible memory leak on push_back #1971

davide9 opened this issue Mar 4, 2020 · 5 comments
Assignees
Labels
kind: bug platform: visual studio related to MSVC release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@davide9
Copy link

davide9 commented Mar 4, 2020

I'm experiencing a memory leak when using push_back to add strings to a json object

Steps to reproduce:

  • Use a custom allocator to track memory allocations
  • Create a json object as array: json myJson = json::array_t{};
  • Add a string type object with push_back: myJson.push_back(String{"Hello leaking world"});
  • Destroy the json instance.
  • Experience the leak.

I'm using Visual Studio 2017 on Windows 10, compiling with standard Debug configuration in x64
My latest synch on this branch was on 28/02/2020 at 15:20 CET.

I've (probably) fixed the issue by removing (on the single header file) the line 19426: val.m_type = value_t::null; from the function void push_back(basic_json&& val).

From my debugging, the issue comes from the fact that the m_value.array->push_back(std::move(val)); is invoking the basic_json(const basic_json&) instead of the basic_json(basic_json&&), which creates a copy of the source value that should still be properly delete by the allocator. My fix shouldn't affect the final result since, if the move constructor is invoked in the push_back of 19422, the move constructor will take care of marking the type of the source basic_json as null.

Hope this will be helpful!

Cheers,

Davide.

@nlohmann nlohmann added the platform: visual studio related to MSVC label Mar 5, 2020
@ArtemSarmini
Copy link
Contributor

@davide9 could you provide minimal reproducible example?

@davide9
Copy link
Author

davide9 commented Mar 16, 2020

Here you can find the code with which I have the bug
src.txt

The issue seems related to the fact that there is no template< class U, class... Args > void construct( U* p, Args&&... args ); in my allocator, which prevent the std::vector::push_back from using the move constructor and letting it fallback to the copy-constructor.

@ArtemSarmini
Copy link
Contributor

It is indeed related, thanks for tracking. It's still a bug though.

@ArtemSarmini
Copy link
Contributor

I'll provide pull request soon, it seems only one line (that null assignment) needs to be changed.

@ArtemSarmini
Copy link
Contributor

By the way, confirmed on gcc 9.3, so it isn't msvc-specific.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug platform: visual studio related to MSVC release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests

3 participants