dcraw: increase linear table parsing to 65536 values

[sgotti]

The dcraw linear_table method limits the max values to 4096.
But 16 bit per channel linear DNGs can provide a LinearizationTable with
65536 entries.

This patch changes the dcraw linear_table method to accept 65536
entries.

[Thanatomanic]

If you change this conditional, wouldn't there be a problem for the intermediate range? I.e. when len > 0x1000 and len < 0x10000?

[sgotti]

All entries above the tag provided number of entries will all have the last entry value and maximum will also have it.

With this patch for 4096 entries all the entries from index 4096 to 65535 will have the value of curve[4095].

In the current code, instead, all entries above 4096 will instead have value of the index (initialized in dcraw::identify) since they aren't populated (or are they populated in another code path?).

So there's something different but I'm unsure if this will have bad effects.

[heckflosse]

@Thanatomanic I pressed the resolve button by accident. The code is correct imho, the original one and the code suggested by me, because all array entries >= index len will be filled with the maximum value. Or do I miss something?

[heckflosse]

@sgotti

In the current code, instead, all entries above 4096 will instead have value of 0 since they aren't populated (or are they populated in another code path?).

I think they will not be 0, but uninitialized (undefined). iirc only global and static arrays are initialized by default.

[sgotti]

In the current code, instead, all entries above 4096 will instead have value of 0 since they aren't populated (or are they populated in another code path?).

I think they will not be 0, but uninitialized (undefined). iirc only global and static arrays are initialized by default.

Yeah, sorry, I forgot to say that it's initialized in dcraw::identify but not to 0 as I remembered but to the index value.

RawTherapee/rtengine/dcraw.cc

Line 9473 in 07ed319

for (i=0; i < 0x10000; i++) curve[i] = i;

I'll fix my previous comment.

[Thanatomanic]

Great, I think this is a good fix. For reference sake, it would be good to a have example DNGs: one with a table of 4096 entries and one with more. @sgotti Do you have such files? If so, can you upload them through filebin.net or similar?

[sgotti]

@Thanatomanic @heckflosse

This file from raw.pixls.us has a 4096 entries LinearizationTable: https://raw.pixls.us/getfile.php/970/nice/Blackmagic%20-%20URSA%204K%20-%2012bit%20(16:9).dng (and is referenced by issue #4285)

The unique file with a LinearizationTable greater than 4096 that I know of is the one provided in this thread: https://discuss.pixls.us/t/problems-with-rt-and-on1-nonoise-dng-files/30027

But with further analysis looks like that kind of DNG is forcing the use of the LinearizationTable for other purposes than the original ones since it has 16 bit input channels mapped to 16 bit output channels. So the curve is not used for converting from e.g. 8bit to 16bit linear but to apply a sort of gamma conversion.

Anyway it could have sense to handle a table > 4096 (libraw does this). For example to map 14-bit to 16-bit data.

[heckflosse]

To avoid unnecessary filling the upper part of the array I suggest this:

void CLASS linear_table (unsigned len)
{
  const unsigned maxLen = std::min(0x10000ull, 1ull << tiff_bps);
  len = std::min(len, maxLen);
  read_shorts(curve, len);
  maximum = curve[len - 1];
  for (std::size_t i = len; i < maxLen; ++i) {
    curve[i] = maximum;
  }
}

[Entropy512]

Hmm, could that potentially behave badly if a malformed DNG that has a smaller LinearizationTable but high bit depth is fed? I believe this is fundamentally invalid, but could lead to a memory denial of service vulnerability here. (For example, feeding a 32-bit TIFF that has a LinearizationTable is invalid but would behave quite badly here...)

Edit: Wait, you have that std:min to handle this if more than 16 bits though, never mind.

[sgotti]

@heckflosse

const unsigned maxLen = std::min(0x10000ull, 1ull << tiff_bps);

From a fast look at the code it seems to me that tiff_bps may be populated in apply_tiff that is called after parse_tiff which calls parse_tiff_ifd that calls linear_table for dng files. 🤯

[heckflosse]

From a fast look at the code it seems to me that tiff_bps may be populated in apply_tiff that is called after parse_tiff which calls parse_tiff_ifd that calls linear_table for dng files.

I placed a std::cout << tiff_bps << std::endl; inside linear_table and it always showed the correct value. I will have another look...

[sgotti]

@heckflosse You're right, it's also populated previously.

[heckflosse]

Btw: I'm not against merging the fix from the first patch, though I think we could also merge my improvements.

[Entropy512]

That one's up to you and @sgotti - I think your fixes are a good idea, even if libraw also appears to do it the same way as @sgotti (repeating the last entry in the DNG table until you reach 65536 entries)

[sgotti]

@heckflosse I'll push a new version with your changes if you're ok.

[Entropy512]

@Beep6581 Looking through outstanding PRs, this particular bugfix probably should be resolved before 5.9 is released since this definitely did cause some bugs with DNGs from certain cameras

[Entropy512]

@Beep6581 Since this missed 5.9, this should probably be added to the 5.10 milestone (not sure if I can do that)

[Beep6581]

@Entropy512 I missed it too. Feel free to merge - the sooner the more time for testing.

this definitely did cause some bugs with DNGs from certain cameras

Which ones?

[Entropy512]

Which ones?

Blackmagic Pocket 4k - #6106

@heckflosse had a few improvements that probably make sense, not sure what the appropriate process is for ninjaing someone else's PR. It's definitely someone else's role to merge unless I've been granted permissions I've never had before... (If so, is whomever did so SURE they know what they are doing? :P )

[Beep6581]

If nobody objects, I will merge this tomorrow and then apply the suggested changes afterwards.