Rawtherapee: RT crashes with HistogramMatching=true and ARW file

[mattiaverga]

A Fedora user reported a crash on opening an ARW image. I could reproduce it and I discovered that this is due to HistogramMatching=true in the [Exposure] section, by setting it to false I can open the file without getting the crash.

The original ARW file and backtrace of the crash are on https://bugzilla.redhat.com/show_bug.cgi?id=1692572

[heckflosse]

Opening the file with HistogramMatching=true works fine on current dev branch on Win7

[heckflosse]

@mattiaverga What RT version was that? I don't see that from the link you provided, which makes it hard to reproduce and also wastes dev time......

Edit: rawtherapee-5.5-1.fc29 is not a version number used by us...

[mattiaverga]

@heckflosse sorry, I forgot to write that, it's RT 5.5 from http://rawtherapee.com/shared/source/ with only the e491c42 patch applied.

At first I couldn't reproduce the crash, I had to delete my ~/.config/Rawtherapee folder to let RT resetting to its defaults and I also deleted the .pp3 file to let RT recreate it following its defaults. I'll attach it here.
DSC09189.ARW.pp3.zip

With this .pp3 file I get the crash every time I try to open the file from command line or from RT browser window (the preview in the browser works fine). If I change HistogramMatching to false the image is opened fine.

I don't know exactly what HistogramMatching does, but I suspect that particular function screws up with a completely black frame like this. I get no crash with HistogramMatching=true applied to other ARW samples downloaded from internet.

[Floessie]

I'm trying the reproduce...

[Floessie]

@mattiaverga @heckflosse I can't reproduce it with stock 5.5 on Debian (Fedora seems to utilize hardening flags), but I can get an abort if I replace mapping[...] with mapping.at(...). Digging deeper...

[Beep6581]

Could not reproduce in 5.5 nor in 5.5-263-g90e1480b7 in Sabayon (GCC7.3.0, deleted config and cache).

[Floessie]

Here's a fix for dev:

