Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leak when saving 32-bit TIFF #4375

Closed
Beep6581 opened this issue Feb 7, 2018 · 12 comments
Closed

Memory leak when saving 32-bit TIFF #4375

Beep6581 opened this issue Feb 7, 2018 · 12 comments
Labels
priority: critical Urgently needs fixing type: bug Something is not doing what it's supposed to be doing
Milestone

Comments

@Beep6581
Copy link
Owner

Beep6581 commented Feb 7, 2018

Several times my whole computer froze (I managed to unfreeze it once by switching to TTY1 and killing RT) when testing patches and saving to various formats including 32-bit TIFF. I didn't encounter these freezes when saving only to 16-bit TIFF so the problem probably involves 32-bit TIFF files.

Steps to reliably reproduce:

  • Open amsterdam.pef with Filmstrip visible, neutral profile.
  • Open the same folder in the File Browser which you will be saving to, I used /tmp
  • Save a 32-bit uncompressed TIFF.

screenshot_20180207_114141

@Beep6581 Beep6581 added type: bug Something is not doing what it's supposed to be doing priority: critical Urgently needs fixing labels Feb 7, 2018
@Beep6581 Beep6581 added this to the v5.4 milestone Feb 7, 2018
@Beep6581
Copy link
Owner Author

Beep6581 commented Feb 7, 2018

@Floessie
Copy link
Collaborator

Floessie commented Feb 7, 2018

Does the file browser need to have the output folder open?

Yes. It's the reading that triggers the bug.

Does the TIFF need to be compressed?

Don't know, I used "uncompressed".

Are any metadata changes necessary?

I didn't do anything to metadata myself.

Maybe you need a slow or otherwise busy machine to trigger it.

HTH,
Flössie

@Beep6581
Copy link
Owner Author

Beep6581 commented Feb 7, 2018

Reproduced, first post updated.

@Beep6581
Copy link
Owner Author

Beep6581 commented Feb 7, 2018

Could not reproduce using @Floessie 's patch:

diff --git a/rtexif/rtexif.cc b/rtexif/rtexif.cc
index 47d885e50..b6a60bbd8 100644
--- a/rtexif/rtexif.cc
+++ b/rtexif/rtexif.cc
@@ -3077,7 +3077,7 @@ void ExifManager::parse (bool isRaw, bool skipIgnored)
         root->printAll ();
 #endif
 
-    } while (ifdOffset && !onlyFirst);
+    } while (ifdOffset > 0 && !onlyFirst);
 
     // Security check : if there's at least one root, there must be at least one image.
     // If the following occurs, then image detection above has failed or it's an unsupported file type.

@Beep6581
Copy link
Owner Author

Beep6581 commented Feb 7, 2018

@Floessie if you're ok to commit, I could make a new coverity build right away.

@Floessie
Copy link
Collaborator

Floessie commented Feb 7, 2018

@Beep6581 Please go ahead and push the fix. But why the new Coverity trigger: This is an old bug and I bet it was in the Coverity log, so this won't reveal anything new. Wouldn't it make more sense to tackle #4376 first?

@Beep6581
Copy link
Owner Author

Beep6581 commented Feb 7, 2018

@Floessie only because you (or someone else?) asked me to make a new one two or three days ago.

@Floessie
Copy link
Collaborator

Floessie commented Feb 7, 2018

@Beep6581 In fact, it was me. That was because of the newly added code, but we don't have to trigger a scan for single line changes...

@Hombre57
Copy link
Collaborator

Hombre57 commented Feb 7, 2018

@Floessie I'd be interested to understand your patch : what's the difference between the suppressed line and the new one ?

I declared ifdOffset as int, while IFDOffset is unsigned int (as told in the specification). I'm coping one into the other here. It might be a problem, but then I don't know what your patch solve if ifdOffset can't be negative.

@Floessie
Copy link
Collaborator

Floessie commented Feb 7, 2018

@Hombre57 The new one breaks the loop, if ifdOffset is negative, obviously. 😄

It might be a problem, but then I don't know what your patch solve if ifdOffset can't be negative.

Why can't ifdOffset be negative? It's signed. And it will be -1, if the fread() in get4() fails, because the fseek() prior to the read failed, too. This can happen, when the file is just being written (or if it's maliciously crafted, the whole rtexif code is very susceptible for that).

I'm coping one into the other here.

If a similar loop is copied from here, it should be fixed there, too.

I hope, this explanation makes things clearer. It's just a common unsigned vs. signed bug. If an entity should only be positive (like absolute offsets or sizes), make them unsigned. I didn't want to touch too much code in this phase, so I opted for changing a single line. Converting all those offsets to size_t would be the correct solution, as we don't support offsets beyond 2GB right now. Edit: Making it unsigned wouldn't have helped, what's needed is a real error flag or an exception.

Best,
Flössie

@Beep6581
Copy link
Owner Author

Beep6581 commented Feb 7, 2018

we don't have to trigger a scan for single line changes

We don't. The last scan was 10 days and 68 commits ago, and I will make a new one for RC1.

@Floessie
Copy link
Collaborator

Floessie commented Feb 7, 2018

The last scan was 10 days and 68 commits ago, and I will make a new one for RC1.

Ah, okay, I thought that was already done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: critical Urgently needs fixing type: bug Something is not doing what it's supposed to be doing
Projects
None yet
Development

No branches or pull requests

3 participants