Hi! We've found a bug in the Foundation API Source Code. A crash happens on OSX systems when run with the libGMalloc tool with this callstack when opening the pump.e57 example (or any other e57 file which has an Integer node):
- e57::BitpackIntegerDecoder<unsigned int="">::inputProcessAligned(char const*, unsigned long, unsigned long) + 1221 (E57FoundationImpl.cpp:8455)
- e57::BitpackDecoder::inputProcess(char const*, unsigned long) + 660 (E57FoundationImpl.cpp:7383)
- e57::CompressedVectorReaderImpl::feedPacketToDecoders(unsigned long long) + 6173 (E57FoundationImpl.cpp:6496)
- e57::CompressedVectorReaderImpl::read() + 620 (E57FoundationImpl.cpp:6401)
- e57::CompressedVectorReader::read() + 29 (E57Foundation.cpp:2726)
.... calling read ....
</unsigned>
Concerned source context (lines 8447-8466):
size_t bitOffset = firstBit;
for (size_t i = 0; i < recordCount; i++) {
/// Get lower word (contains at least the LSbit of the value),
RegisterT low = inp[wordPosition];
SWAB(&low); // swab if necessary
/// Get upper word (may or may not contain interesting bits),
RegisterT high = inp[wordPosition+1]; //<<<<<<<<<<< crash here
SWAB(&high); // swab if necessary
RegisterT w;
if (bitOffset > 0) {
/// Shift high to just above the lower bits, shift low LSBit to bit0, OR together.
/// Note shifts are logical (not arithmetic) because using unsigned variables.
w = (high << (8*sizeof(RegisterT) - bitOffset)) | (low >> bitOffset);
} else {
/// The left shift (used above) is not defined if shift is >= size of word
w = low;
}
//....
Long story short: it is not needed to read the wordPosition+1 byte from inp in every iteration. It is only needed if it is sure that we have to fill the missing bits of a value from the next byte. This is the case only if the bitOffset > 0 AND (bitOffset + bitsPerRecord_ > 8*sizeof(RegisterT)). This means that we need to read higher bits only if the lower bits are shifted enough to have fewer bits left after the shift than bitsPerRecord.
So my fix is that I moved reading the high bits into the if section, stricted the condition, and inserted an other else if statement to shift low bits if bitOffset > 0, but high bits are not needed.
Proposed fix (lines 8447-8471):
size_t bitOffset = firstBit;
for (size_t i = 0; i < recordCount; i++) {
/// Get lower word (contains at least the LSbit of the value),
RegisterT low = inp[wordPosition];
SWAB(&low); // swab if necessary
RegisterT w;
if (bitOffset > 0 && (bitOffset + bitsPerRecord_ > 8*sizeof(RegisterT))) {
/// Don't have enough bits in low to get value -> need high word
/// Get upper word (may or may not contain interesting bits),
RegisterT high = inp[wordPosition+1];
SWAB(&high); // swab if necessary
/// Shift high to just above the lower bits, shift low LSBit to bit0, OR together.
/// Note shifts are logical (not arithmetic) because using unsigned variables.
w = (high << (8*sizeof(RegisterT) - bitOffset)) | (low >> bitOffset);
} else if (bitOffset > 0) {
/// Shift low LSBit to bit0.
/// Note shift is logical (not arithmetic) because using unsigned variables.
w = (low >> bitOffset);
} else {
/// The left shift (used above) is not defined if shift is >= size of word
w = low;
}
Hi, Zoli
