search

Beep6581 / RawTherapee

A powerful cross-platform raw photo processing program
https://rawtherapee.com
GNU General Public License v3.0
2.88k stars 323 forks source link

Fixing some undefined behaviour #2261

Closed Beep6581 closed 9 years ago

Beep6581 commented 9 years ago

Originally reported on Google Code with ID 2277

Valgrind detected an invalid read when parsing exif.

Should be fixed with this patch.

Ingo

Reported by heckflosse@i-weyrich.de on 2014-03-04 13:05:24

Beep6581 commented 9 years ago 
Included another two changes to fix undefined behaviour.

Reported by heckflosse@i-weyrich.de on 2014-03-04 13:22:51

Beep6581 commented 9 years ago

Reported by heckflosse@i-weyrich.de on 2014-03-04 13:43:57

Beep6581 commented 9 years ago 
Included another fix.

Reported by heckflosse@i-weyrich.de on 2014-03-04 13:46:47

Beep6581 commented 9 years ago 
Another three fixes included

Reported by heckflosse@i-weyrich.de on 2014-03-04 14:23:03

Beep6581 commented 9 years ago 
Yet another fix included.

Reported by heckflosse@i-weyrich.de on 2014-03-04 16:34:01

Beep6581 commented 9 years ago 
Hi Ingo, what could be the test case for this?

Reported by michaelezra000 on 2014-03-04 16:56:55

Beep6581 commented 9 years ago 
Don't know. These are corrections for errors reported by valgrind. I checked them by
running valgrind again after I made the changes. But valgrind does only run on Linux.
So you've no chance to test something here. But having a look at the changes I made
wouldn't be bad ;-)

Ingo

Reported by heckflosse@i-weyrich.de on 2014-03-04 17:16:13

Beep6581 commented 9 years ago 
I could crash RT when opening raw file immediately after RT is loaded.
With this patch it no longer seems to be a problem:)!

Reported by michaelezra000 on 2014-03-04 17:42:11

Beep6581 commented 9 years ago 
Sounds too good to be true ;-)

Reported by heckflosse@i-weyrich.de on 2014-03-04 18:07:10

Beep6581 commented 9 years ago

Reported by heckflosse@i-weyrich.de on 2014-03-04 18:27:31

Beep6581 commented 9 years ago 
Included another fix.

Reported by heckflosse@i-weyrich.de on 2014-03-04 20:46:51

Beep6581 commented 9 years ago 
Included a fix for two memory leaks.

Reported by heckflosse@i-weyrich.de on 2014-03-04 21:57:46

Beep6581 commented 9 years ago 
Ingo, I made a search for "<=32768" and here is what came up. Do you think these are
also candidates?

curves.cc (2 matches)
269: for (int i=0; i<=32768; i++) { 
339: for (int i=0; i<=32768; i++) { 

improcfun.cc (4 matches)
917: for (int i=0; i<=32768; i++) {// 
1,117: for (int i=0; i<=32768; i++) {// 
1,797: for (int i=0; i<=32768; i++) {// 
2,008: for (int i=0; i<=32768; i++) {//

Reported by michaelezra000 on 2014-03-05 01:31:54

Beep6581 commented 9 years ago 
Michael, very good idea to search for this "<=" things. 

Yes, these are also candidates. Found some others after searching for "<=48000" and
"<=50000". 

Perhaps Jacques can tell us what's the best way to fix this <= things, because there
are at least two possible ways to fix them.

But in any case it's wrong, because the last value is not initialized.

Ingo

Reported by heckflosse@i-weyrich.de on 2014-03-05 11:47:12

Beep6581 commented 9 years ago 
Fixed another case of undefined behaviour and some memory leaks.

Reported by heckflosse@i-weyrich.de on 2014-03-05 18:43:24

Beep6581 commented 9 years ago 
Hi Ingo

Sorry for your email, I didn't had the time to answer yet, but I have investigated
a bite. I would discard your change in rtexif.cc and add the following line on line
632 of rtexif.cc:

if (!count) count = 1;

This will ensure that the tag will at least allocate 1 element corresponding to the
tag's type. That's quite strange too that the count value read from the file can be
zero, if someone has the time to look to the TIFF standard...

Anyway, nice debugging effort Ingo!

Reported by natureh.510 on 2014-03-05 19:51:56

Beep6581 commented 9 years ago 
Hi Hombre,

the solution you propose, won't work, because the problem was that valuesize was 1024
in that case, but the 1024th byte wasn't a string terminator. That's the reason why
I increased the size of the buffer by one and added the string terminator to avoid
this situation. I can send you the link to the file where that happened in case you
want to have a look.

Ingo

Reported by heckflosse@i-weyrich.de on 2014-03-05 20:11:11

Beep6581 commented 9 years ago 
It also happens that the count value is null, and it would lead to a new char[0], which
is part of the log you sent me i guess. Maybe adding both correction would be better,
since the +1 trick won't work for tag types wider than 1 byte.

Reported by natureh.510 on 2014-03-05 20:18:16

Beep6581 commented 9 years ago 
Here's the patch with the change mentioned in #16

Reported by heckflosse@i-weyrich.de on 2014-03-05 21:14:14

Beep6581 commented 9 years ago 
Actually it is more prudent to put "<" rather than "<=". but in the case 48000 or 50000
it does not matter because it is an open scale.

In any case Ingo is a great job that will allow to increase the stability.

:)

Reported by jdesmis on 2014-03-06 07:06:38

Beep6581 commented 9 years ago 
Jacques, have a look at improcfun.cc line 1173 to 1184:

    LUTf dCcurve(65536,0);
    LUTu hist16_CCAM(65536);
    bool chropC=false;
    float valc;

    if(pW!=1){//only with improccoordinator
        for (int i=0; i<48000; i++) {  //# 32768*1.414  approximation maxi for chroma
            valc = (double)i / 47999.0;
            dCcurve[i] = CLIPD(valc);
        }
        hist16_CCAM.clear();
    }

only the first 48000 values of dCcurve are initialized. The values at position 48000
to 65535 can have random values.

Then in line 1805 to 1813

        for (int i=0; i<=48000; i++) {//
            if (chropC) {
                float hvalc = dCcurve[i];
                int hic = (int)(255.0f*CLIPD(hvalc)); //
                histCCAM[hic] += hist16_CCAM[i] ;
            }
        }

the value at position 48000 will be accessed. It's no out of bound access, but it has
an undefined value.

Anyway I'll change them from <= to <

Ingo

Reported by heckflosse@i-weyrich.de on 2014-03-06 10:39:55

Beep6581 commented 9 years ago 
I wonder if this was the reason for pepper noise in ciecam02?

Reported by michaelezra000 on 2014-03-06 12:24:52

Beep6581 commented 9 years ago 
Ingo

You are right...
You can change <= to <

Reported by jdesmis on 2014-03-06 13:33:35

Beep6581 commented 9 years ago 
Been using issue2277_08.patch today and no issues.

Reported by entertheyoni on 2014-03-07 12:29:51

Beep6581 commented 9 years ago 
Committed to revision df5048e4db26

Reported by heckflosse@i-weyrich.de on 2014-03-11 10:50:46

Beep6581 commented 9 years ago

Reported by heckflosse@i-weyrich.de on 2014-03-12 14:34:42

Beep6581 commented 9 years ago 
Thank you Ingo!

Reported by entertheyoni on 2014-03-12 18:09:27

Beep6581 commented 9 years ago 
Reopened the issue to fix another invalid read in igv-demosaic, a small memory leak
in exif handling for sony/minolta cams and an access to uninitialized variables. Latter
didn't lead to undefined behaviour, but the fix avoids some calculations when Vignette
Filter or Graduated Filter is enabled and Vignetting Correction is disabled.

Ingo

Reported by heckflosse@i-weyrich.de on 2014-03-13 13:43:16

Beep6581 commented 9 years ago

Reported by heckflosse@i-weyrich.de on 2014-03-13 13:59:30

Beep6581 commented 9 years ago 
When adding a file to the queue, valgrind reported conditional jumps or moves in fprintf.
Fixed with the changes to options.h, I included in this patch.

Reported by heckflosse@i-weyrich.de on 2014-03-13 17:02:09

Beep6581 commented 9 years ago 
In #30 I meant 'Conditional jump or move depends on uninitialised value(s)'

Reported by heckflosse@i-weyrich.de on 2014-03-13 17:18:14

Beep6581 commented 9 years ago 
guess i feel obliged to cheer you guys on with this effort!

Reported by janrinze on 2014-03-13 22:16:24

Beep6581 commented 9 years ago 
Hi Ingo, Thanks for the next patch:)
I must confess though, I am not sure how to test it:)

Reported by michaelezra000 on 2014-03-14 02:40:05

Beep6581 commented 9 years ago 
Michael, I can't tell how to test this.
Committed to revision cb8d230d41e1, but without the change to igv-demosaic, because
it needs some more investigation.
Will reopen the issue when RT 4.1 is tagged.

Ingo

Reported by heckflosse@i-weyrich.de on 2014-03-14 19:47:02

Beep6581 commented 9 years ago

Reported by entertheyoni on 2014-03-16 17:17:15

Beep6581 commented 9 years ago

Reported by heckflosse@i-weyrich.de on 2014-03-16 20:37:42

Beep6581 commented 9 years ago 
Reopened to post another 4 fixes.

Reported by heckflosse@i-weyrich.de on 2014-03-17 15:28:43

Beep6581 commented 9 years ago 
Included another fix in the patch.

Reported by heckflosse@i-weyrich.de on 2014-03-17 21:26:21

Beep6581 commented 9 years ago 
Committed to revision 9d5b21abf3f8

Reported by heckflosse@i-weyrich.de on 2014-03-18 12:21:58

Beep6581 commented 9 years ago

Reported by heckflosse@i-weyrich.de on 2014-03-19 22:48:20

Beep6581 commented 9 years ago 
Reopened to post some new fixes to uninitialised values reported by valgrind when openeing
a new detail window.

Reported by heckflosse@i-weyrich.de on 2014-03-24 19:46:22

Beep6581 commented 9 years ago 
Committed to revision eea761e55548

Reported by heckflosse@i-weyrich.de on 2014-03-24 20:20:30

Beep6581 commented 9 years ago

Reported by heckflosse@i-weyrich.de on 2014-03-30 22:50:36