Tk Source Code

View Ticket
Login
Ticket UUID: b601ce3ab167ad1dc2bd3951e97a8c33f9590c94
Title: A corrupted image can cause resource exhaustion
Type: Bug Version: 8.6.4
Submitter: kjnash Created on: 2016-01-09 05:23:03
Subsystem: 41. Photo Images Assigned To: fvogel
Priority: 6 Severity: Severe
Status: Closed Last Modified: 2017-08-02 12:21:10
Resolution: Fixed Closed By: fvogel
    Closed on: 2017-08-02 12:21:10
Description:
A corrupted image can cause Tk to hang, and to use a huge amount of memory, leading to thrashing and resource exhaustion.

This occurs when running the attached file from the command line on up-to-date CentOS 6 x86_64.

Tested with ActiveTcl8.6.4.1.299124-linux-x86_64-threaded
Same result with ActiveTcl8.5.18.0.298892-linux-x86_64-threaded

Occasionally the script fails with an appropriate error message.  Usually it continues running, demanding an increasing amount of memory.
User Comments: fvogel added on 2017-08-02 12:21:10:
Merged to core-8-5-branch, core-8-6-branch and trunk.

Thanks again, Keith!!

fvogel added on 2017-08-02 10:50:21:
I have prompted Simon Bachmann about the proposed patch for a second opinion, since he knows that part of Tk very well thanks to his work on TIP-166.

He sent me positive feedback. Merge can happen IMO.

fvogel added on 2017-06-13 19:40:38:

This is now looking very fine for me on Windows.

I have run the test suite after forcing

  testConstraint SegfaultOn8.5 1
in tests/imbPhoto.c:46 so that the new tests are exercised, and they all pass. However I didn't exercise the tests that require the base64 package.

There is no new failure (actually, no failure in the test suite at all) compared to core-8-5-branch.


kjnash added on 2017-06-13 01:03:27:
If (as I hope) bug-b601ce3ab1 is merged with core-8-5-branch, and then the changes are merged with core-8-6-branch and trunk, note that the last two do not need any changes to the file "generic/tkImgPhoto.c".

The changes to the other files (generic/tkImgGIF.c and tests/imgPhoto.test) should be simple to merge.

kjnash added on 2017-06-12 12:33:05:
Commit [553ddb8e] to branch bug-b601ce3ab1 has revised tests/imgPhoto.test, which should work on all platforms.

kjnash added on 2017-06-03 09:28:46:
Thank you for your perseverance!

I now see that the error message, and hence the test results, are platform-dependent.  On 64-bit Linux and Windows, the tests all passed (unless they were skipped for base64PackageNeeded). On 32-bit Windows I get the same test results as you.

I have modified tests/imgPhoto.test so that tests that expect an error do not check the precise error message, which is unimportant.

This patch against 571fff1a is patch-gif-tests-8.5.patch.  Please also install the images from BadGIF-test-images.tar.gz.

I have done the same tests on the trunk (Tcl 08a3bc6b34, Tk a7e1ae03).  The original patch BadGIF-trunk-88df9a16.patch can be applied (with one hunk in tests/imgPhoto.test having an offset), along with the images from BadGIF-test-images.tar.gz.  The results are similar to those for core-8-5-branch, with different error messages on 32-bit Windows and 64-bit Linux.

Again, I have modified tests/imgPhoto.test so that tests that expect an error do not check the precise error message, which is unimportant.

This second patch against a7e1ae03 is patch-gif-tests-trunk.patch.

The two patches and image tarball for the trunk can also be applied to core-8-6-branch.



== Results with tests/imgPhoto.test from Tk 571fff1a ==

Building against Tcl 0055a16a8b from core-8-5-branch, I tried Tk 571fff1a from branch bug-b601ce3ab1.

Editing tests/imgPhoto.test to set 
    testConstraint SegfaultOn8.5 1
I find:

================
(1) 64-bit Linux
================

$ make test-classic TESTFLAGS="-verbose bps -file ../../Tk_Source_Code-571fff1a/tests/imgPhoto.test"

<snip>

