3

I am using giflib, and have discovered that if I open a new file before closing the previous one, all hell breaks loose.

  1. DGifOpen (file A)
  2. DGifSlurp (file A)
  3. DGifOpen (file B)
  4. DGifSlurp (file B) - fails with D_GIF_ERR_READ_FAILED

Which is remarkable, since I can verify with a custom read function that every read succeeds and returns the number of bytes requested.

But...

  1. DGifOpen (file A)
  2. DGifSlurp (file A)
  3. DGifClose (file A)
  4. DGifOpen (file B)
  5. DGifSlurp (file B) - works!

The documentation says giflib isre-entrant and thread-safe.

So why does it matter if there are multiple open files? GifFileType should be encapsulating all state; and there is no overlap between user-side data structures for file A and file B.

Update & related question: Is giflib intended to safely handle malicious input files, or is it the wrong tool for that scenario? I do not see any automated use of valgrind in the test system, and I can't seem to find the CI server for the project.

Update: This has nothing to do with overlapped files, but rather with the fact that giflib 5.1.2 only works when the heap is zeroed - uninitialized reads are causing the failure. Overlapping reads were just a way to trigger that.

To reproduce, download giflib 5.1.2, and

  1. Run ./autogen.sh
  2. Run make check
  3. Then cd util && cat ../pic/porsche.gif | valgrind .libs/lt-gifsponge
  4. Watch valgrind report the uninitialized read.
Lilith River
  • 16,204
  • 2
  • 44
  • 76

1 Answers1

2

I'm the giflib maintainer, and also the original author of the DGifSlurp() entry point.

The big change between 4.x and 5.x was the elimination of static storage from the library. The old API could not be re-entrant because image states had some static pointers.

You are using the API correctly and the overlapped multiple opens should work. But there might be a bug in the implementation - the code is very old and some of it hasn't had a close look in literally 20 years.

I'll work with you on this. First thing to do is figure out where the error is being thrown; there are something like 14 different exits that can set that code.

ESR
  • 115
  • 4
  • Thank you! I'm working on setting up a failing test. – Lilith River Mar 29 '16 at 20:23
  • It looks like there is heap corruption involved, so it may not be giflib's fault, just coincidence. It's 100% repeatable in a certain order in a certain test suite, but not elsewhere. – Lilith River Mar 29 '16 at 21:21
  • Not heap corruption, actually. -> "Conditional jump or move depends on uninitialised value(s)" at DGifSetupDecompress, DGifGetImageDesc, DGifSlurp. – Lilith River Mar 29 '16 at 21:26
  • Turns out my test isn't needed. Run `make check`, then `cd util`, then `cat ../pic/porsche.gif | valgrind .libs/lt-gifsponge` to see the uninitialized read. – Lilith River Mar 29 '16 at 21:30
  • 1
    Try the 5.1.4 version I just shipped. Changing some mallocs to callocs may have eliminated this problem. – ESR Apr 02 '16 at 16:12