Navigation Menu

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

if end pointer non null, assumes end of string #247

Closed
wants to merge 1 commit into from
Closed

if end pointer non null, assumes end of string #247

wants to merge 1 commit into from

Conversation

TomzBench
Copy link

Hello,

This modifies parse with opts so that if the return end pointer is pointing than that is the end of the string so as to not use strlen. Was not sure of other method to parse something that is not a string. I added small test case.

Could not find a method to parse "memory" as opposed to parse string.

@FSMaxB
Copy link
Collaborator

FSMaxB commented Feb 28, 2018

It is unfortunate that cJSON only supports zero terminated strings at the moment.

Using return_parse_end is not an option however, because this would break existing code and might introduce security issues if whatever return_parse_end points to has an unknown number in it (it should probably still work with zero terminated strings, because it is an invalid token, but I am not confident enough to actually do this).

I am currently working on #186 which provides configuration using the builder pattern and also takes a length for the string. I can't give a timeline on when it will be finished though. Could be 1 month, could be half a year. Some feedback would be good though.

I'm curious what your usecase is though. Do you just want to avoid the overhead of the strlen or do you actually have strings that aren't zero terminated?

@FSMaxB FSMaxB closed this Feb 28, 2018
@TomzBench
Copy link
Author

Hi - Yes I was sure this would break existing as well but just thought i'd submit PR anyway.

My use case is I definitely have memory that is not null terminated. In most places in my projects I am very explicit with lengths. Not so much for performance of strlen but because of safety. I used an older version of this library and I thought it supported parsing memory - when recently update this seemed to break for me.

@FSMaxB
Copy link
Collaborator

FSMaxB commented Feb 28, 2018

No version of cJSON ever supported parsing non zero-terminated strings.

The only thing that changed is that there was no strlen in the past. This means that if you were parsing valid JSON, cJSON would just stop parsing and everything was fine, so it did work in theory. But if the string didn't contain a complete JSON, it would do a buffer overrun and read whatever is in memory after that, which isn't a good thing.

@TomzBench
Copy link
Author

I see -

If PR included a compiler flag (disabled by default as to not break existing) could that be supported here? I would think parsing non null terminated strings would be important for text based protocols that do not have NULL chars in their payloads.

@FSMaxB
Copy link
Collaborator

FSMaxB commented Feb 28, 2018

No, I want to bring support for that along with #186. You'll have to wait until this is finished.

@FSMaxB
Copy link
Collaborator

FSMaxB commented Feb 28, 2018

Since your modification isn't too big, it's feasible to maintain it out of tree for the time being.

@TomzBench
Copy link
Author

Yes that's fine too.

Thanks,

@FSMaxB
Copy link
Collaborator

FSMaxB commented Feb 28, 2018

About text based protocols: As long as there is a NULL character somewhere, even if it is way past the end of the JSON, cJSON can still parse it, because there is no problem with having non-JSON data behind the actual JSON.

@TomzBench
Copy link
Author

The strlen tends to spit out valgrind errors even if the parse works OK. And if caller has the length the strlen is redundant. To correctly parse with out valgrind errors you have to mem copy into a bufferlen+1 and manually add the '\0'.

@FSMaxB
Copy link
Collaborator

FSMaxB commented Feb 28, 2018

Yes, of course. What I'm saying is that if you have a text based protocol that contains a JSON and the string that contains the JSON is zero terminated, it will work fine. Meaning that the zero terminator isn't required to be directly after the JSON.

@TomzBench
Copy link
Author

Yes - by text based protocols I mean the json might end like this:

"HTTP/1.1 /index.html\r\n"
"Content-Length:whocares\r\n"
"\r\n"
"{"hello":"world"}\r\n\r\n"

@TomzBench
Copy link
Author

And then the strlen will run passed the buffer and cause valgrind errors...

@FSMaxB
Copy link
Collaborator

FSMaxB commented Feb 28, 2018

Yes, because it is not zero terminated. There needs to be a zero terminator at the moment, but it doesn't have to be directly after the JSON.

@TomzBench
Copy link
Author

Yes I understand - I just mean text based protocols are not NULL terminated. Many protocols like ZMQ, MQTT and other light weight protocols that target constrained environments prepend the packet's with the length and are not null terminated.

@TomzBench
Copy link
Author

So no where in the buffer is NULL terminated but "length prepended". And my point is that I think this is very common. And I mentioned cause you asked for feedback! I do not mind maintaining my small diff while you work on the options object but just thought you should know about non null terminated use cases.

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.

None yet

2 participants