++++ imgPhoto-18.1 SKIPPED: base64PackageNeeded
++++ imgPhoto-18.2 PASSED
++++ imgPhoto-18.3 PASSED
++++ imgPhoto-18.4 SKIPPED: base64PackageNeeded
++++ imgPhoto-18.5 PASSED
++++ imgPhoto-18.6 PASSED
++++ imgPhoto-18.7 SKIPPED: base64PackageNeeded
++++ imgPhoto-18.8 PASSED
++++ imgPhoto-18.9 PASSED
++++ imgPhoto-18.10 SKIPPED: base64PackageNeeded
++++ imgPhoto-18.11 PASSED
++++ imgPhoto-18.12 PASSED

Tests ended at Sat Jun 03 07:07:31 BST 2017
all.tcl:        Total   122     Passed  117     Skipped 5       Failed  0
Sourced 1 Test Files.
Number of tests skipped for each constraint:
        4       base64PackageNeeded
        1       nonPortable


====================
(2) 64-bit Windows 7, Tcl/Tk built as 64-bit windows version using MinGW-W64
====================

$ make test-classic TESTFLAGS="-verbose bps -file ../../Tk_Source_Code-571fff1a/tests/imgPhoto.test"

<snip>

++++ imgPhoto-18.1 SKIPPED: base64PackageNeeded
++++ imgPhoto-18.2 PASSED
++++ imgPhoto-18.3 PASSED
++++ imgPhoto-18.4 SKIPPED: base64PackageNeeded
++++ imgPhoto-18.5 PASSED
++++ imgPhoto-18.6 PASSED
++++ imgPhoto-18.7 SKIPPED: base64PackageNeeded
++++ imgPhoto-18.8 PASSED
++++ imgPhoto-18.9 PASSED
++++ imgPhoto-18.10 SKIPPED: base64PackageNeeded
++++ imgPhoto-18.11 PASSED
++++ imgPhoto-18.12 PASSED

Tests ended at Sat Jun 03 07:10:57 +0100 2017
all.tcl:        Total   122     Passed  117     Skipped 5       Failed  0
Sourced 1 Test Files.
Number of tests skipped for each constraint:
        4       base64PackageNeeded
        1       nonPortable
stderr32[64bit win]$


====================
(3) 64-bit Windows 7, Tcl/Tk built as 32-bit using MinGW-W64
====================

$ make test-classic TESTFLAGS="-verbose bps -file ../../Tk_Source_Code-571fff1a/tests/imgPhoto.test"

<snip>

++++ imgPhoto-18.1 SKIPPED: base64PackageNeeded


==== imgPhoto-18.2 Reject corrupted GIF (base 64 string) FAILED
==== Contents of test case:

    image create photo gif1 -data $data

---- Result was:
not enough free memory for image buffer
---- Result should have been (exact matching):
error reading color map
==== imgPhoto-18.2 FAILED



==== imgPhoto-18.3 Reject corrupted GIF (file) FAILED
==== Contents of test case:

    image create photo gif1 -file $fileName

---- Result was:
not enough free memory for image buffer
---- Result should have been (exact matching):
error reading color map
==== imgPhoto-18.3 FAILED

++++ imgPhoto-18.4 SKIPPED: base64PackageNeeded
++++ imgPhoto-18.5 PASSED
++++ imgPhoto-18.6 PASSED
++++ imgPhoto-18.7 SKIPPED: base64PackageNeeded
++++ imgPhoto-18.8 PASSED
++++ imgPhoto-18.9 PASSED
++++ imgPhoto-18.10 SKIPPED: base64PackageNeeded
++++ imgPhoto-18.11 PASSED
++++ imgPhoto-18.12 PASSED

Tests ended at Sat Jun 03 07:41:55 +0100 2017
all.tcl:        Total   122     Passed  115     Skipped 5       Failed  2
Sourced 1 Test Files.
Files with failing tests: imgPhoto.test
Number of tests skipped for each constraint:
        4       base64PackageNeeded
        1       nonPortable

fvogel added on 2017-06-02 20:59:28:

I have applied your patch in a new branch bug-b601ce3ab1 derived from core-8-5-branch. I had to apply it manually (copy/paste lines).

Running the test suite with the patched imgPhoto.test, several new tests are skipped because the SegfaultOn8.5 (new) constraint is not (yet) satisfied. This is expected because the branch is not yet labelled 8.5.20.

However forcing this constraint to true in imgPhoto.test I get the following output for the new tests:

++++ imgPhoto-18.1 SKIPPED: base64PackageNeeded


==== imgPhoto-18.2 Reject corrupted GIF (base 64 string) FAILED
==== Contents of test case:

    image create photo gif1 -data $data

---- Result was:
not enough free memory for image buffer
---- Result should have been (exact matching):
error reading color map
==== imgPhoto-18.2 FAILED



==== imgPhoto-18.3 Reject corrupted GIF (file) FAILED
==== Contents of test case:

    image create photo gif1 -file $fileName

---- Result was:
not enough free memory for image buffer
---- Result should have been (exact matching):
error reading color map
==== imgPhoto-18.3 FAILED

++++ imgPhoto-18.4 SKIPPED: base64PackageNeeded
++++ imgPhoto-18.5 PASSED
++++ imgPhoto-18.6 PASSED
++++ imgPhoto-18.7 SKIPPED: base64PackageNeeded
++++ imgPhoto-18.8 PASSED
++++ imgPhoto-18.9 PASSED
++++ imgPhoto-18.10 SKIPPED: base64PackageNeeded
++++ imgPhoto-18.11 PASSED
++++ imgPhoto-18.12 PASSED

What do you think?


kjnash added on 2017-06-02 18:57:38:
A GIF file's header tells the computer how large the image is.  This data can be corrupted.  The header of the supplied GIF describes an image that requires over 3GB of memory.  I should have mentioned that the file must be read as UTF-8.

When I first reported the bug I was using a machine with "only" 4GB of memory - hence the hanging and resource exhaustion.  On a machine with 16GB of memory, the bug is not so severe - it simply allocates the memory, and reads the image.  Then Tk realises that the image is corrupt.

I have just tested the bug again with Tk 8.6.6 on Windows 7 with 4GB of memory, and Windows Task Manager shows the process slowly reaching 2.7GB of memory before an sequence of error boxes pop up indicating insufficient memory.  At the second attempt (in a new interpreter) the process exceeds 3GB and I get the correct error message "no image data for this index".

In the example script, the image data is less than 1kB, and Tk should stop reading when it has reached the end of the read buffer.  Instead it appears to allocate enough memory for the copy, and continues copying from adjacent memory until it has read the amount specified in the GIF header.

I suggest applying the patch to 8.6, because without it a corrupted or malicious GIF can waste considerable resources, and will have unpredicable consequences on smaller systems.

The patch supplied for the trunk is still good - the relevant files in commit 707912f8ac have been modified since the patch was produced, but the changes are compatible.

I also suggest applying the patch to 8.5: the patch exists, 8.5 is not yet deprecated, and it is still receiving updates.

The patch for 8.5 is still good - the relevant files in commit b901466690 (core-8-5-branch) have not been modified since the patch was produced.

fvogel added on 2017-06-01 18:40:16:

With current trunk, on Windows 7, when running the provided demo script (without applying the patch), I apparently always get the error message:

    no image data for this index

I can notice momory use jumps to 357 Mb during execution of the demo script. However the script does not hang for me (it returns in no visible time), memory is immediately released after script end, and it spits what seems to be a regular error message.

In short: I don't seem to be able to replicate the issue. Was this bug perhaps already fixed (probably when fixing another bug, because the provided patch really was not applied)?


fvogel added on 2017-04-09 08:37:47:
Thanks for the precise report, and even more for the patch!

I'm unsure 8.5 should be patched since its end of life there.

What about 8.6 however?

kjnash added on 2017-04-08 13:32:31:

The attached files are a fix for this bug.

BadGIF-trunk-88df9a16.patch

  • this is a patch for the trunk with changes to files:
    • generic/tkImgGIF.c (one line added)
      • The extra line fixes a test for the end of a read buffer, which caused a memcpy to continue beyond the end of the read buffer (but not beyond the end of the write buffer).
    • tests/imgPhoto.test (new tests)

BadGIF-core-8-5-branch-b20b6d95.patch
  • this is a patch for core-8-5-branch with changes to files:
    • generic/tkImgGIF.c (one line added, same as for trunk)
    • generic/tkImgPhoto.c - several lines backported from trunk:
      • tests for values that will overflow (int) or (uint) arithmetic
      • avoiding (int) overflow by casting multiplicands to (size_t), instead of doing the multiplication as (int) and then casting the result
    • tests/imgPhoto.test (new tests)

BadGIF-test-images.tar.gz
  • this is a tarball of test images tests/*.gif used by the patched tests/imgPhoto.test in both the trunk and core-8-5-branch


Attachments: