Rawtherapee: Error in parsing pp3 file with long lines

[agriggio]

The attached pp3 file results in the following error in Procparams::load:

-->Key file contains line '999999998;' which is not a key-value pair, group, or comment

Here's a patch that fixes the problem:

diff --git a/rtengine/procparams.cc b/rtengine/procparams.cc
--- a/rtengine/procparams.cc
+++ b/rtengine/procparams.cc
@@ -3749,14 +3749,12 @@
             return 1;
         }

-        char* buffer = new char[1024];
+        char buffer[1024];
         std::ostringstream ostr;

         while (fgets (buffer, 1024, f)) {
-            ostr << buffer << "\n";
-        }
-
-        delete [] buffer;
+            ostr << buffer;
+        }

         if (!keyFile.load_from_data (ostr.str())) {
             return 1;

Before pushing, however, I'd like to understand the why are we reading the file into memory at all instead of using Glib::KeyFile::load_from_file directly. Anyone knows?

test.txt

[Hombre57]

@agriggio No idea ! Could there be a dead lock in that case if RT crash while interpreting the keyfile (as it already occurred) ?

Why not enclosing the buffer into braces to delete it early (if the compiler doesn't do it itself) ?

[heckflosse]

@Hombre57

Why not enclosing the buffer into braces to delete it early (if the compiler doesn't do it itself) ?

Stack allocation of this (1024 chars) size is perfectly fine imho. It avoids new and delete and we should trust the compiler to early reduce the stack pointer or not.

[agriggio]

@Hombre57 suggested on IRC that this might be due to line-ending conversions. If so, here's a better patch:

diff --git a/rtengine/procparams.cc b/rtengine/procparams.cc
--- a/rtengine/procparams.cc
+++ b/rtengine/procparams.cc
@@ -3749,14 +3749,15 @@
             return 1;
         }

-        char* buffer = new char[1024];
+        char buffer[1024];
         std::ostringstream ostr;

         while (fgets (buffer, 1024, f)) {
-            ostr << buffer << "\n";
-        }
-
-        delete [] buffer;
+            ostr << buffer;
+            if (buffer[strlen(buffer)-1] == '\n') {
+                ostr << '\n';
+            }
+        }

         if (!keyFile.load_from_data (ostr.str())) {
             return 1;

(Note: I'm still using g_fopen instead of a std::ifstream because the former is supposed to handle unicode file names properly in a cross-platform manner...)

[agriggio]

Actually, forget about the above. This seems to work just fine:

diff --git a/rtengine/procparams.cc b/rtengine/procparams.cc
--- a/rtengine/procparams.cc
+++ b/rtengine/procparams.cc
@@ -3743,29 +3743,10 @@
             pedited->set (false);
         }

-        FILE* f = g_fopen (fname.c_str (), "rt");
-
-        if (!f) {
+        if (!keyFile.load_from_file(fname)) {
             return 1;
         }

-        char* buffer = new char[1024];
-        std::ostringstream ostr;
-
-        while (fgets (buffer, 1024, f)) {
-            ostr << buffer << "\n";
-        }
-
-        delete [] buffer;
-
-        if (!keyFile.load_from_data (ostr.str())) {
-            return 1;
-        }
-
-        fclose (f);
-
-        // load tonecurve:
-
         ppVersion = PPVERSION;
         appVersion = APPVERSION;

[Hombre57]

From IRC, reported as successfully tested, even with UTF-8 chars, so +1 to merge.

[Floessie]

When I enter a directory with undeveloped images, the new function throws for every file that has no PP3, resulting in many ugly lines on the terminal.

(gdb) bt
#0  write () at ../sysdeps/unix/syscall-template.S:84
#1  0x00007ffff11007e7 in _IO_new_file_write (f=0x7ffff1428600 <_IO_2_1_stdout_>, data=0x7fffb4002e20, n=29) at fileops.c:1271
#2  0x00007ffff10ffb32 in new_do_write (fp=0x7ffff1428600 <_IO_2_1_stdout_>, data=0x7fffb4002e20 "-->No such file or directory\n\177", to_do=to_do@entry=29) at fileops.c:526
#3  0x00007ffff11018a9 in _IO_new_do_write (fp=<optimized out>, data=<optimized out>, to_do=29) at fileops.c:502
#4  0x00007ffff1100e86 in _IO_new_file_xsputn (f=0x7ffff1428600 <_IO_2_1_stdout_>, data=<optimized out>, n=1) at fileops.c:1339
#5  0x00007ffff10d5489 in _IO_vfprintf_internal (s=0x7ffff1428600 <_IO_2_1_stdout_>, format=<optimized out>, ap=ap@entry=0x7fffddd4d718) at vfprintf.c:1668
#6  0x00007ffff10dd1f9 in __printf (format=<optimized out>) at printf.c:33
#7  0x0000555556095fc8 in rtengine::procparams::ProcParams::load (this=0x7fffb40037d8, fname=..., pedited=0x0) at /home/user/src/rawtherapee/rtengine/procparams.cc:8230
#8  0x0000555555e4b2e3 in Thumbnail::loadProcParams (this=0x7fffb4003620) at /home/user/src/rawtherapee/rtgui/thumbnail.cc:329
#9  0x0000555555e49400 in Thumbnail::Thumbnail (this=0x7fffb4003620, cm=0x5555567ffd00 <CacheManager::getInstance()::instance>, fname=..., cf=0x7fffddd5f6b0) at /home/user/src/rawtherapee/rtgui/thumbnail.cc:44
#10 0x0000555555aec400 in CacheManager::getEntry (this=0x5555567ffd00 <CacheManager::getInstance()::instance>, fname=...) at /home/user/src/rawtherapee/rtgui/cachemanager.cc:105
#11 0x0000555555dbbb67 in PreviewLoader::Impl::processNextJob (this=0x55555bebb1a0) at /home/user/src/rawtherapee/rtgui/previewloader.cc:128
#12 0x0000555555dbd082 in sigc::bound_mem_functor0<void, PreviewLoader::Impl>::operator() (this=0x7fffb40017a8) at /usr/include/sigc++-2.0/sigc++/functors/mem_fun.h:1991
#13 0x0000555555dbce60 in sigc::adaptor_functor<sigc::bound_mem_functor0<void, PreviewLoader::Impl> >::operator() (this=0x7fffb40017a0) at /usr/include/sigc++-2.0/sigc++/adaptors/adaptor_trait.h:256
#14 0x0000555555dbcc1f in sigc::internal::slot_call0<sigc::bound_mem_functor0<void, PreviewLoader::Impl>, void>::call_it (rep=0x7fffb4001770) at /usr/include/sigc++-2.0/sigc++/functors/slot.h:114
#15 0x00007ffff459ceb2 in ?? () from /usr/lib/x86_64-linux-gnu/libglibmm-2.4.so.1
#16 0x00007ffff542adce in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#17 0x00007ffff542a3d5 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#18 0x00007ffff1434494 in start_thread (arg=0x7fffddd60700) at pthread_create.c:333
#19 0x00007ffff1176aff in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:97
(gdb) frame 7
#7  0x0000555556095fc8 in rtengine::procparams::ProcParams::load (this=0x7fffb40037d8, fname=..., pedited=0x0) at /home/user/src/rawtherapee/rtengine/procparams.cc:8230
8230            printf ("-->%s\n", e.what().c_str());
(gdb) print fname 
$1 = (const Glib::ustring &) @0x7fffddd5f570: {static npos = 18446744073709551615, string_ = "/home/user/y/20121111-151416-20121111-151428.tif.pp3"}

Maybe ditch that printf()?

[agriggio]

let's just put a file exists check before calling the keyfile parser

[agriggio]

Here's a patch. If no objections, I'll push soon.

diff --git a/rtengine/procparams.cc b/rtengine/procparams.cc
--- a/rtengine/procparams.cc
+++ b/rtengine/procparams.cc
@@ -3383,7 +3383,8 @@
             pedited->set (false);
         }

-        if (!keyFile.load_from_file(fname)) {
+        if (!Glib::file_test(fname, Glib::FILE_TEST_EXISTS) ||
+            !keyFile.load_from_file(fname)) {
             return 1;
         }

[agriggio]

well I went ahead and pushed :-)