[Bug] Out-of-bounds writes in `BitView::_Read64<true>` potentially causing crashes or infinite loops

[chiayy-com]

In the code (In the BitView::_Read64 method withCheckAlignment==true)

https://github.com/Chia-Network/bladebit/blob/c669876eff68a29c679e3ff501a3ab656adf30b9/src/util/BitView.h#L156-L163 https://github.com/Chia-Network/bladebit/blob/c669876eff68a29c679e3ff501a3ab656adf30b9/src/util/BitView.h#L186-L192

const size_t totalBytes     = CDiv( sizeBits, 8 );
const int32  remainderBytes = (int32)( totalBytes - fieldIndex * 8 );

field = 0;
byte* fieldBytes = (byte*)&field;
for( int32 i = 0; i < remainderBytes; i++ )
    fieldBytes[i] = pField[i];

The calculation of remainderBytes is wrong. Which causes the value of remainderBytes to exceeds 8 potentially. So fieldBytes[i] may reference to some dangers zone, resulting in undefined behavior.

FYI: An example I encountered is that remainderBytes==12, and fieldBytes[8] happens to references to i, causing infinite loop. (built on Windows, debug mode)

[harold-b]

const size_t totalBytes     = CDiv( sizeBits, 8 );
const int32  remainderBytes = (int32)( totalBytes - fieldIndex * 8 );

I have not gone through this code in a while, but this seems to be correct. It would never exceed 8 bytes if the correct sizeBits and position was passed. Perhaps add an ASSERT( position < sizeBits ); wherever you're using this. Or if there is a bug, it could lie elsewhere, unless I'm missing something here.

[chiayy-com]

https://github.com/Chia-Network/bladebit/blob/c669876eff68a29c679e3ff501a3ab656adf30b9/src/util/BitView.h#L127C1-L169

Consider evaluating _Read64<true>(bitCount, someAddressNonAligned, 0, 96);

You start with

sizeBits = 96;
position = 0;

so you get

totalBytes = ceil(96 / 8) = 12;
fieldIndex = floor(0 / 64) = 0;
isLastField = (0 == floor(96 / 64) - 1) = true;

remainderBytes = 12 - 0 * 8 = 12

I encountered this bug when I tried to fetch proof from a plot created with some old plotter, which is valid used with chiapos, but causes bladebit to crash.

[harold-b]

OK, so it seems the bug would be in isLastField (no - 1 necessary), not in the remainderBytes calculation. If you can modify the PR to reflect that then perhaps we can review to ensure everything else is also correct.

[chiayy-com]

OK. I'd like to do the modification.

BTW, would you like it if I also simplify these lines this time? https://github.com/Chia-Network/bladebit/blob/c669876eff68a29c679e3ff501a3ab656adf30b9/src/util/BitView.h#L150-L153

from

if( isPtrAligned && !isLastField )
    field = *((uint64*)pField);
else if( !isLastField )
    memcpy( &field, pField, sizeof( uint64 ) );

to

if( !isLastField )
    field = *((uint64*)pField);

This original optimization seems unnecessary. A 64-bit dereference is a single instruction under most architectures (unless not 64-bit). Though it'll be a slower instruction if the address is unaligned, there's no reason a memset could do it faster.

AFAIK, a memset is typically a byte-by-byte copy loop for <=8 data, possibly optimized using some specialized instruction set, but still won't over-perform a simple read. Feel free to correct me if I missed something.

[harold-b]

Yes, please go ahead. At the time that was written I had incorrectly assumed it was invalid to read/write unaligned in arm64 as it was in the 32bit instruction set.

[jmhands]

@harold-b if this PR happened we can close this issue