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

power2gt incorrect for non 32bit integers #27

Closed
FSMaxB opened this issue Sep 27, 2016 · 9 comments
Closed

power2gt incorrect for non 32bit integers #27

FSMaxB opened this issue Sep 27, 2016 · 9 comments
Milestone

Comments

@FSMaxB
Copy link
Collaborator

FSMaxB commented Sep 27, 2016

pow2gt calculates the next biggest power of 2 for a given x. But it only does it for integers up to sizes of 32 bit, 64 bit Integers are not supported.

I'm not sure, but it might even introduce undefined behavior for 16 bit integers. I'll have to do more research into this.

One possible solution would be to write a loop that does everything during runtime and relying on the compiler to be smart enough to optimise it properly.

@DaveGamble
Copy link
Owner

DaveGamble commented Sep 27, 2016

Do you mean the case where sizeof(int)!=4 ?
In the 16bit case, the buffered output length will be limited to 32k; unprebuffered output ought to work fine, but that's a bigger issue, and concerns the usage of int throughout the code, and isn't limited to just pow2gt. So far, no reports of issues with this from embedded users.

In the case of sizeof(int)>4, it means we have a maximal output size of ~2GB which needn't apply. However, I'm not totally convinced this is a major issue for a quick lightweight json library.

I'd like to close this issue and reopen it on first report of it causing a real-world issue.

@FSMaxB
Copy link
Collaborator Author

FSMaxB commented Sep 27, 2016

Is it still guaranteed for 64 bit integers bigger than 2^32 that the output of pow2gt is at least as big as it's input (I can't do the bit fiddling in my head right now)?

If not, there is a heap buffer overflow in the memcpy in ensure.

@FSMaxB
Copy link
Collaborator Author

FSMaxB commented Sep 27, 2016

And for systems with 16 bit integers this causes undefined behavior (right shifting by 16).

@DaveGamble
Copy link
Owner

Is it still guaranteed for 64 bit integers bigger than 2^32 that the output of pow2gt is at least as big as it's input (I can't do the bit fiddling in my head right now)?

  • Yes.

And for systems with 16 bit integers this causes undefined behavior (right shifting by 16).

  • Compiler will emit a warning, which should abort compile.

@FSMaxB
Copy link
Collaborator Author

FSMaxB commented Sep 27, 2016

What about something like this:

x |= x >> 1;
x |= x >> 2;
x |= x >> 4;
x |= x >> 8;
#if INT_MAX > 65536UL
x |= x >> 16;
#endif
#if INT_MAX > 4294967296UL
x |= x >> 32;
#endif

@DaveGamble
Copy link
Owner

I need to check whether INT_MAX gets defined as 32767 rather than 65536, but in principle, something like that, yes... but moreover I'll have a trace through the code for anywhere else that might fail.

Um.. at the risk of asking an off-topic question, could I ask why it is that you're prepared to invest so much time in maintaining cJSON? In practical terms, it's inferior in every metric to RapidJSON...?

@FSMaxB
Copy link
Collaborator Author

FSMaxB commented Sep 28, 2016

RapidJSON didn't exist yet when I started using cJSON and RapidJSON is written in C++, not C.

@FSMaxB
Copy link
Collaborator Author

FSMaxB commented Nov 7, 2016

It should work like this.

@FSMaxB FSMaxB added this to the 1.0.0 milestone Nov 7, 2016
@DaveGamble
Copy link
Owner

Awesome :D Good work! :D 👍

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

No branches or pull requests

2 participants