Hi,
Similar memory leak is in tiff loading function. I am sending test cases as
files for both tiff and png.
This is because buffer will not be deleted in case of exception between new
and delete:
*char* *buffer = *new* *char*[bufferSize];
*if*( !buffer )
{
TIFFClose(hInTiffHandle);
PODOFO_RAISE_ERROR( ePdfError_OutOfMemory );
}
*for*(row = 0; row < height; row++)
{
*if*(TIFFReadScanline(hInTiffHandle,
&buffer[row * scanlineSize],
row) == (-1))
{
TIFFClose(hInTiffHandle);
PODOFO_RAISE_ERROR( ePdfError_UnsupportedImageFormat );
}
}
PdfMemoryInputStream stream(buffer, bufferSize);
SetImageData(*static_cast*<*unsigned* *int*>(width),
*static_cast*<*unsigned* *int*>(height),
*static_cast*<*unsigned* *int*>(bitsPerSample),
&stream);
*delete*[] buffer;
Here is output from valgrind and clang memory sanitizer:
==17391== 8,192 bytes in 1 blocks are definitely lost in loss record 1,202
of 1,257
==17391== at 0x4C3089F: operator new[](unsigned long) (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==17391== by 0x970753: PoDoFo::PdfImage::LoadFromTiffHandle(void*)
(PdfImage.cpp:638)
==17391== by 0x9711ED: PoDoFo::PdfImage::LoadFromTiffData(unsigned char
const*, long) (PdfImage.cpp:838)
==7771==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 8192 byte(s) in 1 object(s) allocated from:
#0 0x55f796949e05 in operator new[](unsigned long)
/checkout/src/llvm-project/compiler-rt/lib/lsan/lsan_interceptors.cc:231:37
#1 0x55f796eb7ec5 in PoDoFo::PdfImage::LoadFromTiffHandle(void*)
podofo/doc/PdfImage.cpp:638
#2 0x55f796eb895f in PoDoFo::PdfImage::LoadFromTiffData(unsigned char
const*, long) podofo/doc/PdfImage.cpp:838
On Tue, Nov 5, 2019 at 8:17 PM Michal Sudolsky <[email protected]> wrote:
> If function png_read_image encounters error it will call longjmp and will
> jump to place where was called setjmp. In such case pBuffer and pRows will
> never be deallocated.
>
> Cut of code from mentioned functions:
> ```
> // Read the file
> if( setjmp(png_jmpbuf(pPng)) )
> {
> png_destroy_read_struct(&pPng, &pInfo, (png_infopp)NULL);
> PODOFO_RAISE_ERROR( ePdfError_InvalidHandle );
> }
>
> long lLen = static_cast<long>(png_get_rowbytes(pPng, pInfo) * height);
> char* pBuffer = static_cast<char*>(podofo_calloc(lLen, sizeof(char)));
> ...
> png_bytepp pRows = static_cast<png_bytepp>(podofo_calloc(height,
> sizeof(png_bytep)));
> ...
> png_read_image(pPng, pRows);
> ...
> podofo_free(pBuffer);
> podofo_free(pRows);
> ```
>
> Test example:
> ```
> // include podofo, vector, etc
> int main()
> {
> PdfMemDocument doc;
> PdfImage image(&doc);
> std::vector<unsigned char> corrupt_png = {
> 0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A,
> 0x00, 0x00, 0x00, 0x0D,
> 'I', 'H', 'D', 'R',
> 0x00, 0x00, 0x04, 0x00,
> 0x00, 0x00, 0x05, 0xA9,
> 0x08, 0x02, 0x00, 0x00, 0x00,
> 0x68, 0x1B, 0xF7, 0x46,
> 0x00, 0x00, 0x00, 0x00,
> 'I', 'D', 'A', 'T',
> 0x35, 0xAF, 0x06, 0x1E,
> 0x00, 0x00, 0x00, 0x00,
> 'I', 'E', 'N', 'D',
> 0xAE, 0x42, 0x60, 0x82
> };
> try
> {
> image.LoadFromPngData(corrupt_png.data(), corrupt_png.size());
> }
> catch(PdfError &e)
> {
> e.PrintErrorMsg();
> }
> return 0;
> }
> ```
>
> Test for leaks using valgrind (libpng outputs errors to stderr as it is
> not muted in podofo like libjpeg and libtiff):
> ```
> libpng error: Not enough image data
>
> PoDoFo encountered an error. Error: 2 ePdfError_InvalidHandle
> Error Description: A NULL handle was passed, but initialized data was
> expected.
> Callstack:
> #0 Error Source: podofo/doc/PdfImage.cpp:1142
>
> ==97240==
> ==97240== HEAP SUMMARY:
> ==97240== in use at exit: 4,462,920 bytes in 2 blocks
> ==97240== total heap usage: 128 allocs, 126 frees, 4,567,527 bytes
> allocated
> ==97240==
> ==97240== 4,462,920 (11,592 direct, 4,451,328 indirect) bytes in 1 blocks
> are definitely lost in loss record 2 of 2
> ==97240== at 0x4C31B25: calloc (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==97240== by 0x1759CE: PoDoFo::podofo_calloc(unsigned long, unsigned
> long)
> ==97240== by 0x1E0EE4: PoDoFo::PdfImage::LoadFromPngData(unsigned char
> const*, long)
> ==97240== by 0x125610: main
> ==97240==
> ==97240== LEAK SUMMARY:
> ==97240== definitely lost: 11,592 bytes in 1 blocks
> ==97240== indirectly lost: 4,451,328 bytes in 1 blocks
> ==97240== possibly lost: 0 bytes in 0 blocks
> ==97240== still reachable: 0 bytes in 0 blocks
> ==97240== suppressed: 0 bytes in 0 blocks
> ```
>
> There is png image which advertises that it has dimensions 1024x1449 and
> RGB colorspace so after decompression it will take 1024 * 1449 * 3 =
> 4,451,328 bytes of memory as pBuffer. It has height 1449 so there are
> allocated 1449 row pointers which takes 1449 * 8 = 11,592 bytes as pRows.
> Buffer pBuffer is reported as indirect loss because there are pointers
> in pRows which points to it.
>
> This leak is especially dangerous in server applications that uses podofo
> as library. Let say such application takes as inputs from network pdf and
> image then inserts image into pdf and sends resulting pdf back. Attacker
> can repeatedly send crafted png images like in test example above to
> exhaust memory and cause crash.
>
>
> Also line "if( setjmp(png_jmpbuf(pPng)) )" is probably undefined behaviour
> (there should be comparison with integer constant):
>
> https://en.cppreference.com/w/cpp/utility/program/setjmp
>
>
> Also function pngReadData should call png_error in case it cannot read
> requested length of data but instead it will read only available data or do
> nothing (if _size == _pos):
>
> ```
> if (length > _size - _pos)
> {
> memcpy(data, &_data[_pos], _size - _pos);
> _pos = _size;
> }
> else
> {
> memcpy(data, &_data[_pos], length);
> _pos += length;
> }
> ```
>
>
> Also are you sure that throwing C++ exceptions from function which is
> called from C code in another library (libjpeg) is not undefined behaviour
> or something? Maybe in PdfImage::LoadFromJpegData should be used
> setjmp/longjmp mechanism too:
>
> ```
> extern "C" {
> void JPegErrorExit(j_common_ptr cinfo)
> {
> #if 1
> char buffer[JMSG_LENGTH_MAX];
>
> /* Create the message */
> (*cinfo->err->format_message) (cinfo, buffer);
> #endif
> jpeg_destroy(cinfo);
> PODOFO_RAISE_ERROR_INFO( ePdfError_UnsupportedImageFormat, buffer);
> }
> ```
>
>
_______________________________________________
Podofo-users mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/podofo-users
