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 in cmark_consolidate_text_nodes #32

Closed
elibarzilay opened this issue May 4, 2015 · 2 comments
Closed

Memory leak in cmark_consolidate_text_nodes #32

elibarzilay opened this issue May 4, 2015 · 2 comments

Comments

@elibarzilay
Copy link
Contributor

AFAICT, the call it does a call to cmark_node_set_literal copies the string from buf, but that string is never copied. It's easy to verify the leak with running it in a loop and looking at the memory (which is how I caught it) then adding a free for the detached string and the leak is gone.

[I'd submit it as a PR, but it seems silly to put stuff in an allocated string, then copying it and freeing the first. A more proper solution would be some way to signal a request to not copy the text (which cmark_node_set_literal could get as a new argument which will be passed on to cmark_chunk_set_cstr) but that's an API change, so I'm just reporting it...]

jgm added a commit that referenced this issue May 4, 2015
@jgm jgm closed this as completed in 2cbb135 May 4, 2015
@jgm
Copy link
Member

jgm commented May 4, 2015

This can also be revealed by

valgrind --leak-check=full --dysmutil=yes \
  build/src/cmark --normalize README.md

I have improved 'make leakcheck' to detect this case.

Could you say more about your "more proper solution"? I
didn't quite understand. I just pushed a fix that avoids
allocating the extra string, but you may have an even better
idea.

@elibarzilay
Copy link
Contributor Author

Well, what I tried was adding a nocopy argument to cmark_node_set_literal, and that leads to a new argument to cmark_chunk_set_cstr. It's not great, especially since it adds an argument to a public function -- but maybe it makes sense to have it there. (And since I don't know the answer I left it as a comment only.)

I'm not sure about it, but it looks like after your fix it still double-copies the string (allocates the string buffer, then allocates a copy, then frees the first copy).

jgm added a commit that referenced this issue May 6, 2015
This improves on #32, I think.

@elibarzilay, does this look better?  We now avoid the allocations
associated with cmark_get_literal, and copy directly from
the chunk to the buffer.
PKRoma pushed a commit to PKRoma/cmark that referenced this issue Aug 19, 2018
Also clean up CMakeLists considerably.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants