Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix double free when building Gtest/GMock in shared libraries and lin… #1339

Merged
merged 4 commits into from
Jan 15, 2018

Conversation

Romain-Geissler
Copy link
Contributor

…king a test executable with both.

Hi,

This fixes issue #930. It is based on what was done in PR #1286 which itself was based on PR #1059 .

The only change from #1286 is that in the tests, the shared library gmock_shared_main is not modified at all, so that the build can go on, and tests are not broken. Tested locally, it fixes the core dump people have when linking both gmock and gtest using shared libraries.

Cheers,
Romain

@Romain-Geissler
Copy link
Contributor Author

Note: the build failed on 2 OS X hosts in the Travis jobs, but this is certainly not due to this pull request. If fails the same way on the last commit of branch master.

Copy link

@agirault agirault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking at this 👍

"${gtest_dir}/src/gtest-all.cc"
src/gmock-all.cc
src/gmock_main.cc)
if (MSVC)
Copy link

@agirault agirault Dec 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it necessary to keep doing it that way for MSVC? Wouldn't the target_link_libraries way work fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently not if I believe #1286. But the original build issue seems not available anymore. I have honestly not tried myself (and don't have any Windows machine beyond the continuous integration ones).

@gennadiycivil
Copy link
Contributor

@Romain-Geissler Please
Thank you very much for this contribution. Please provide testing output that clearly demonstrates before/after.

cxx_library(gmock_main_use_own_tuple "${cxx_use_own_tuple}"
"${gtest_dir}/src/gtest-all.cc" src/gmock-all.cc src/gmock_main.cc)

cxx_test_with_flags(gmock_use_own_tuple_test "${cxx_use_own_tuple}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously gmock_use_own_tuple_test was also built with non-MSVC compiler.
I have tested changed version of this patch with gmock_use_own_tuple_test building enabled on GNU/Linux with GCC and gmock_use_own_tuple_test test passed.
I think that these 2 lines should be copied to non-MSVC branch below.

@Arfrever
Copy link

Arfrever commented Jan 7, 2018

This patch fixes tests of Protobuf, which otherwise fail with segmentation fault and abort.
Please merge (some version of) this patch.

@gennadiycivil
Copy link
Contributor

gennadiycivil commented Jan 9, 2018

Thank you again for this. Could you please provide testing evidence to prove that this in fact "This patch fixes tests of Protobuf, which otherwise fail with segmentation fault and abort"
@Arfrever

@Arfrever
Copy link

Arfrever commented Jan 10, 2018

This was reproduced by both Toralf Förster and me:
https://bugs.gentoo.org/631698
https://bugs.gentoo.org/644044 (Duplicate of 631698)

From Toralf Förster's log in the latter bug:

make  check-TESTS
make[3]: Entering directory '/var/tmp/portage/dev-libs/protobuf-3.5.1.1/work/protobuf-3.5.1.1-abi_x86_64.amd64/src'
make[4]: Entering directory '/var/tmp/portage/dev-libs/protobuf-3.5.1.1/work/protobuf-3.5.1.1-abi_x86_64.amd64/src'
/var/tmp/portage/dev-libs/protobuf-3.5.1.1/work/protobuf-3.5.1.1/test-driver: line 107:  3393 Segmentation fault      "$@" > $log_file 2>&1
FAIL: protobuf-test
/var/tmp/portage/dev-libs/protobuf-3.5.1.1/work/protobuf-3.5.1.1/test-driver: line 107: 14114 Aborted                 "$@" > $log_file 2>&1
FAIL: protobuf-lazy-descriptor-test
/var/tmp/portage/dev-libs/protobuf-3.5.1.1/work/protobuf-3.5.1.1/test-driver: line 107: 14217 Aborted                 "$@" > $log_file 2>&1
FAIL: protobuf-lite-test
PASS: google/protobuf/compiler/zip_output_unittest.sh
PASS: google/protobuf/io/gzip_stream_unittest.sh
PASS: protobuf-lite-arena-test
============================================================================
Testsuite summary for Protocol Buffers 3.5.1
============================================================================
# TOTAL: 6
# PASS:  3
# SKIP:  0
# XFAIL: 0
# FAIL:  3
# XPASS: 0
# ERROR: 0
============================================================================
See src/test-suite.log
Please report to protobuf@googlegroups.com
============================================================================
make[4]: *** [Makefile:6758: test-suite.log] Error 1
make[4]: Leaving directory '/var/tmp/portage/dev-libs/protobuf-3.5.1.1/work/protobuf-3.5.1.1-abi_x86_64.amd64/src'
make[3]: *** [Makefile:6866: check-TESTS] Error 2
make[3]: Leaving directory '/var/tmp/portage/dev-libs/protobuf-3.5.1.1/work/protobuf-3.5.1.1-abi_x86_64.amd64/src'
make[2]: *** [Makefile:6972: check-am] Error 2
make[2]: Leaving directory '/var/tmp/portage/dev-libs/protobuf-3.5.1.1/work/protobuf-3.5.1.1-abi_x86_64.amd64/src'
make[1]: *** [Makefile:6974: check] Error 2
make[1]: Leaving directory '/var/tmp/portage/dev-libs/protobuf-3.5.1.1/work/protobuf-3.5.1.1-abi_x86_64.amd64/src'
make: *** [Makefile:1554: check-recursive] Error 1

(I got the same results.)

Romain Geissler's patch has been added to dev-cpp/gtest (in version 1.8.0-r1) in Gentoo yesterday. Toralf Förster was using older, unpatched version 1.8.0 of dev-cpp/gtest.

@gennadiycivil gennadiycivil merged commit 051fe2f into google:master Jan 15, 2018
facebook-github-bot pushed a commit to facebook/folly that referenced this pull request Jan 24, 2018
Summary:
Previously to use gmock you simply needed to link against libgmock_main.
However, googletest pull request #1339 was recently merged into upstream
googletest: google/googletest#1339

These changes now require explicitly linking against libgmock_main, libgmock,
and libgtest.

Reviewed By: yfeldblum

Differential Revision: D6794709

fbshipit-source-id: 03dcccec966e62240987ee0051dfa87be8614cca
SSE4 added a commit to bincrafters/conan-gtest that referenced this pull request Feb 20, 2018
Peter-Levine added a commit to Peter-Levine/gentoo that referenced this pull request Sep 14, 2018
The doublefree bug is fixed upstream with the merge of
google/googletest#1339

Package-Manager: Portage-2.3.49, Repoman-2.3.10
gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Sep 22, 2018
The doublefree bug is fixed upstream with the merge of
google/googletest#1339

Package-Manager: Portage-2.3.49, Repoman-2.3.10
muffgaga added a commit to electronicvisions/spack that referenced this pull request Jan 25, 2019
Includes fix for double free bug
(google/googletest#1339).

Change-Id: I4b79f3f79ff376359b213c7c86e1132f58826379
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants