Rawtherapee: RT crashes with wrong value in profile (curve)

[Krzysiu]

RT 5.3 on WIndows 7

Version: 5.3
Branch: 5.3
Commit: ec0f793
Commit date: 2017-09-30
Compiler: gcc 7.2.0
Processor: generic x86
System: Windows
Bit depth: 64 bits
Gtkmm: V3.22.0
Lensfun: V0.3.2.0
Build type: release
Build flags: -m64 -mwin32 -mthreads -Wno-aggressive-loop-optimizations -std=c++11 -mtune=generic -Werror=unused-label -fopenmp -Werror=unknown-pragmas -Wall -Wno-unused-result -Wno-deprecated-declarations -DNDEBUG -O3
Link flags: -m64 -mthreads -static-libgcc -mtune=generic -s -O3
OpenMP support: ON
MMAP support: ON

OS/PC: Windows 7 64 bit, tested on different machines and problem is the same

Download source: http://rawtherapee.com/releases_head/windows/RawTherapee_5.3_WinVista_64.zip

Screenshot

Reproducing the bug

1. Create profile with setting Curve=1; in Exposure section:
   ^{full profile which triggers the bug: http://txt.krzysiu.net/view/raw/899658d8 - let's call it BADPROF}
2. Either set it as default profile or load it as full or partial (?) profile - you can see there's curve editor shown, but nodes to create/edit the curve are missing
3. Click on the curve editor
4. Program crashes - oh, curve!

It also creates additional problems. All reproduction steps below have common part which has to be made before next steps:

1. Set BADPROF as default profile or load it as a full profile
2. Open any photo

1. With such malformed profile, RT creates photo profile copying that wrong setting (excepted: ignoring wrong value and saving default (0)/no value). Example:
   1.1 The profile is saved automatically along with erroneous setting Curve=1;
2. It's impossible to edit curve with such profile
   2.1 Set curve to linear (i.e. no editor, default setting)
   2.2 Go back to previous setting (Curve type: custom)
   2.3 Still lack of nodes & crash on click on the curve
3. Profile with good value (Curve=0;) doesn't fix the problem, if erroneous profile was loaded earlier.
   3.1 Load profile with fixed Curve setting
   ^{http://txt.krzysiu.net/view/raw/f266fe8e - let's call it GOODPROF the same as BADPROF, but with Curve changed from problematic 1 to working 0}
   3.2 Switch the curve type to Custom
   3.3 Resetting bad setting didn't help - there's no nodes in the editor and clicking on it causes crash
4. Or other way round (initial steps don't apply here!)
   4.1 Open file with GOODPROF as a main profile
   4.2 [optional] set curve editor to custom
   4.3 Load BADPROF
   4.4 Nodes were there with 4.1 and 4.2, but they disappear with 4.3

Excepted fix
My proposed way of dealing with Curve=1; setting would be to allow it, but set default values. I think such fix would be the best and would follow other settings - if something's not set, then default is used - in this case it would be 0;0;1;1; (nodes coordinates).

[heckflosse]

Bug confirmed.

[heckflosse]

This should fix the issue:

diff --git a/rtengine/procparams.cc b/rtengine/procparams.cc
index 64b7fea1..6b25d3fa 100644
--- a/rtengine/procparams.cc
+++ b/rtengine/procparams.cc
@@ -88,10 +88,22 @@ Glib::ustring relativePathIfInside (const Glib::ustring &procparams_fname, bool
     return prefix + embedded_fname.substr (dir1.length());
 }

-void avoidEmptyCurve (std::vector<double> &curve)
+void avoidInvalidCurve (std::vector<double> &curve)
 {
+    // a curve is valid under one of the following conditions
+    // 1) curve has exactly one entry which is D(F)CT_Linear
+    // 2) number of curve entries is > 1 and odd
+    // 3) curve[0] == DCT_Parametric and curve size is >= 8
     if (curve.empty()) {
-        curve.push_back (FCT_Linear);
+        curve.push_back (DCT_Linear);
+    } else if(curve.size() == 1 && curve[0] != DCT_Linear) {
+        curve[0] = DCT_Linear;
+    } else if(curve.size() % 2 == 0 && curve[0] != DCT_Parametric) {
+        curve.clear();
+        curve.push_back (DCT_Linear);
+    } else if(curve[0] == DCT_Parametric && curve.size() < 8) {
+        curve.clear();
+        curve.push_back (DCT_Linear);
     }
 }

@@ -143,7 +155,7 @@ void getFromKeyfile(
 )
 {
     value = keyfile.get_double_list(group_name, key);
-    avoidEmptyCurve(value);
+    avoidInvalidCurve(value);
 }

 template<typename T>

[heckflosse]

@Floessie @Krzysiu I just updated the patch.

[Hombre57]

@Krzysiu I don't understand your point : are you testing "RT's bulletproofness" regarding handwritted malformed pp3 file ? The curve you describe, made of only one value of "1" can't be generated by the GUI AFAIK.

[Floessie]

@Hombre57 IHMO it doesn't matter if a crash was found by fuzzing or "legal" usage.

[Hombre57]

Ok, then you should also check that the coordinates are each greater or equal than the previous ones for the X axis and that they are clamped to the [0;1] domain (don't know for the Parametric curve)

[Beep6581]

I created this beauty in RT the other day, thought I'd share:

[heckflosse]

IMHO each reported crash should be fixed, especially crashes with such a good bug report as this one (thanks to @Krzysiu). Solving them saves us time because they may be reported later without a good bug report if we don't solve them and then we will spend more time to find the bug than to fix it.

[heckflosse]

Though I think we should fix this, I also think we should not fix this for 5.4.
Rationale: This is an old bug, 5.4 is around the corner, and my patch is not thoroughly tested for all curve types written by rt (I don't speak about pp3 files manually edited). If there's a bug in my patch for correct pp3 files, this bug will hurt.

Clear vote against committing my patch for 5.4

Ingo

[heckflosse]

As this is still set as a 5.5 blocker, I would like to get opinions how to proceed because no one tested my patch, which makes it risky to commit it for 5.5.

I've no objections to move it to 5.6 milestone.

[Floessie]

One way to gain a better test coverage is to move this patch into a branch, turn that into a PR and fix it there. It's a pitty this patch got lost during the 5.5 cycle, though I'm also to blame as I could have tested it. :disappointed:

[Beep6581]

These RTC and PP3 files cause RT 5.4-1012-g5cb42f29e to crash:
https://filebin.net/j8d0dg5y945z15ne

The RTC files cause it to crash right after they're loaded, while the PP3s make RT crash when you click in the curve area after loading the PP3.

parametric_4.pp3 does not make RT crash, and the parametric sliders are set to the values from the PP3, but the curve remains linear until you touch a slider.

Here is @heckflosse 's patch edited to apply to current dev:

diff --git a/rtengine/procparams.cc b/rtengine/procparams.cc
index 526a36765..b8857141f 100644
--- a/rtengine/procparams.cc
+++ b/rtengine/procparams.cc
@@ -87,10 +87,22 @@ Glib::ustring relativePathIfInside(const Glib::ustring &procparams_fname, bool f
     return prefix + embedded_fname.substr(dir1.length());
 }

-void avoidEmptyCurve(std::vector<double> &curve)
+void avoidInvalidCurve (std::vector<double> &curve)
 {
+    // a curve is valid under one of the following conditions
+    // 1) curve has exactly one entry which is D(F)CT_Linear
+    // 2) number of curve entries is > 1 and odd
+    // 3) curve[0] == DCT_Parametric and curve size is >= 8
     if (curve.empty()) {
-        curve.push_back(FCT_Linear);
+        curve.push_back (DCT_Linear);
+    } else if(curve.size() == 1 && curve[0] != DCT_Linear) {
+        curve[0] = DCT_Linear;
+    } else if(curve.size() % 2 == 0 && curve[0] != DCT_Parametric) {
+        curve.clear();
+        curve.push_back (DCT_Linear);
+    } else if(curve[0] == DCT_Parametric && curve.size() < 8) {
+        curve.clear();
+        curve.push_back (DCT_Linear);
     }
 }

@@ -142,7 +154,7 @@ void getFromKeyfile(
 )
 {
     value = keyfile.get_double_list(group_name, key);
-    avoidEmptyCurve(value);
+    avoidInvalidCurve(value);
 }

 template<typename T>

With the patch the corrupted curves from the PP3 files no longer load so RT doesn't crash when I click in the curve area, but the RTC files still load and crash RT.

[heckflosse]

@Beep6581 I'm working on the RTC stuff now

[Floessie]

@heckflosse Here's my take:

diff --git a/rtengine/curves.cc b/rtengine/curves.cc
index aab74a7de..892ea146c 100644
--- a/rtengine/curves.cc
+++ b/rtengine/curves.cc
@@ -45,6 +45,30 @@ using namespace std;
 namespace rtengine
 {

+bool sanitizeCurve(std::vector<double>& curve)
+{
+    // A curve is valid under one of the following conditions:
+    // 1) Curve has exactly one entry which is D(F)CT_Linear
+    // 2) Number of curve entries is > 1 and odd
+    // 3) curve[0] == DCT_Parametric and curve size is >= 8
+    if (curve.empty()) {
+        curve.push_back (DCT_Linear);
+        return true;
+    } else if(curve.size() == 1 && curve[0] != DCT_Linear) {
+        curve[0] = DCT_Linear;
+        return true;
+    } else if(curve.size() % 2 == 0 && curve[0] != DCT_Parametric) {
+        curve.clear();
+        curve.push_back (DCT_Linear);
+        return true;
+    } else if(curve[0] == DCT_Parametric && curve.size() < 8) {
+        curve.clear();
+        curve.push_back (DCT_Linear);
+        return true;
+    }
+    return false;
+}
+
 Curve::Curve () : N(0), ppn(0), x(nullptr), y(nullptr), mc(0.0), mfc(0.0), msc(0.0), mhc(0.0), hashSize(1000 /* has to be initialized to the maximum value */), ypp(nullptr), x1(0.0), y1(0.0), x2(0.0), y2(0.0), x3(0.0), y3(0.0), firstPointIncluded(false), increment(0.0), nbr_points(0) {}

 void Curve::AddPolygons ()
diff --git a/rtengine/curves.h b/rtengine/curves.h
index a9f4b4ace..30fb01102 100644
--- a/rtengine/curves.h
+++ b/rtengine/curves.h
@@ -19,9 +19,12 @@
 #ifndef __CURVES_H__
 #define __CURVES_H__

-#include <glibmm.h>
 #include <map>
 #include <string>
+#include <vector>
+
+#include <glibmm.h>
+
 #include "rt_math.h"
 #include "../rtgui/mycurve.h"
 #include "../rtgui/myflatcurve.h"
@@ -42,6 +45,7 @@ using namespace std;

 namespace rtengine
 {
+
 class ToneCurve;
 class ColorAppearance;

@@ -55,6 +59,8 @@ void setUnlessOOG(T &r, T &g, T &b, const T &rr, const T &gg, const T &bb)
     }
 }

+bool sanitizeCurve(std::vector<double>& curve);
+
 namespace curves {

 inline void setLutVal(const LUTf &lut, float &val)
diff --git a/rtengine/procparams.cc b/rtengine/procparams.cc
index 526a36765..b07778c55 100644
--- a/rtengine/procparams.cc
+++ b/rtengine/procparams.cc
@@ -87,13 +87,6 @@ Glib::ustring relativePathIfInside(const Glib::ustring &procparams_fname, bool f
     return prefix + embedded_fname.substr(dir1.length());
 }

-void avoidEmptyCurve(std::vector<double> &curve)
-{
-    if (curve.empty()) {
-        curve.push_back(FCT_Linear);
-    }
-}
-
 void getFromKeyfile(
     const Glib::KeyFile& keyfile,
     const Glib::ustring& group_name,
@@ -142,7 +135,7 @@ void getFromKeyfile(
 )
 {
     value = keyfile.get_double_list(group_name, key);
-    avoidEmptyCurve(value);
+    rtengine::sanitizeCurve(value);
 }

 template<typename T>
diff --git a/rtgui/diagonalcurveeditorsubgroup.cc b/rtgui/diagonalcurveeditorsubgroup.cc
index a022c8650..c23b25f9a 100644
--- a/rtgui/diagonalcurveeditorsubgroup.cc
+++ b/rtgui/diagonalcurveeditorsubgroup.cc
@@ -32,6 +32,8 @@
 #include "diagonalcurveeditorsubgroup.h"
 #include "rtimage.h"

+#include "../rtengine/curves.h"
+
 DiagonalCurveEditorSubGroup::DiagonalCurveEditorSubGroup (CurveEditorGroup* prt, Glib::ustring& curveDir) : CurveEditorSubGroup(curveDir)
 {

@@ -814,6 +816,8 @@ void DiagonalCurveEditorSubGroup::loadPressed ()
                 }
             }

+            rtengine::sanitizeCurve(p);
+
             if (p[0] == (double)(DCT_Spline)) {
                 customCurve->setPoints (p);
                 customCurve->queue_draw ();
diff --git a/rtgui/flatcurveeditorsubgroup.cc b/rtgui/flatcurveeditorsubgroup.cc
index 2cc96a184..27b7ac940 100644
--- a/rtgui/flatcurveeditorsubgroup.cc
+++ b/rtgui/flatcurveeditorsubgroup.cc
@@ -33,6 +33,8 @@
 #include "flatcurveeditorsubgroup.h"
 #include "rtimage.h"

+#include "../rtengine/curves.h"
+
 FlatCurveEditorSubGroup::FlatCurveEditorSubGroup (CurveEditorGroup* prt, Glib::ustring& curveDir) : CurveEditorSubGroup(curveDir)
 {

@@ -418,6 +420,8 @@ void FlatCurveEditorSubGroup::loadPressed ()
                 }
             }

+            rtengine::sanitizeCurve(p);
+
             if (p[0] == (double)(FCT_MinMaxCPoints)) {
                 CPointsCurve->setPoints (p);
                 CPointsCurve->queue_draw ();

This still crashes in LUT<>::..., but that seems to be a different problem. I didn't know where to place sanitizeCurve(), so please move it to where it's more suitable.

HTH,
Flössie

[heckflosse]

@Floessie If the curve is type parametric, the values 1 to 3 of the vector must be ordered ascending and no two of this values be equal.

Something like this would be a fallback for parametric curves:

                    // p[1] to p[3] must be ordered ascending and no two values be equal
                    for (int i = 1; i < 3; i++) {
                        if (p[i] >= p[i + 1]) {
                            p[1] = 0.25f;
                            p[2] = 0.5f;
                            p[3] = 0.75f;
                            break;
                        }
                    }

[heckflosse]

@Floessie

I enhanced your patch:

diff --git a/rtengine/curves.cc b/rtengine/curves.cc
index aab74a7de..9e3c6527e 100644
--- a/rtengine/curves.cc
+++ b/rtengine/curves.cc
@@ -45,6 +45,42 @@ using namespace std;
 namespace rtengine
 {

+bool sanitizeCurve(std::vector<double>& curve)
+{
+    // A curve is valid under one of the following conditions:
+    // 1) Curve has exactly one entry which is D(F)CT_Linear
+    // 2) Number of curve entries is > 3 and odd
+    // 3) curve[0] == DCT_Parametric and curve size is >= 8 and curve[1] .. curve[3] are ordered ascending and are distinct
+    if (curve.empty()) {
+        curve.push_back (DCT_Linear);
+        return true;
+    } else if(curve.size() == 1 && curve[0] != DCT_Linear) {
+        curve[0] = DCT_Linear;
+        return true;
+    } else if((curve.size() % 2 == 0 || curve.size() < 5) && curve[0] != DCT_Parametric) {
+        curve.clear();
+        curve.push_back (DCT_Linear);
+        return true;
+    } else if(curve[0] == DCT_Parametric) {
+        if (curve.size() < 8) {
+            curve.clear();
+            curve.push_back (DCT_Linear);
+            return true;
+        } else {
+            // curve[1] to curve[3] must be ordered ascending and distinct
+            for (int i = 1; i < 3; i++) {
+                if (curve[i] >= curve[i + 1]) {
+                    curve[1] = 0.25f;
+                    curve[2] = 0.5f;
+                    curve[3] = 0.75f;
+                    break;
+                }
+            }
+        }
+    }
+    return false;
+}
+
 Curve::Curve () : N(0), ppn(0), x(nullptr), y(nullptr), mc(0.0), mfc(0.0), msc(0.0), mhc(0.0), hashSize(1000 /* has to be initialized to the maximum value */), ypp(nullptr), x1(0.0), y1(0.0), x2(0.0), y2(0.0), x3(0.0), y3(0.0), firstPointIncluded(false), increment(0.0), nbr_points(0) {}

 void Curve::AddPolygons ()
diff --git a/rtengine/curves.h b/rtengine/curves.h
index a9f4b4ace..30fb01102 100644
--- a/rtengine/curves.h
+++ b/rtengine/curves.h
@@ -19,9 +19,12 @@
 #ifndef __CURVES_H__
 #define __CURVES_H__

-#include <glibmm.h>
 #include <map>
 #include <string>
+#include <vector>
+
+#include <glibmm.h>
+
 #include "rt_math.h"
 #include "../rtgui/mycurve.h"
 #include "../rtgui/myflatcurve.h"
@@ -42,6 +45,7 @@ using namespace std;

 namespace rtengine
 {
+
 class ToneCurve;
 class ColorAppearance;

@@ -55,6 +59,8 @@ void setUnlessOOG(T &r, T &g, T &b, const T &rr, const T &gg, const T &bb)
     }
 }

+bool sanitizeCurve(std::vector<double>& curve);
+
 namespace curves {

 inline void setLutVal(const LUTf &lut, float &val)
diff --git a/rtengine/procparams.cc b/rtengine/procparams.cc
index 526a36765..b07778c55 100644
--- a/rtengine/procparams.cc
+++ b/rtengine/procparams.cc
@@ -87,13 +87,6 @@ Glib::ustring relativePathIfInside(const Glib::ustring &procparams_fname, bool f
     return prefix + embedded_fname.substr(dir1.length());
 }

-void avoidEmptyCurve(std::vector<double> &curve)
-{
-    if (curve.empty()) {
-        curve.push_back(FCT_Linear);
-    }
-}
-
 void getFromKeyfile(
     const Glib::KeyFile& keyfile,
     const Glib::ustring& group_name,
@@ -142,7 +135,7 @@ void getFromKeyfile(
 )
 {
     value = keyfile.get_double_list(group_name, key);
-    avoidEmptyCurve(value);
+    rtengine::sanitizeCurve(value);
 }

 template<typename T>
diff --git a/rtgui/diagonalcurveeditorsubgroup.cc b/rtgui/diagonalcurveeditorsubgroup.cc
index a022c8650..c23b25f9a 100644
--- a/rtgui/diagonalcurveeditorsubgroup.cc
+++ b/rtgui/diagonalcurveeditorsubgroup.cc
@@ -32,6 +32,8 @@
 #include "diagonalcurveeditorsubgroup.h"
 #include "rtimage.h"

+#include "../rtengine/curves.h"
+
 DiagonalCurveEditorSubGroup::DiagonalCurveEditorSubGroup (CurveEditorGroup* prt, Glib::ustring& curveDir) : CurveEditorSubGroup(curveDir)
 {

@@ -814,6 +816,8 @@ void DiagonalCurveEditorSubGroup::loadPressed ()
                 }
             }

+            rtengine::sanitizeCurve(p);
+
             if (p[0] == (double)(DCT_Spline)) {
                 customCurve->setPoints (p);
                 customCurve->queue_draw ();
diff --git a/rtgui/flatcurveeditorsubgroup.cc b/rtgui/flatcurveeditorsubgroup.cc
index 2cc96a184..27b7ac940 100644
--- a/rtgui/flatcurveeditorsubgroup.cc
+++ b/rtgui/flatcurveeditorsubgroup.cc
@@ -33,6 +33,8 @@
 #include "flatcurveeditorsubgroup.h"
 #include "rtimage.h"

+#include "../rtengine/curves.h"
+
 FlatCurveEditorSubGroup::FlatCurveEditorSubGroup (CurveEditorGroup* prt, Glib::ustring& curveDir) : CurveEditorSubGroup(curveDir)
 {

@@ -418,6 +420,8 @@ void FlatCurveEditorSubGroup::loadPressed ()
                 }
             }

+            rtengine::sanitizeCurve(p);
+
             if (p[0] == (double)(FCT_MinMaxCPoints)) {
                 CPointsCurve->setPoints (p);
                 CPointsCurve->queue_draw ();

[Floessie]

@heckflosse Thanks! I'm still not content with the current location of sanitizeCurve(). Any idea where to put it?

[heckflosse]

@Floessie

Any idea where to put it?

No

[Floessie]

@heckflosse @Beep6581 Successfully tested with all RTCs from the filebin above. No crashes anymore.

[heckflosse]

@Floessie @Beep6581 Shall we commit for 5.5 or is it too risky?

[Beep6581]

@heckflosse let's commit.