From 6ba9cd1541f4faa748e0fa5b24a7caba673b86ef Mon Sep 17 00:00:00 2001 From: Nigel Stewart Date: Fri, 3 Dec 2021 17:07:32 +1000 Subject: [PATCH 1/3] fixup: BitpackIntegerDecoder can overrun the input buffer if the last record finishes on the last word --- src/Decoder.cpp | 25 ++++++++++++++++--------- src/Decoder.h | 1 + 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/Decoder.cpp b/src/Decoder.cpp index 7824f60..6978d5a 100644 --- a/src/Decoder.cpp +++ b/src/Decoder.cpp @@ -645,7 +645,8 @@ BitpackIntegerDecoder::BitpackIntegerDecoder( bool isScaledInteger, u SourceDestBuffer &dbuf, int64_t minimum, int64_t maximum, double scale, double offset, uint64_t maxRecordCount ) : BitpackDecoder( bytestreamNumber, dbuf, sizeof( RegisterT ), maxRecordCount ), - isScaledInteger_( isScaledInteger ), minimum_( minimum ), maximum_( maximum ), scale_( scale ), offset_( offset ) + isScaledInteger_( isScaledInteger ), minimum_( minimum ), maximum_( maximum ), scale_( scale ), offset_( offset ), + registerBits_( sizeof(RegisterT) * 8) { /// Get pointer to parent ImageFileImpl ImageFileImplSharedPtr imf( dbuf.impl()->destImageFile() ); //??? should be function for this, @@ -731,7 +732,19 @@ size_t BitpackIntegerDecoder::inputProcessAligned( const char *inbuf, #endif RegisterT w; - if ( bitOffset > 0 ) + if ( bitOffset == 0 ) + { + /// The left shift (used below) is not defined if shift is >= size of + /// word + w = low; + } + // Avoid reading the next word, unless it is needed + // If the last record finishes on the last bit of input, avoid UMR + else if ( bitOffset + bitsPerRecord_ <= registerBits_) + { + w = low >> bitOffset; + } + else { /// Get upper word (may or may not contain interesting bits), RegisterT high = inp[wordPosition + 1]; @@ -743,13 +756,7 @@ size_t BitpackIntegerDecoder::inputProcessAligned( const char *inbuf, /// 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; + w = ( high << ( registerBits_ - bitOffset ) ) | ( low >> bitOffset ); } #ifdef E57_MAX_VERBOSE diff --git a/src/Decoder.h b/src/Decoder.h index 7d9496c..561ebb1 100644 --- a/src/Decoder.h +++ b/src/Decoder.h @@ -149,6 +149,7 @@ namespace e57 double offset_; unsigned bitsPerRecord_; RegisterT destBitMask_; + const size_t registerBits_; }; class ConstantIntegerDecoder : public Decoder From cfb6b48605e36895c70d20c0bf128e42598ba7c7 Mon Sep 17 00:00:00 2001 From: Nigel Stewart Date: Fri, 10 Dec 2021 17:49:45 +1000 Subject: [PATCH 2/3] BitpackIntegerDecoder static initialiser --- src/Decoder.cpp | 3 +-- src/Decoder.h | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Decoder.cpp b/src/Decoder.cpp index 6978d5a..af0ef5b 100644 --- a/src/Decoder.cpp +++ b/src/Decoder.cpp @@ -645,8 +645,7 @@ BitpackIntegerDecoder::BitpackIntegerDecoder( bool isScaledInteger, u SourceDestBuffer &dbuf, int64_t minimum, int64_t maximum, double scale, double offset, uint64_t maxRecordCount ) : BitpackDecoder( bytestreamNumber, dbuf, sizeof( RegisterT ), maxRecordCount ), - isScaledInteger_( isScaledInteger ), minimum_( minimum ), maximum_( maximum ), scale_( scale ), offset_( offset ), - registerBits_( sizeof(RegisterT) * 8) + isScaledInteger_( isScaledInteger ), minimum_( minimum ), maximum_( maximum ), scale_( scale ), offset_( offset ) { /// Get pointer to parent ImageFileImpl ImageFileImplSharedPtr imf( dbuf.impl()->destImageFile() ); //??? should be function for this, diff --git a/src/Decoder.h b/src/Decoder.h index 561ebb1..716a2a9 100644 --- a/src/Decoder.h +++ b/src/Decoder.h @@ -149,7 +149,7 @@ namespace e57 double offset_; unsigned bitsPerRecord_; RegisterT destBitMask_; - const size_t registerBits_; + static const size_t registerBits_ = sizeof(RegisterT) * 8; }; class ConstantIntegerDecoder : public Decoder From c5d463db2ec54432635fa5ce0ac51fa546efd0a9 Mon Sep 17 00:00:00 2001 From: Nigel Stewart Date: Sat, 11 Dec 2021 08:24:47 +1000 Subject: [PATCH 3/3] constexpr size_t RegisterBits for BitpackIntegerDecoder --- src/Decoder.cpp | 4 ++-- src/Decoder.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Decoder.cpp b/src/Decoder.cpp index af0ef5b..c8bdb3a 100644 --- a/src/Decoder.cpp +++ b/src/Decoder.cpp @@ -739,7 +739,7 @@ size_t BitpackIntegerDecoder::inputProcessAligned( const char *inbuf, } // Avoid reading the next word, unless it is needed // If the last record finishes on the last bit of input, avoid UMR - else if ( bitOffset + bitsPerRecord_ <= registerBits_) + else if ( bitOffset + bitsPerRecord_ <= RegisterBits ) { w = low >> bitOffset; } @@ -755,7 +755,7 @@ size_t BitpackIntegerDecoder::inputProcessAligned( const char *inbuf, /// 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 << ( registerBits_ - bitOffset ) ) | ( low >> bitOffset ); + w = ( high << ( RegisterBits - bitOffset ) ) | ( low >> bitOffset ); } #ifdef E57_MAX_VERBOSE diff --git a/src/Decoder.h b/src/Decoder.h index 716a2a9..f602f54 100644 --- a/src/Decoder.h +++ b/src/Decoder.h @@ -149,7 +149,7 @@ namespace e57 double offset_; unsigned bitsPerRecord_; RegisterT destBitMask_; - static const size_t registerBits_ = sizeof(RegisterT) * 8; + static constexpr size_t RegisterBits = sizeof( RegisterT ) * 8; }; class ConstantIntegerDecoder : public Decoder