Closed Bug 1313028 Opened 3 years ago Closed 3 years ago

GCC 6 null pointer check removal causes segfaulting in ActionResultHolder::GetValueAndDelete()

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jseward, Unassigned)

References

Details

Attachments

(1 file)

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
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
Attachment #8804806 - Flags: feedback?(cmanchester)
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+
I'm not aware of us having applied any patches for gtest/gmock.
Flags: needinfo?(b56girard)
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.
If it's fixed upstream then it seems fine to patch locally until we update our vendored copy to pick up the upstream fix.
Ted; thanks.  Chris, can you pls upgrade your f+ to an r+ so I can land this?
Flags: needinfo?(cmanchester)
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.
https://hg.mozilla.org/mozilla-central/rev/f74ebe3e7a86
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.