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

Leak #316

Closed
andysCaplin opened this issue Apr 6, 2021 · 6 comments · Fixed by microsoft/vcpkg#17126
Closed

Leak #316

andysCaplin opened this issue Apr 6, 2021 · 6 comments · Fixed by microsoft/vcpkg#17126

Comments

@andysCaplin
Copy link

Hi - I've upgraded our code from v0.162.2 to v0.163.0 and our valgrind run reported some leaks in jsoncons.

I've attached a file.

Our application is C code and the only issue I've had previously with leaks is that I needed to explicitly set memory allocated by the library to NULL to free it. i.e. when I call an API like this

{
    jsoncons::json        selected;

    selected = jsoncons::jsonpath::json_query(jsonob->json, pparams->jsonpath);
...
    selected = NULL;
}

The selected variable didn't seem to be freed when it went out of scope. I had to have the line setting it to NULL to make sure it was freed.
But otherwise I haven't had any other issues with leaks in any version up to this one.

val-jsonob.txt

@andysCaplin andysCaplin added the Bug label Apr 6, 2021
@danielaparker
Copy link
Owner

danielaparker commented Apr 6, 2021

Thanks for reporting. I believe this is related to #314 currently being tested on master. We should have a maintenance release that addresses this issue out soon.

We'll look into improving our coverage for memory leaks, we lost some when travis was recently acquired and became unfriendly to open source.

@andysCaplin
Copy link
Author

Cheers for prompt response. I'll try that out when it's available and add a comment to this issue.

@danielaparker
Copy link
Owner

Should be okay on master, would you be able to check master before we release this?

Thanks,
Daniel

@danielaparker
Copy link
Owner

Our application is C code and the only issue I've had previously with leaks is that I needed to explicitly set memory allocated by the library to NULL to free it. i.e. when I call an API like this

{
    jsoncons::json        selected;

    selected = jsoncons::jsonpath::json_query(jsonob->json, pparams->jsonpath);
...
    selected = NULL;
}

The selected variable didn't seem to be freed when it went out of scope. I had to have the line setting it to NULL to make sure it was freed.

I don't understand how

...
selected = NULL;
}

would make a difference. I think that's the same as assigning a 0, i.e.

selected = 0;

which would result in a call to the internal function Destroy_() to release memory, before initializing to 0.

But the jsoncons::json destructor also calls Destroy_(), and it should be invoked as it goes out of scope.

@andysCaplin
Copy link
Author

Hi - yes I'll test this today and get back to you.
Also, I'll re-test if I still need the NULL assignment.

@andysCaplin
Copy link
Author

Just tested 0.163.1 and the leaks I'd seen are gone now.

We also test for invalid accesses and un-initialised variable accesses and there are none of those either.

Cheers for the fix.

Re the NULL assignment - that's my mistake. The example I gave isn't necessary.

I have a C structure that I calloc and that has some jsoncons::jsons in it.
Before I free that C structure I assign the jsoncons::jsons in the structure to NULL or they leak - but from your description it makes sense that that would free them.
When I saw the leak from that structure initially I think I just started assigning all jsoncons::jsons to NULL when I was finished with them but obviously for the locals I don't need to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants