tlk00 / BitMagic

BitMagic Library
http://bitmagic.io
Other
412 stars 48 forks source link

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

Closed mmastrac closed 6 years ago

mmastrac commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

tlk00 commented 6 years ago

I think i fixed the issue in the master.

mattfs commented 6 years ago

Confirmed fixed!