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

Possible int overflow in encoding.h when compiling on a 32-bit compiler #30

Closed
mmastrac opened this issue Jun 28, 2018 · 4 comments
Closed

Comments

@mmastrac
Copy link

When compiling w/-Wconversion:

Error: Implicit conversion loses integer precision: 'unsigned long long' to 'bm::word_t' (aka 'unsigned int')

As the type of bm::word_t is unsigned int (ie: 32-bits on a 32-bit platform), this function could silently truncate the result of the data. I don't actually use this code so I don't have a test-case available to prove it, but it looks like a reasonable warning.

{
#if (BM_UNALIGNED_ACCESS_OK == 1)
	bm::id64_t a = *((bm::id64_t*)buf_);
#else
	bm::word_t a = buf_[0]+
                   ((bm::id64_t)buf_[1] << 8)  +
                   ((bm::id64_t)buf_[2] << 16) +
                   ((bm::id64_t)buf_[3] << 24) +
                   ((bm::id64_t)buf_[4] << 32) +
                   ((bm::id64_t)buf_[5] << 40) +
                   ((bm::id64_t)buf_[6] << 48) +
                   ((bm::id64_t)buf_[7] << 56);
#endif
    buf_+=sizeof(a);
    return a;
}
@tlk00
Copy link
Owner

tlk00 commented Jun 29, 2018

Agreed, it is totally reasonable even in 64-bit compile.
It's a legit bug. The code is a victim of Intel unaligned access OK mode.

What is the platform you use. If you happen to be on x86, please help me to identify why
BM_UNALIGNED_ACCESS_OK was not set there. (Must be some combination of OS/compiler prevented it).

Thank you for reporting it!

@mmastrac
Copy link
Author

No problem. Currently building on iOS (arm32/arm64) which doesn't support unaligned access.

@tlk00
Copy link
Owner

tlk00 commented Jun 30, 2018

I think i fixed the issue in the master.

@mattfs
Copy link

mattfs commented Jul 2, 2018

Confirmed fixed!

@tlk00 tlk00 closed this as completed Jul 2, 2018
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

3 participants