Abrt in FXMEM_DefaultFree |
|||||||||
Issue descriptionDetailed 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.
,
Nov 2 2017
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.
,
Nov 2 2017
Is this really a libtiff bug?
,
Nov 3 2017
,
Nov 3 2017
The failure is: DCHECK(PartitionPageGetRawSize(page) == 0); Do we have a PartitionAlloc vs. other allocator mismatch?
,
Nov 3 2017
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.
,
Nov 3 2017
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?
,
Nov 3 2017
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.
,
Nov 3 2017
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.
,
Nov 3 2017
Looks like thestig is on the case. :) Thank you!
,
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
,
Nov 3 2017
,
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
,
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.
,
Nov 4 2017
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.
,
Nov 7 2017
,
Nov 7 2017
,
Nov 7 2017
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by ClusterFuzz
, Nov 2 2017Labels: Test-Predator-AutoComponents