diff --git a/rtengine/histmatching.cc b/rtengine/histmatching.cc
index f91086285..b7a3a9666 100644
--- a/rtengine/histmatching.cc
+++ b/rtengine/histmatching.cc
@@ -122,7 +122,11 @@ void mappingToCurve(const std::vector<int> &mapping, std::vector<double> &curve)
         {
             if (!maxdelta) maxdelta = step * 2;
             int prev = start;
-            if (addstart && mapping[start] >= 0) {
+            if (
+                addstart
+                && start < static_cast<int>(mapping.size())
+                && mapping[start] >= 0
+            ) {
                 curve.push_back(coord(start));
                 curve.push_back(coord(mapping[start]));
             }

Preparing a full cleanup patch...

[Floessie]

Here's the cleanup patch for mappingToCurve():

diff --git a/rtengine/histmatching.cc b/rtengine/histmatching.cc
index f91086285..750bdc936 100644
--- a/rtengine/histmatching.cc
+++ b/rtengine/histmatching.cc
@@ -18,19 +18,21 @@
  *  along with RawTherapee.  If not, see <http://www.gnu.org/licenses/>.
  */

-#include "rawimagesource.h"
-#include "rtthumbnail.h"
-#include "curves.h"
+#include <iostream>
+
 #include "color.h"
-#include "rt_math.h"
+#include "curves.h"
 #include "iccstore.h"
+#include "improcfun.h"
 #include "procparams.h"
+#include "rawimagesource.h"
+#include "rt_math.h"
+#include "rtthumbnail.h"
+
 #include "../rtgui/mydiagonalcurve.h"
-#include "improcfun.h"
+
 //#define BENCHMARK
 #include "StopWatch.h"
-#include <iostream>
-

 namespace rtengine {

@@ -98,42 +100,72 @@ int findMatch(int val, const std::vector<int> &cdf, int j)
 }


-void mappingToCurve(const std::vector<int> &mapping, std::vector<double> &curve)
+void mappingToCurve(const std::vector<int>& mapping, std::vector<double>& curve)
 {
     curve.clear();

-    int idx = 15;
-    for (; idx < int(mapping.size()); ++idx) {
-        if (mapping[idx] >= idx) {
+    std::size_t idx = 15;
+    for (; idx < mapping.size(); ++idx) {
+        if (mapping[idx] >= static_cast<int>(idx)) {
             break;
         }
     }
-    if (idx == int(mapping.size())) {
-        for (idx = 1; idx < int(mapping.size()); ++idx) {
-            if (mapping[idx] >= idx) {
+
+    if (idx == mapping.size()) {
+        for (idx = 1; idx < mapping.size(); ++idx) {
+            if (mapping[idx] >= static_cast<int>(idx)) {
                 break;
             }
         }
     }

-    auto coord = [](int v) -> double { return double(v)/255.0; };
-    auto doit =
-        [&](int start, int stop, int step, bool addstart, int maxdelta=0) -> void
+    const auto coord =
+        [](int v) -> double
+        {
+            return static_cast<double>(v) / 255.0;
+        };
+
+    const auto doit =
+        [&mapping, &curve, &coord](
+            std::size_t start,
+            std::size_t stop,
+            std::size_t step,
+            bool addstart,
+            std::size_t maxdelta = 0
+        )
         {
-            if (!maxdelta) maxdelta = step * 2;
-            int prev = start;
-            if (addstart && mapping[start] >= 0) {
+            if (!maxdelta) {
+                maxdelta = step * 2;
+            }
+
+            std::size_t prev = start;
+
+            if (
+                addstart
+                && start < mapping.size()
+                && mapping[start] >= 0
+            ) {
                 curve.push_back(coord(start));
                 curve.push_back(coord(mapping[start]));
             }
-            for (int i = start; i < stop; ++i) {
-                int v = mapping[i];
+
+            for (std::size_t i = start; i < stop; ++i) {
+                const int v = mapping[i];
+
                 if (v < 0) {
                     continue;
                 }
-                bool change = i > 0 && v != mapping[i-1];
-                int diff = i - prev;
-                if ((change && std::abs(diff - step) <= 1) || diff > maxdelta) {
+
+                const std::size_t diff = i - prev;
+
+                if (
+                    diff > maxdelta
+                    || (
+                        i > 0
+                        && v != mapping[i - 1]
+                        && std::abs(static_cast<int>(diff) - static_cast<int>(step)) <= 1
+                    )
+                ) {
                     curve.push_back(coord(i));
                     curve.push_back(coord(v));
                     prev = i;
@@ -144,24 +176,26 @@ void mappingToCurve(const std::vector<int> &mapping, std::vector<double> &curve)
     curve.push_back(0.0);
     curve.push_back(0.0);

-    int start = 0;
+    std::size_t start = 0;
     while (start < idx && (mapping[start] < 0 || start < idx / 2)) {
         ++start;
     }

-    const int npoints = 8;
-    int step = std::max(int(mapping.size())/npoints, 1);
-    int end = mapping.size();
+    constexpr std::size_t npoints = 8;
+
+    std::size_t step = std::max<std::size_t>(mapping.size() / npoints, 1);
+    std::size_t end = mapping.size();
+
     if (idx <= end / 3) {
         doit(start, idx, idx / 2, true);
         step = (end - idx) / 4;
         doit(idx, end, step, false, step);
     } else {
         doit(start, idx, idx > step ? step : idx / 2, true);
-        doit(idx, end, step, idx - step > step / 2 && std::abs(curve[curve.size()-2] - coord(idx)) > 0.01);
+        doit(idx, end, step, idx - step > step / 2 && std::abs(curve[curve.size() - 2] - coord(idx)) > 0.01);
     }

-    if (curve.size() > 2 && (1 - curve[curve.size()-2] <= coord(step) / 3)) {
+    if (curve.size() > 2 && 1 - curve[curve.size() - 2] <= coord(step) / 3) {
         curve.pop_back();
         curve.pop_back();
     }
@@ -178,29 +212,34 @@ void mappingToCurve(const std::vector<int> &mapping, std::vector<double> &curve)
             // (x - xa) / (xb - xa) = (y - ya) / (yb - ya)
             return (x - xa) / (xb - xa) * (yb - ya) + ya;
         };
-    idx = -1;
-    for (size_t i = curve.size()-1; i > 0; i -= 2) {
-        if (curve[i] <= 0.f) {
-            idx = i+1;
+
+    bool has_idx = false;
+    idx = 0;
+
+    for (ssize_t i = curve.size() - 1; i > 0; i -= 2) {
+        if (curve[i] <= 0.0) {
+            has_idx = true;
+            idx = i + 1;
             break;
         }
     }
-    if (idx >= 0 && size_t(idx) < curve.size()) {
+
+    if (has_idx && idx < curve.size()) {
         // idx is the position of the first point in the upper part of the S
         // for each 3 consecutive points (xa, ya), (x, y), (xb, yb) we check
         // that y is above the point at x of the line between the other two
         // if this is not the case, we remove (x, y) from the curve
-        while (size_t(idx+5) < curve.size()) {
-            float xa = curve[idx];
-            float ya = curve[idx+1];
-            float x = curve[idx+2];
-            float y = curve[idx+3];
-            float xb = curve[idx+4];
-            float yb = curve[idx+5];
-            float yy = getpos(x, xa, ya, xb, yb);
+        while (idx + 5 < curve.size()) {
+            const float xa = curve[idx];
+            const float ya = curve[idx + 1];
+            const float x = curve[idx + 2];
+            const float y = curve[idx + 3];
+            const float xb = curve[idx + 4];
+            const float yb = curve[idx + 5];
+            const float yy = getpos(x, xa, ya, xb, yb);
             if (yy > y) {
                 // we have to remove (x, y) from the curve
-                curve.erase(curve.begin()+(idx+2), curve.begin()+(idx+4));
+                curve.erase(curve.begin() + (idx + 2), curve.begin() + (idx + 4));
             } else {
                 // move on to the next point
                 idx += 2;
@@ -209,19 +248,23 @@ void mappingToCurve(const std::vector<int> &mapping, std::vector<double> &curve)
     }

     if (curve.size() < 4) {
-        curve = { DCT_Linear }; // not enough points, fall back to linear
+        curve = {DCT_Linear}; // not enough points, fall back to linear
     } else {
         curve.insert(curve.begin(), DCT_Spline);
-        DiagonalCurve c(curve);
+        const DiagonalCurve c(curve);
+
+        curve = {DCT_CatumullRom};
+
         double gap = 0.05;
         double x = 0.0;
-        curve = { DCT_CatumullRom };
+
         while (x < 1.0) {
             curve.push_back(x);
             curve.push_back(c.getVal(x));
             x += gap;
             gap *= 1.4;
         }
+
         curve.push_back(1.0);
         curve.push_back(c.getVal(1.0));
     }

@heckflosse Please give this a full review. I changed the indices to std::size_t and hope I didn't screw it. Some further observations:

• There is a potential bug here which I tried to fix by using ssize_t.
• We could assert(start > 0) instead of this here.
• We could assert(curve.size() > 2) here.

HTH,
Flössie

[heckflosse]

@Floessie I didn't test yet because I'm on a different branch atm. But from what I read, it looks good.

[Floessie]

@heckflosse I'm not able to push before Sunday evening. Feel free to commit.

[agriggio]

Here's my take on the patch, FWIW:

diff --git a/rtengine/histmatching.cc b/rtengine/histmatching.cc
--- a/rtengine/histmatching.cc
+++ b/rtengine/histmatching.cc
@@ -109,7 +109,7 @@
         }
     }
     if (idx == int(mapping.size())) {
-        for (idx = 1; idx < int(mapping.size()); ++idx) {
+        for (idx = 1; idx < int(mapping.size())-1; ++idx) {
             if (mapping[idx] >= idx) {
                 break;
             }

[Beep6581]

Bump, let's resolve this to release 5.6-rc1. I can't reproduce the crash though.

[Floessie]

@agriggio Wouldn't your fix still fail for mapping.size() < 15?

[agriggio]

sure, but mapping has size 256

[Floessie]

@agriggio Okay. Then, why not use a std::array<> for a fixed size array?

@Beep6581 To make some progress here before releasing 5.6 I suggest the following:

• You pick Alberto's patch for 5.6 as it's the most concise one.
• I still think there is another bug here which should be fixed in passing.
• After the release we take my cleanup patch as a base, remove the initially proposed index check, turn the std::vector<>s into std::array<>s where feasible (no need to assert anymore, then), and let it brew in dev for 5.7.

Best,
Flössie

[mattiaverga]

With the two above patches applied on top of RT 5.5 I get no more crashes. Thanks!

[Floessie]

@mattiaverga Mattia, could also give dev a try. Should be fixed there, too.

[Beep6581]

If the patches work and are approved, please commit tomorrow, or remove the 5.7 milestone.

[Floessie]

@Beep6581 AFAIK the fixes are already in dev. The cleanup patch is just what it is: a cleanup.

[Beep6581]

@Floessie hmm it seems this fix https://github.com/Beep6581/RawTherapee/issues/5249#issuecomment-477025043 is not in dev - https://github.com/Beep6581/RawTherapee/blob/dev/rtengine/histmatching.cc#L125

I opened a new issue (milestone 5.8) for the cleanup, leaving this one to address only the fix.

[Floessie]

It's fixed by Alberto's patch.

[Beep6581]

Oh great, then closing.