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

Tests segfault with C++17 #12104

Closed
antonio-rojas opened this issue Mar 1, 2023 · 13 comments
Closed

Tests segfault with C++17 #12104

antonio-rojas opened this issue Mar 1, 2023 · 13 comments
Assignees

Comments

@antonio-rojas
Copy link

What version of protobuf and what language are you using?
Version: 22.0 / git master
Language: C++

What operating system (Linux, Windows, ...) and version?
Linux x86_64

What runtime / compiler are you using (e.g., python version or gcc version)
GCC 12.2.1

What did you do?
cmake -B build -S protobuf
-DCMAKE_INSTALL_PREFIX=/usr
-Dprotobuf_USE_EXTERNAL_GTEST=ON
-Dprotobuf_BUILD_SHARED_LIBS=ON
-Dprotobuf_ABSL_PROVIDER=package
cmake --build build
cmake --build build --target test

What did you expect to see
Tests pass

What did you see instead?
Tests segfault

#0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
#1  0x00007f65d1ea0953 in __pthread_kill_internal (signo=6, threadid=<optimized out>) at pthread_kill.c:78
#2  0x00007f65d1e51ea8 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#3  0x00007f65d1e3b53d in __GI_abort () at abort.c:79
#4  0x00007f65d2bb74d2 in absl::lts_20230125::log_internal::LogMessage::FailWithoutStackTrace ()
    at /usr/src/debug/abseil-cpp/abseil-cpp-20230125.1/absl/log/internal/log_message.cc:333
#5  0x00007f65d2bb7835 in absl::lts_20230125::log_internal::LogMessage::Die (this=0x7ffebae42210)
    at /usr/src/debug/abseil-cpp/abseil-cpp-20230125.1/absl/log/internal/log_message.cc:491
#6  0x00007f65d2bb7dc9 in absl::lts_20230125::log_internal::LogMessageFatal::~LogMessageFatal (this=<optimized out>, __in_chrg=<optimized out>)
    at /usr/src/debug/abseil-cpp/abseil-cpp-20230125.1/absl/log/internal/log_message.cc:582
#7  0x00007f65d283fc0b in google::protobuf::compiler::objectivec::TextFormatDecodeData::DecodeDataForString(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) [clone .cold] ()
   from /home/antonio/Software/Arch/packages/protobuf/trunk/src/build/libprotoc.so.4.22.0.0
#8  0x000055700ee88053 in google::protobuf::compiler::objectivec::(anonymous namespace)::ObjCHelperDeathTest_TextFormatDecodeData_DecodeDataForString_Failures_Test::TestBody() ()
#9  0x00007f65d23adb3c in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void> (location=0x7f65d23b68f0 "the test body", 
    method=<optimized out>, object=0x557010ad5c00) at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:2621
#10 testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) [clone .constprop.0] (object=0x557010ad5c00, method=<optimized out>, location=0x7f65d23b68f0 "the test body")
    at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:2657
#11 0x00007f65d239a198 in testing::Test::Run (this=0x557010ad5c00) at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:2696
#12 testing::Test::Run (this=0x557010ad5c00) at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:2686
#13 0x00007f65d239a3a7 in testing::TestInfo::Run (this=0x557010a67420) at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:2845
#14 0x00007f65d239a504 in testing::TestSuite::Run (this=0x557010a676c0) at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:3004
#15 testing::TestSuite::Run (this=0x557010a676c0) at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:2977
--Type <RET> for more, q to quit, c to continue without paging--
#16 0x00007f65d23a6a6f in testing::internal::UnitTestImpl::RunAllTests (this=this@entry=0x557010995f70)
    at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:5890
#17 0x00007f65d23a56a9 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (
    location=0x7f65d23b7ea0 "auxiliary test code (environments or event listeners)", 
    method=(bool (testing::internal::UnitTestImpl::*)(testing::internal::UnitTestImpl * const)) 0x7f65d23a6700 <testing::internal::UnitTestImpl::RunAllTests()>, object=0x557010995f70) at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:2602
