New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 780709 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

Abrt in FXMEM_DefaultFree

Project Member Reported by ClusterFuzz, Nov 2 2017

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=5274503591755776

Fuzzer: libFuzzer_pdf_codec_tiff_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Abrt
Crash Address: 0x03e900003f24
Crash State:
  FXMEM_DefaultFree
  TIFFFetchStripThing
  TIFFReadDirectory
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=508791:508824

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5274503591755776

Issue filed automatically.

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.
 
Project Member

Comment 1 by ClusterFuzz, Nov 2 2017

Components: Internals>Plugins>PDF
Labels: Test-Predator-AutoComponents
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.
Cc: msrchandra@chromium.org pnangunoori@chromium.org
Labels: M-64 Test-Predator-Wrong
Owner: palmer@chromium.org
Status: Assigned (was: Untriaged)
Predator and CL could not provide any possible suspects.
Using the code search for the file, “fx_memory.cpp” assigning to concern owner from GIT blame.
Suspecting Commit#
https://pdfium.googlesource.com/pdfium.git/+/7726a581626bbb72d6ab294ae1adbad4ca10dfb0

@palmer -- Could you please look into this issue, kindly reassign if it has nothing to do with your changes.
Thank You.

Cc: npm@chromium.org
Is this really a libtiff bug?
Cc: kkaluri@chromium.org
 Issue 781083  has been merged into this issue.
The failure is: DCHECK(PartitionPageGetRawSize(page) == 0);

Do we have a PartitionAlloc vs. other allocator mismatch?
Oh, the libtiff code is trying to do free(NULL). Shouldn't PartitionFree(ptr) handle this and do nothing if its |ptr| argument is NULL? The malloc(3) manpage here says:

If ptr is NULL, no operation is performed.

I feel like we had this discussion before, but I can't remember the outcome.
We have had this discussion before, but I also can't remember the outcome. :) I think I like PA's don't-accept-NULL view for (a) avoiding a branch and (b) enforcing that callers know what they're doing. To accomodate callers that don't know what they're doing, I'd rather have a trivial wrapper. I can add one to PA, or we can add it somewhere in PDFium or in libtiff. I'm not sure what's best. Thoughts?
Right, and I'm in the "no need for the caller to check" camp, because free() traditionally does that check already. So if the point is not NULL, the code ends up doing 2 checks for the same thing.

Since libtiff is third party code, I prefer to change that as little as possible. Though _TIFFfree() is implemented in core/fxcodec/codec/ccodec_tiffmodule.cpp, so that's a convenient place to add the check.
Sent you a CL.

I think we had the discussion on https://pdfium-review.googlesource.com/6552 before. There is a TODO in FX_Free() with a comment regarding this topic.
Cc: palmer@chromium.org
Owner: thestig@chromium.org
Status: Started (was: Assigned)
Looks like thestig is on the case. :) Thank you!
Project Member

Comment 11 by bugdroid1@chromium.org, Nov 3 2017

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/882a190807b6f791e3b3f1a692c59fb93d746f94

commit 882a190807b6f791e3b3f1a692c59fb93d746f94
Author: Lei Zhang <thestig@chromium.org>
Date: Fri Nov 03 20:35:36 2017

Check for NULL pointer in _TIFFfree().

PartitionAlloc does not handle free(NULL), so _TIFFfree() needs to do
the check, just like png_free_default() and _cmsFree() for other third
party code.

BUG= chromium:780709 

Change-Id: I4e2ff0ba642c66e4a73e151c9ab42ebb42d34a5b
Reviewed-on: https://pdfium-review.googlesource.com/17791
Reviewed-by: Chris Palmer <palmer@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>

[modify] https://crrev.com/882a190807b6f791e3b3f1a692c59fb93d746f94/core/fxcodec/codec/ccodec_tiffmodule.cpp

Status: Fixed (was: Started)
Project Member

Comment 13 by bugdroid1@chromium.org, Nov 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/19b5af15457e3901f7ea3c5e7bff99ff54c5bd00

commit 19b5af15457e3901f7ea3c5e7bff99ff54c5bd00
Author: pdfium-deps-roller@chromium.org <pdfium-deps-roller@chromium.org>
Date: Sat Nov 04 00:16:01 2017

Roll src/third_party/pdfium/ ff63663ce..882a19080 (2 commits)

https://pdfium.googlesource.com/pdfium.git/+log/ff63663ce08c..882a190807b6

$ git log ff63663ce..882a19080 --date=short --no-merges --format='%ad %ae %s'
2017-11-03 thestig Check for NULL pointer in _TIFFfree().
2017-11-03 thestig Simplify CPDF_SecurityHandler.

Created with:
  roll-dep src/third_party/pdfium
BUG= 780709 


The AutoRoll server is located here: https://pdfium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


TBR=dsinclair@chromium.org

Change-Id: I6a8b9c8df133be7d34f4a114b4fffd08691df754
Reviewed-on: https://chromium-review.googlesource.com/753426
Reviewed-by: <pdfium-deps-roller@chromium.org>
Commit-Queue: <pdfium-deps-roller@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513982}
[modify] https://crrev.com/19b5af15457e3901f7ea3c5e7bff99ff54c5bd00/DEPS

Project Member

Comment 14 by ClusterFuzz, Nov 4 2017

ClusterFuzz has detected this issue as fixed in range 513933:513982.

Detailed report: https://clusterfuzz.com/testcase?key=5274503591755776

Fuzzer: libFuzzer_pdf_codec_tiff_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Abrt
Crash Address: 0x03e900003f24
Crash State:
  FXMEM_DefaultFree
  TIFFFetchStripThing
  TIFFReadDirectory
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=508791:508824
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=513933:513982

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5274503591755776

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 15 by ClusterFuzz, Nov 4 2017

Labels: ClusterFuzz-Verified
Status: Verified (was: Fixed)
ClusterFuzz testcase 4686767650504704 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Labels: Test-Predator-Auto-CC
Labels: -Test-Predator-AutoComponents Test-Predator-Auto-Components
Labels: -Test-Predator-Auto-CC

Sign in to add a comment