Closed
Bug 1313028
Opened 8 years ago
Closed 8 years ago
GCC 6 null pointer check removal causes segfaulting in ActionResultHolder::GetValueAndDelete()
Categories
(Testing :: General, defect)
Testing
General
Tracking
(firefox52 fixed)
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: jseward, Unassigned)
References
Details
Attachments
(1 file)
1.89 KB,
patch
|
chmanchester
:
review+
chmanchester
:
feedback+
|
Details | Diff | Splinter Review |
An optimised build with GCC 6.2 on x86_64-Linux, Fedora 23, results in ./mach gtest eventually segfaulting, due to calling ActionResultHolder::GetValueAndDelete() with this=0x0. I suspect it is a result of: Result InvokeWith(const ArgumentTuple& args) { return static_cast<const ResultHolder*>( this->UntypedInvokeWith(&args))->GetValueAndDelete(); } GCC 6 can rightly (per the spec) assume that this->UntypedInvokeWith(&args) doesn't return a NULL value, since we are subsequently calling GetValueAndDelete(), implying that we somehow know that the value we are calling on is non-NULL. But that isn't always true. The result is a null dereference like this: Invalid read of size 8 at 0x155C597D: GetValueAndDelete (gmock-spec-builders.h:1357) by 0x155C597D: InvokeWith (gmock-spec-builders.h:1490) by 0x155C597D: Invoke (gmock-generated-function-mockers.h:76) by 0x155C597D: (anonymous namespace)::TestMock::MockedCall() (SanityTest.cpp:20) by 0x155C5C2E: MozillaGMockSanity_Runs_Test::TestBody() (SanityTest.cpp:28) by 0x155BF91B: testing::Test::Run() (gtest.cc:2162) by 0x155BFA2B: testing::TestInfo::Run() (gtest.cc:2338) by 0x155BFAC9: testing::TestCase::Run() (gtest.cc:2445) by 0x155C05C9: testing::internal::UnitTestImpl::RunAllTests() (gtest.cc:4237) by 0x155C06C3: HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (gtest.cc:2145) by 0x155C06C3: testing::UnitTest::Run() (gtest.cc:3871) by 0x155C4D8E: mozilla::RunGTestFunc() (GTestRunner.cpp:118) by 0x14F00222: XREMain::XRE_mainStartup(bool*) (nsAppRunner.cpp:3742) by 0x14F03AEE: XREMain::XRE_main(int, char**, nsXREAppData const*) (nsAppRunner.cpp:4568) by 0x14F03E08: XRE_main (nsAppRunner.cpp:4674) by 0x404CA1: do_main(int, char**, char**, nsIFile*) (nsBrowserApp.cpp:282) Address 0x0 is not stack'd, malloc'd or (recently) free'd
Reporter | ||
Comment 1•8 years ago
|
||
Per https://github.com/google/googletest/issues/705 this is a known problem that has been fixed already in the GTest/GMock trunk, but we have an older and unfixed release. A simple fix for our version, that appears to work, is shown here: https://github.com/google/googletest/issues/705#issuecomment-235067917
Reporter | ||
Updated•8 years ago
|
Attachment #8804806 -
Flags: feedback?(cmanchester)
Comment 3•8 years ago
|
||
Comment on attachment 8804806 [details] [diff] [review] bug1313028-1.diff I'm not sure what our policy is for maintaining local patches to gmock, BenWa, is there any precedent for this?
Flags: needinfo?(b56girard)
Attachment #8804806 -
Flags: feedback?(cmanchester) → feedback+
Comment 4•8 years ago
|
||
I'm not aware of us having applied any patches for gtest/gmock.
Flags: needinfo?(b56girard)
Reporter | ||
Comment 5•8 years ago
|
||
Well, would it be OK to land this? We need to take this fix or something equivalent, in order to have |mach gtest| work when building with gcc 6.
Comment 6•8 years ago
|
||
If it's fixed upstream then it seems fine to patch locally until we update our vendored copy to pick up the upstream fix.
Reporter | ||
Comment 7•8 years ago
|
||
Ted; thanks. Chris, can you pls upgrade your f+ to an r+ so I can land this?
Flags: needinfo?(cmanchester)
Updated•8 years ago
|
Flags: needinfo?(cmanchester)
Attachment #8804806 -
Flags: review+
Pushed by jseward@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f74ebe3e7a86 GCC 6 null pointer check removal causes segfaulting in ActionResultHolder::GetValueAndDelete(). r=chmanchester.
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f74ebe3e7a86
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•