#18 testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (
    location=0x7f65d23b7ea0 "auxiliary test code (environments or event listeners)", 
    method=(bool (testing::internal::UnitTestImpl::*)(testing::internal::UnitTestImpl * const)) 0x7f65d23a6700 <testing::internal::UnitTestImpl::RunAllTests()>, object=0x557010995f70) at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:2657
#19 testing::UnitTest::Run (this=<optimized out>) at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:5455
#20 0x00007f65d2bc8066 in RUN_ALL_TESTS () at /usr/src/debug/gtest/googletest-1.13.0/googletest/include/gtest/gtest.h:2314
#21 main (argc=<optimized out>, argv=0x7ffebae42768) at /usr/src/debug/gtest/googletest-1.13.0/googlemock/src/gmock_main.cc:70

Anything else we should know about your project / environment

Same issue when using the bundled abseil-cpp. Building with C++14 makes test pass correctly.

@antonio-rojas antonio-rojas added the untriaged auto added to all issues by default when created. label Mar 1, 2023
@deannagarcia deannagarcia removed the untriaged auto added to all issues by default when created. label Mar 2, 2023
@mkruskal-google
Copy link
Member

This is coming from a death test, where we expect a crash. Are you actually seeing a test failure? If so could you provide a more complete log? googletest handles death tests by spawning a new process and analyzing the crash.

@antonio-rojas
Copy link
Author

This is coming from a death test, where we expect a crash. Are you actually seeing a test failure? If so could you provide a more complete log? googletest handles death tests by spawning a new process and analyzing the crash.

Here is the full output of ctest
ctest.txt

@mkruskal-google
Copy link
Member

That's a very different error. There's no stack trace and it's failing on a different test?

@antonio-rojas
Copy link
Author

antonio-rojas commented Mar 4, 2023

There were many core dumps (probably because all of the previous tests) and I may have posted the wrong one. This looks more like it:

(gdb) bt
#0  __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:76
#1  0x00007ffff7a5fc09 in google::protobuf::internal::WireFormatLite::VerifyUtf8String(char const*, int, google::protobuf::internal::WireFormatLite::Operation, char const*) () from /home/antonio/Software/Arch/packages/protobuf/trunk/src/protobuf-22.0/build/libprotobuf.so.4.22.0.0
#2  0x0000555555bf772c in google::protobuf::internal::(anonymous namespace)::Utf8ValidationTest_OldVerifyUTF8String_Test::TestBody() ()
#3  0x00007ffff773bb3c in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void> (location=0x7ffff77448f0 "the test body", 
    method=<optimized out>, object=0x5555567148a0) at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:2621
#4  testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) [clone .constprop.0] (object=0x5555567148a0, method=<optimized out>, location=0x7ffff77448f0 "the test body")
    at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:2657
#5  0x00007ffff7728198 in testing::Test::Run (this=0x5555567148a0) at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:2696
#6  testing::Test::Run (this=0x5555567148a0) at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:2686
#7  0x00007ffff77283a7 in testing::TestInfo::Run (this=0x555556743e50) at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:2845
#8  0x00007ffff7728504 in testing::TestSuite::Run (this=0x5555567430d0) at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:3004
#9  testing::TestSuite::Run (this=0x5555567430d0) at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:2977
#10 0x00007ffff7734a6f in testing::internal::UnitTestImpl::RunAllTests (this=this@entry=0x555556696f70)
    at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:5890
#11 0x00007ffff77336a9 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (
    location=0x7ffff7745ea0 "auxiliary test code (environments or event listeners)", 
    method=(bool (testing::internal::UnitTestImpl::*)(testing::internal::UnitTestImpl * const)) 0x7ffff7734700 <testing::internal::UnitTestImpl::RunAllTests()>, object=0x555556696f70) at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:2602
#12 testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (
    location=0x7ffff7745ea0 "auxiliary test code (environments or event listeners)", 
    method=(bool (testing::internal::UnitTestImpl::*)(testing::internal::UnitTestImpl * const)) 0x7ffff7734700 <testing::internal::UnitTestImpl::RunAllTests()>, object=0x555556696f70) at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:2657
#13 testing::UnitTest::Run (this=<optimized out>) at /usr/src/debug/gtest/googletest-1.13.0/googletest/src/gtest.cc:5455
#14 0x00007ffff7f91066 in RUN_ALL_TESTS () at /usr/src/debug/gtest/googletest-1.13.0/googletest/include/gtest/gtest.h:2314
#15 main (argc=<optimized out>, argv=0x7fffffffe2e8) at /usr/src/debug/gtest/googletest-1.13.0/googlemock/src/gmock_main.cc:70

@antonio-rojas
Copy link
Author

Minimal test case:

$ cat test.cpp
#include <google/protobuf/wire_format.h>

int main() {
  const char* kInvalidUTF8String = "Invalid UTF-8: \xA0\xB0\xC0\xD0";
  std::string data(kInvalidUTF8String);
  using namespace google::protobuf::internal;
  WireFormat::VerifyUTF8String(data.data(), data.size(), WireFormat::SERIALIZE);
}
g++ test.cpp -lprotobuf -o test
$ ./test
Violación de segmento (`core' generado)
(gdb) bt
#0  __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:76
#1  0x00007ffff7df1869 in google::protobuf::internal::WireFormatLite::VerifyUtf8String(char const*, int, google::protobuf::internal::WireFormatLite::Operation, char const*) () from /usr/lib/libprotobuf.so.22.2.0
#2  0x0000555555556433 in google::protobuf::internal::WireFormat::VerifyUTF8String(char const*, int, google::protobuf::internal::WireFormat::Operation)
    ()
#3  0x0000555555556304 in main ()

@omenos
Copy link
Contributor

omenos commented Apr 27, 2023

This general issue is still relevant with v23.0-rc1. I've been building protobuf for Fedora 38 and Rawhide for testing purposes which uses GCC 13 (defaulting to -std=gnu++17) and though the project can build, the full-test suite will fail (lite-test and conformance tests pass). I ultimately had to rebuild both gtest and abseil-cpp using C++14, and force protobuf to use it as well in order to pass tests.

This commit may have been removed too hastily without further coverage of newer standards in testing... It's also a bit of a giveaway that the Bazel config is enforcing C++14 for compilation.

@mkruskal-google
Copy link
Member

Is the issue that the segfault only happens with C++17? That's really surprising since we do a lot of testing for C++17, but that would help narrow this down

@antonio-rojas
Copy link
Author

Yes, it only happens with C++17

copybara-service bot pushed a commit that referenced this issue May 8, 2023
This crash was due to the fact that we were passing `nullptr` as a `const
char*` parameter and relying on that implicitly converting into an empty
`absl::string_view`. `absl::string_view` supports that functionality, but
starting with C++17 its behavior changes since it's just a type alias for
`std::string_view`. `std::string_view` does not have any special conversion for
nullptr and so we were just getting crashes.

PiperOrigin-RevId: 530423047
copybara-service bot pushed a commit that referenced this issue May 8, 2023
This crash was due to the fact that we were passing `nullptr` as a `const
char*` parameter and relying on that implicitly converting into an empty
`absl::string_view`. `absl::string_view` supports that functionality, but
starting with C++17 its behavior changes since it's just a type alias for
`std::string_view`. `std::string_view` does not have any special conversion for
nullptr and so we were just getting crashes.

PiperOrigin-RevId: 530423047
copybara-service bot pushed a commit that referenced this issue May 8, 2023
This crash was due to the fact that we were passing `nullptr` as a `const
char*` parameter and relying on that implicitly converting into an empty
`absl::string_view`. `absl::string_view` supports that functionality, but
starting with C++17 its behavior changes since it's just a type alias for
`std::string_view`. `std::string_view` does not have any special conversion for
nullptr and so we were just getting crashes.

PiperOrigin-RevId: 530431663
@omenos
Copy link
Contributor

omenos commented May 8, 2023

Adding secondary confirmation that it's only with C++17. Against Fedora Rawhide I tried multiple permutations of the gtest, abseil-cpp, and protobuf packages with C++14 and C++17 and only having a complete C++14 chain worked (with the exception of json-cpp).

Additional commentary in this Fedora PR:

https://src.fedoraproject.org/rpms/protobuf/pull-request/22

@acozzette
Copy link
Member

This is now fixed in d38ba32, and I am going to cherry-pick the fix into 22.x and 23.x.

acozzette added a commit to acozzette/protobuf that referenced this issue May 8, 2023
This crash was due to the fact that we were passing `nullptr` as a `const
char*` parameter and relying on that implicitly converting into an empty
`absl::string_view`. `absl::string_view` supports that functionality, but
starting with C++17 its behavior changes since it's just a type alias for
`std::string_view`. `std::string_view` does not have any special conversion for
nullptr and so we were just getting crashes.

PiperOrigin-RevId: 530431663
acozzette added a commit to acozzette/protobuf that referenced this issue May 8, 2023
This crash was due to the fact that we were passing `nullptr` as a `const
char*` parameter and relying on that implicitly converting into an empty
`absl::string_view`. `absl::string_view` supports that functionality, but
starting with C++17 its behavior changes since it's just a type alias for
`std::string_view`. `std::string_view` does not have any special conversion for
nullptr and so we were just getting crashes.

PiperOrigin-RevId: 530431663
acozzette added a commit that referenced this issue May 9, 2023
This crash was due to the fact that we were passing `nullptr` as a `const
char*` parameter and relying on that implicitly converting into an empty
`absl::string_view`. `absl::string_view` supports that functionality, but
starting with C++17 its behavior changes since it's just a type alias for
`std::string_view`. `std::string_view` does not have any special conversion for
nullptr and so we were just getting crashes.

PiperOrigin-RevId: 530431663
mkruskal-google added a commit that referenced this issue May 9, 2023
@omenos
Copy link
Contributor

omenos commented May 10, 2023

@acozzette I can confirm that this fix allows testing to pass, however in a semi-related issue, Python will fail to build as it is hardcoded to use C++14:

https://github.com/protocolbuffers/protobuf/blob/main/python/setup.py#L368-L373

    if sys.platform != 'win32':
      extra_compile_args.append('-Wno-write-strings')
      extra_compile_args.append('-Wno-invalid-offsetof')
      extra_compile_args.append('-Wno-sign-compare')
      extra_compile_args.append('-Wno-unused-variable')
      extra_compile_args.append('-std=c++14')

So an additional patch(es) will be necessary to ensure across the entire project the CXX_STANDARD is consistent.

@acozzette
Copy link
Member

What kind of error are you getting with the Python build?

I'm not sure of the best solution for setup.py. We require at least C++14, but ideally users should be able to choose versions above that as well.

@omenos
Copy link
Contributor

omenos commented May 10, 2023

@acozzette

Full build log here: https://download.copr.fedorainfracloud.org/results/mroche/protobuf/fedora-rawhide-x86_64/05906231-protobuf/builder-live.log.gz

If you want to see the patches in use for that specific build (the setup has been modified since, but the general idea is the same):

# wget https://download.copr.fedorainfracloud.org/results/mroche/protobuf/srpm-builds/05906231/protobuf-23.0-1.fc39.src.rpm
# rpm2cpio protobuf-23.0-1.fc39.src.rpm | cpio -idm

If you search the log above for creating build/temp.linux-x86_64-cpython-311/google/protobuf/pyext you'll reach the error point. It has to do with Python trying to build with C++14 hardcoded, but it breaks because it's trying to build against abseil-cpp that was compiled with C++17.

I've successfully built by either:

  • modifying the setup to use c++17/gnu++17
  • modifying the setup to drop the line completely and use the toolchain's default standard.

I'm presently using the latter for Fedora builds as it makes the most sense for that specific context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants