Skip to content

Commit

Permalink
BUG: fixed regression in gdcmImageCodec.cxx
Browse files Browse the repository at this point in the history
S. https://sourceforge.net/p/gdcm/bugs/554/
An unexpected behavior was introduced in commit 4ecd77a,
causing pixel values being all greater or equal to zero on DICOM images where negative values are expected.
When trying to solve the following cppcheck issues:
shiftNegativeLHS,GDCM/Source/MediaStorageAndFileFormat/gdcmImageCodec.cxx:465,portability,Shifting a negative value is technically undefined behaviour
shiftNegativeLHS,GDCM/Source/MediaStorageAndFileFormat/gdcmImageCodec.cxx:521,portability,Shifting a negative value is technically undefined behaviour
The changes in gdcmImageCodec.cxx:465 and gdcmImageCodec.cxx:521 implies a potential difference in the sign of nmask,
depending on the specific values of PF.GetBitsAllocated() and PF.GetBitsStored(). If the right shift operation in the
modified version produces a non-negative value, the result will be positive, whereas the result in the non modified
version will always be negative due to the initial assignment of 0x8000 as a signed integer.
  • Loading branch information
issakomi committed Jan 5, 2024
1 parent d31fb17 commit 02f1c75
Showing 1 changed file with 4 additions and 2 deletions.
6 changes: 4 additions & 2 deletions Source/MediaStorageAndFileFormat/gdcmImageCodec.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,8 @@ bool ImageCodec::CleanupUnusedBits(char * data8, size_t datalen)
smask = (uint16_t)(
smask << ( 16 - (PF.GetBitsAllocated() - PF.GetBitsStored() + 1) ));
// nmask : to propagate sign bit on negative values
int16_t nmask = (int16_t)(0x8000U >> ( PF.GetBitsAllocated() - PF.GetBitsStored() - 1 ));
int16_t nmask = (int16_t)0x8000;
nmask = (int16_t)(nmask >> ( PF.GetBitsAllocated() - PF.GetBitsStored() - 1 ));

uint16_t *start = (uint16_t*)data;
for( uint16_t *p = start ; p != start + datalen / 2; ++p )
Expand Down Expand Up @@ -515,7 +516,8 @@ bool ImageCodec::DoOverlayCleanup(std::istream &is, std::ostream &os)
smask = (uint16_t)(
smask << ( 16 - (PF.GetBitsAllocated() - PF.GetBitsStored() + 1) ));
// nmask : to propagate sign bit on negative values
int16_t nmask = (int16_t)(0x8000U >> ( PF.GetBitsAllocated() - PF.GetBitsStored() - 1 ));
int16_t nmask = (int16_t)0x8000;
nmask = (int16_t)(nmask >> ( PF.GetBitsAllocated() - PF.GetBitsStored() - 1 ));

uint16_t c;
while( is.read((char*)&c,2) )
Expand Down

0 comments on commit 02f1c75

Please sign in to comment.