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

Issue 649242 link

Starred by 3 users

Issue metadata

Status: Archived
Owner:
Closed: Sep 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

quipper crash in _IO_new_fclose

Project Member Reported by djkurtz@chromium.org, Sep 22 2016

Issue description

Chrome OS Version: R54 & R55: ~8594.0.0 -> >= 8800.0.0
Chrome OS Platform: all

Expected Result:

No crash

Actual Result:

How frequently does this problem reproduce? (Always, sometimes, hard to
reproduce?)

I see 15,343 instances across all platforms, from version ~8594.0.0 through 8800.0.0

What is the impact to the user, and is there a workaround? If so, what is
it?

Unknown.

Please provide any additional information below. Attach a screen shot or
log if possible.

All crashes:

https://crash.corp.google.com/browse?q=product.name%3D%27ChromeOS%27%20AND%20stable_signature%20like%20%27_IO_new_fclose%25%27%20AND%20exec_name%3D%27quipper%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D

Example on samus: https://crash.corp.google.com/browse?q=ReportID%3D%27dbd8b41e00000000%27

Thread 0 CRASHED [SIGSEGV @ 0x00000000 ] MAGIC SIGNATURE THREAD
0x00007fb5bd61a3c0	(libc-2.19.so -iofclose.c:54 )	_IO_new_fclose
0x00007fb5be415b89	(quipper -dso.cc:142 )	quipper::ReadModuleBuildId
0x00007fb5be40a61f	(quipper -perf_parser.cc:350 )	quipper::PerfParser::FillInDsoBuildIds
0x00007fb5be40c450	(quipper -perf_parser.cc:279 )	quipper::PerfParser::ProcessEvents
0x00007fb5be40d5d3	(quipper -perf_parser.cc:161 )	quipper::PerfParser::ParseRawEvents
0x00007fb5be3f40d0	(quipper -perf_protobuf_io.cc:29 )	quipper::SerializeFromFileWithOptions
0x00007fb5be3eda51	(quipper -perf_recorder.cc:48 )	quipper::PerfRecorder::RunCommandAndGetSerializedOutput
0x00007fb5be3ec742	(quipper -quipper.cc:61 )	main
0x00007fb5bd5cafb5	(libc-2.19.so -libc-start.c:292 )	__libc_start_main
0x00007fb5be3ec917	(quipper + 0x0000a917 )	_start
0x00007ffcf46d5897		
 

Comment 1 by sque@chromium.org, Sep 22 2016

Cc: chongjiang@chromium.org
I assume _IO_new_fclose is an internal function called by fclose. There's probably a quipper::FileReader::FileReader() call between _IO_new_fclose() and quipper::ReadModuleBuildId() that's inlined out.

I suspect that the file is not found, and subsequently, fclose() is called by the destructor of the FileReader object on a null FILE pointer.

According to this, that's not allowed by glibc:
http://stackoverflow.com/questions/16922871/why-glibcs-fclosenull-cause-segmentation-fault-instead-of-returning-error

If I'm right, it should be a straightforward fix to avoid calling fclose(null).
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/86ab5d0ba36eed7131045204d2e7a70facc21414

commit 86ab5d0ba36eed7131045204d2e7a70facc21414
Author: Simon Que <sque@chromium.org>
Date: Thu Sep 22 20:02:11 2016

quipper: Don't call fclose() on null FILE pointer

The behavior is undefined. Check for it in FileReader.

Also replaced some fopen/fclose file reads with FileReader.

BUG= chromium:649242 
TEST=build successfully

Change-Id: I331ae2daf7b92fd7f727b184b907c9d1f8f4fbff
Reviewed-on: https://chromium-review.googlesource.com/388046
Commit-Ready: Simon Que <sque@chromium.org>
Tested-by: Simon Que <sque@chromium.org>
Reviewed-by: Chong Jiang <chongjiang@chromium.org>

[modify] https://crrev.com/86ab5d0ba36eed7131045204d2e7a70facc21414/chromiumos-wide-profiling/file_reader.cc
[modify] https://crrev.com/86ab5d0ba36eed7131045204d2e7a70facc21414/chromiumos-wide-profiling/test_utils.cc
[modify] https://crrev.com/86ab5d0ba36eed7131045204d2e7a70facc21414/chromiumos-wide-profiling/utils.cc

Comment 3 by sque@chromium.org, Sep 26 2016

Status: Fixed (was: Assigned)
Labels: Merge-Request-54

Comment 5 by dimu@chromium.org, Oct 2 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)
Project Member

Comment 6 by sheriffbot@chromium.org, Oct 5 2016

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 7 by sque@chromium.org, Oct 6 2016

Cc: djkurtz@chromium.org
chongjiang and I can't do a +2. Maybe we need a sheriff to review. djkurtz, can you review for +2?

https://chromium-review.googlesource.com/#/c/394231/
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 6 2016

Labels: merge-merged-release-R54-8743.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/2c31f86d436bcd434c710da5ad8aee7c439dcb14

commit 2c31f86d436bcd434c710da5ad8aee7c439dcb14
Author: Simon Que <sque@chromium.org>
Date: Thu Sep 22 20:02:11 2016

quipper: Don't call fclose() on null FILE pointer

The behavior is undefined. Check for it in FileReader.

Also replaced some fopen/fclose file reads with FileReader.

BUG= chromium:649242 
TEST=build successfully

Change-Id: I111cb3263abdfbc6f232517f47f296646e3f60c3
Previous-Reviewed-on: https://chromium-review.googlesource.com/394231
(cherry picked from commit f9583eeb3849b1616d9e6af28222608ff0d665a6)
Reviewed-on: https://chromium-review.googlesource.com/394231
Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Chong Jiang <chongjiang@chromium.org>
Reviewed-by: Simon Que <sque@chromium.org>
Tested-by: Simon Que <sque@chromium.org>

[modify] https://crrev.com/2c31f86d436bcd434c710da5ad8aee7c439dcb14/chromiumos-wide-profiling/file_reader.cc
[modify] https://crrev.com/2c31f86d436bcd434c710da5ad8aee7c439dcb14/chromiumos-wide-profiling/test_utils.cc
[modify] https://crrev.com/2c31f86d436bcd434c710da5ad8aee7c439dcb14/chromiumos-wide-profiling/utils.cc

Project Member

Comment 9 by sheriffbot@chromium.org, Oct 8 2016

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Approved-54

Comment 11 by dchan@google.com, Mar 4 2017

Labels: VerifyIn-58

Comment 12 by dchan@google.com, Apr 17 2017

Labels: VerifyIn-59

Comment 13 by dchan@google.com, May 30 2017

Labels: VerifyIn-60
Labels: VerifyIn-61

Comment 15 by dchan@chromium.org, Oct 14 2017

Status: Archived (was: Fixed)

Sign in to add a comment