tif_dirread.c memory leak

[Kurt Schwehr]

tif_dirread.c

@@ -4577 +4577 @@
                         dircount16 = (uint16)dircount64;
                         dirsize = 20;
                 }
+                if (dircount16 == 0) {
+                  // don't attempt to malloc (and later leak) anything if dircount16 is 0
+                  // (which is the return error code).
+                   TIFFErrorExt(tif->tif_clientdata, module,
+                                "Invalid zero directory count");
+                   return 0;
+                }
                 origdir = _TIFFCheckMalloc(tif, dircount16,
                     dirsize, "to read TIFF directory");
                 if (origdir == NULL)

[Even Rouault]

Kurt, could explain more why this is necessary / add a file that exhibits the issue. _TIFFCheckMalloc() should return NULL if dircount16 == 0. I don't see what would be leaked.

[Kurt Schwehr]

Pascal Massimino has an email stuck in moderation to the libtiff mailing list. He said that the fuzzer artifact isn't something we can share.

His general description:

If dircount16 == 0, TIFFFetchDirectory() will return an error (actually, dircount16 *is* the return code).

But before that, some calls to TIFFCheckRealloc() were made, leaving some 1-bytes buffers dandling behind.

The early exit code will now make sure we abort at once, before any extra allocation.

[Even Rouault]

I'm still not convinced by your explanations. And simulating the issue doesn't show any memory leak with Valgrind

Index: frmts/gtiff/libtiff/tif_dirread.c
===================================================================
--- frmts/gtiff/libtiff/tif_dirread.c	(révision 39453)
+++ frmts/gtiff/libtiff/tif_dirread.c	(copie de travail)
@@ -4577,6 +4577,7 @@
 			dircount16 = (uint16)dircount64;
 			dirsize = 20;
 		}
+		dircount16 = 0; // simulation
 		origdir = _TIFFCheckMalloc(tif, dircount16,
 		    dirsize, "to read TIFF directory");
 		if (origdir == NULL)

$ valgrind --leak-check=full gdalinfo byte.tif
==11623== Memcheck, a memory error detector
==11623== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==11623== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==11623== Command: gdalinfo byte.tif
==11623== 
ERROR 1: byte.tif:Failed to allocate memory for to read TIFF directory (0 elements of 12 bytes each)
ERROR 1: TIFFReadDirectory:Failed to read directory at offset 408
gdalinfo failed - unable to open 'byte.tif'.
==11623== 
==11623== HEAP SUMMARY:
==11623==     in use at exit: 86,568 bytes in 12 blocks
==11623==   total heap usage: 11,559 allocs, 11,547 frees, 1,649,454 bytes allocated
==11623== 
==11623== LEAK SUMMARY:
==11623==    definitely lost: 0 bytes in 0 blocks
==11623==    indirectly lost: 0 bytes in 0 blocks
==11623==      possibly lost: 0 bytes in 0 blocks
==11623==    still reachable: 86,568 bytes in 12 blocks
==11623==         suppressed: 0 bytes in 0 blocks
==11623== Reachable blocks (those to which a pointer was found) are not shown.
==11623== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==11623== 
==11623== For counts of detected and suppressed errors, rerun with: -v
==11623== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

[Even Rouault]

Kurt, have you checked with Pascal regarding my above comments if this issue is still valid ?

[Kurt Schwehr]

Pascal has been out of the office since the initial bug report.

[Even Rouault]

@schwehr Is this still current ? (and was this report with a recent libtiff version? The behaviour of TIFFCheckMalloc() changed at some point regarding 0 size allocation)

[Even Rouault]

Closing as "works for me". Re-open if you can show a Valgrind or MSAN log that actually mentions a memory leak ;-)