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

Working #144

Closed
wants to merge 4 commits into from
Closed

Working #144

wants to merge 4 commits into from

Conversation

sdasgup3
Copy link

@sdasgup3 sdasgup3 commented Oct 17, 2018

  • Fixed a memory leak in jsondump
  • Added an example to access high level structures

@pt300
Copy link
Collaborator

pt300 commented Oct 17, 2018

Four things for now:

  1. You're freeing variables just before the program exits. What's the point in that? You're not fixing any memory leak.
  2. You aren't supposed to cast void pointers.
  3. I kind of don't see the point of your example.
  4. Your example is vulnerable to the same out of bounds read as jsondump. See heap buffer overflow on some inputs #125

@sdasgup3
Copy link
Author

  1. I am using the code as sub-part in another project.
  2. Agreed.
  3. Unlike other json library which provides API's to access Json objects, jsmn has a minimalist approach. I wrote some simple access-or functions like get_primitive to be combined to extract higher structure. I
    thought it might be useful to be in the examples directory. I can work out a better example.

@pt300
Copy link
Collaborator

pt300 commented Oct 30, 2018

I'm not sure if you were trying to answer each of my point separately but anyway.
Looking again at your example I think I kind of see what you are trying to do. I'm not really sure if that code really helps but these type checks could point people into right direction regarding input validation. At this point I still have mixed feelings about it.
Regarding the code itself, beside the mistakes I already mentioned earlier (pointer casting, out of bounds read), your code has really erratic indentation which makes it hard to read.
I'm not a maintainer but in it's current form I don't see this pull request getting accepted.

@pt300 pt300 closed this May 12, 2019
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

Successfully merging this pull request may close these issues.

2 participants