LibRaw 0.18.5 (update: was 0.18.3 and 0.184)

LibRaw 0.18.5 released and available on both download page and on Github repository.

This is bugfix release, changes are (compared to 0.18.2):

  • Fix for possible buffer overrun in kodak_65000 decoder
  • Fix for possible heap overrun in Canon makernotes parser
  • Fix for CVE-2017-13735
  • CVE-2017-14265: Additional check for X-Trans CFA pattern data

Comments

compiler warning

Hi, I'm getting the following warning from gcc-6.4.0 in libraw_cpp.cxx:

* /tmp/portage/portage/media-libs/libraw-0.18.5/work/LibRaw-0.18.5/src/libraw_cxx.cpp:1784:64: warning: iteration 6 invokes undefined behavior [-Waggressive-loop-optimizations]

CFLAGS="-O2 -march=native -ftree-vectorize -mprefer-avx128 -mvzeroupper -fstack-protector -ftree-loop-distribution -ftree-loop-distribute-patterns -pipe"

Code:

static inline void unpack28bytesto16x16ns(unsigned char *src, unsigned short *dest)
{
dest[0] = (src[3] << 6) | (src[2] >> 2);
dest[1] = ((src[2] & 0x3) << 12) | (src[1] << 4) | (src[0] >> 4);
dest[2] = (src[0] & 0xf) << 10 | (src[7] << 2) | (src[6] >> 6);
dest[3] = ((src[6] & 0x3f) << 8) | src[5];
dest[4] = (src[4] << 6) | (src[11] >> 2);
dest[5] = ((src[11] & 0x3) << 12) | (src[10] << 4) | (src[9] >> 4);
dest[6] = (src[9] & 0xf) << 10 | (src[8] << 2) | (src[15] >> 6);
dest[7] = ((src[15] & 0x3f) << 8) | src[14];
dest[8] = (src[13] << 6) | (src[12] >> 2);
dest[9] = ((src[12] & 0x3) << 12) | (src[19] << 4) | (src[18] >> 4);
dest[10] = (src[18] & 0xf) << 10 | (src[17] << 2) | (src[16] >> 6);
dest[11] = ((src[16] & 0x3f) << 8) | src[23]; ## line 1784
dest[12] = (src[22] << 6) | (src[21] >> 2);
dest[13] = ((src[21] & 0x3) << 12) | (src[20] << 4) | (src[27] >> 4);
dest[14] = (src[27] & 0xf) << 10 | (src[26] << 2) | (src[25] >> 6);
dest[15] = ((src[25] & 0x3f) << 8) | src[24];
}

gcc line numbering

gcc preprocessor does not count blank and comment lines, and I gave the wrong code section. sorry.

I pulled the rest of the warnings from the build log:

/tmp/portage/portage/media-libs/libraw-0.18.5/work/LibRaw-0.18.5/src/libraw_cxx.cpp: In member function ‘virtual int LibRaw::open_datastream(LibRaw_abstract_datastream*)’:

/tmp/portage/portage/media-libs/libraw-0.18.5/work/LibRaw-0.18.5/src/libraw_cxx.cpp:1784:64: warning: iteration 6 invokes undefined behavior [-Waggressive-loop-optimizations]

imgdata.idata.xtrans[0]

 = imgdata.idata.xtrans_abs[0][c];
                                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
 
 
/tmp/portage/portage/media-libs/libraw-0.18.5/work/LibRaw-0.18.5/src/libraw_cxx.cpp:1783:22: note: within this loop
 
     for(int c = 0; c < 36; c++)
                    ~~^~~~
 
So it appears the actual code section begins at line 1999:
 
 
    // XTrans Compressed?
    if (!imgdata.idata.dng_version && !strcasecmp(imgdata.idata.make, "Fujifilm") &&
        (load_raw == &LibRaw::unpacked_load_raw))
    {
      if (imgdata.sizes.raw_width * imgdata.sizes.raw_height * 2 != libraw_internal_data.unpacker_data.data_size)
      {
        if (imgdata.sizes.raw_width * imgdata.sizes.raw_height * 7 / 4 == libraw_internal_data.unpacker_data.data_size)
          load_raw = &LibRaw::fuji_14bit_load_raw;
        else
          parse_fuji_compressed_header();
      }
      if (imgdata.idata.filters == 9)
      {
        // Adjust top/left margins for X-Trans
        int newtm = imgdata.sizes.top_margin % 6 ? (imgdata.sizes.top_margin / 6 + 1) * 6 : imgdata.sizes.top_margin;
        int newlm = imgdata.sizes.left_margin % 6 ? (imgdata.sizes.left_margin / 6 + 1) * 6 : imgdata.sizes.left_margin;
        if (newtm != imgdata.sizes.top_margin || newlm != imgdata.sizes.left_margin)
        {
          imgdata.sizes.height -= (newtm - imgdata.sizes.top_margin);
          imgdata.sizes.top_margin = newtm;
          imgdata.sizes.width -= (newlm - imgdata.sizes.left_margin);
          imgdata.sizes.left_margin = newlm;
          for (int c1 = 0; c1 < 6; c1++)
            for (int c2 = 0; c2 < 6; c2++)
              imgdata.idata.xtrans[c1][c2] = imgdata.idata.xtrans_abs[c1][c2];
        }
      }
}
 
 
gcc optimizes the nested 6x6 loop to:   for(int c = 0; c < 36; c++)
 
on the 6th iteration of that loop the result of  imgdata.idata.xtrans[c] = imgdata.idata.xtrans_abs[c];  
is undefined. 

what is wrong with

what is wrong with

for(int c = 0; c < 36; c++) 

loop if both xtrans and xtrans_abs are [6][6] arrays, so xtrans[0][35] is equal to xtrans[5][5]?

-- Alex Tutubalin

the loop is ok

That is a very common optimization and is not likely to be the issue. The problem is usually found in the expression itself, commonly either the right-side array element is undefined or the result overflows the variable.

On the 6th iteration of that loop the result of this expression is undefined:

imgdata.idata.xtrans

 = imgdata.idata.xtrans_abs[c];  

C++ standard defines arrays

C++ standard defines arrays as dense arrays, no spacing or extra rows padding.

Also, because xtrans/_abs arrays are defined in LibRaw class definition, and it is possible that we compile two source files with different -O.. switches, array layout should not changed from -O0 (no warning) to -O2 (warning issued).
So, I think this is gcc problem: it should either show this warning with any -O.. setting, or do not show it with -O2

Anyway, this loop was already changed to to 6-wide loops in master branch with commit message 'to make gcc happy' :)

-- Alex Tutubalin