Menu

#554 Regression in gdcmImageCodec.cxx introduced in 3.0.22

3.1
open
nobody
None
5
2024-01-09
2024-01-04
No

An unexpected behavior was introduced in commit https://github.com/malaterre/GDCM/commit/4ecd77a6bbccd32eaa939d0b06d441ec7a07da6e#, 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.

Discussion

  • Mihail Isakov

    Mihail Isakov - 2024-01-06

    I have reproduces the change. I have done a (draft) PR https://github.com/malaterre/GDCM/pull/165

    Do you have a suitable DICOM file?

    #include <iostream>
    #include <cstdint>
    #include <vector>
    
    int main()
    {
      const std::vector<unsigned short> a{ 7, 6, 5, 4, 3, 2, 1, 0 };
    
      std::cout << "Old:" << '\n';
      for (size_t x = 0; x < a.size(); ++x)
      {
        //////////////////////////////////////////////
        //
        //
        int16_t nmask = (int16_t)0x8000;
        nmask = (int16_t)(nmask >> a.at(x));
        //
        //
        //////////////////////////////////////////////
        std::cout << nmask << '\n';
      }
    
      std::cout << "New:" << '\n';
      for (size_t x = 0; x < a.size(); ++x)
      {
        //////////////////////////////////////////////
        //
        //
        int16_t nmask = (int16_t)(0x8000U >> a.at(x));
        //
        //
        //////////////////////////////////////////////
        std::cout << nmask << '\n';
      }
    
      return 0;
    }
    

    Old:
    -256
    -512
    -1024
    -2048
    -4096
    -8192
    -16384
    -32768
    New:
    256
    512
    1024
    2048
    4096
    8192
    16384
    -32768

     
  • Manuel Urvoy

    Manuel Urvoy - 2024-01-09

    Hello,

    thank you for the investigation and for the PR.
    Unfortunately I cannot share the DICOM images that present the issue.
    @malat Do you plan to release a new GDCM version to revolve the issue soon?

    Thank you

     

    Last edit: Manuel Urvoy 2024-01-09
    • Mathieu Malaterre

      Next release is planned for this week. Thanks

       
      👍
      1

Log in to post a comment.