protocolbuffers / protobuf

Protocol Buffers - Google's data interchange format
http://protobuf.dev
Other
65.64k stars 15.49k forks source link

protobuf::message using protobuf::map across dlls leads to crashes #18097

Closed freydaniel closed 1 week ago

freydaniel commented 2 months ago

What version of protobuf and what language are you using?

Version: v23.4 (2c5fa078d8e86e5f4bd34e6f4c9ea9e8d7d4d44a) through grpc v1.58.3

Language: C++

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

Windows 10

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

MSVC x64 v142

What did you do?

I statically link protobuf to a custom dll where we generate and dllexport a proto message containing a map<string, message>.

syntax = "proto3";

option cc_enable_arenas = true;

...

message IOStateRequest
{
  map<string, VEStateRequest> ve_state_requests = 1;
  CRStateRequest cr_state_request = 2;
}

I instantiate a message of that type in our unit test exe, pass it as a reference to the helper method IOStateHelper::changeVERunningState in the dll, where it creates and adds messages to the map.

void IOStateHelper::changeVERunningState(
  const std::string& inProductName,
  const eProcessRunningState& inRunningState,
  IOState& inOutIOState,)
{
  VEState _veState;
  _veState.set_running_state(inRunningState);

  auto* _veStates = inOutIOState.mutable_ve_states();
  (*_veStates)[inProductName] = _veState;
}

I then test the content of the map in the unit test.

TEST(IOStateHelperTest, changeVERunningState)
{
  IOState _ioState;
  std::string _productName = "Test";

  eProcessRunningState _veRunningState = ePRS_Running;
  IOStateHelper::changeVERunningState(_productName, _veRunningState, _ioState);

  int _veSize = _ioState.ve_states_size();
  EXPECT_EQ(_veSize, 1);
  if (_veSize != 1)
  {
    return;
  }
  EXPECT_EQ(_ioState.ve_states().at(_productName).running_state(), _veRunningState);
}

What did you expect to see

I expected to be able to access the values I inserted inside the helper method with the very keys I inserted in the map in the helper method.

What did you see instead?

The keys and values are in the map (checked with iteration). But when I access the map at the expected key with .at() the test crashes:

!! This test has probably CRASHED !!
Test output:

WARNING: All log messages before absl::InitializeLog() is called are written to STDERR
F0000 00:00:1725450987.183222   29068 map.h:1317] Check failed: it != end() key not found: Test
*** Check failure stack trace: ***
    @   00007FF762015C4E  (unknown)
    @   00007FF762015FA3  (unknown)
    @   00007FF762015031  (unknown)
    @   00007FF7620134CF  (unknown)
    @   00007FF7618C1213  (unknown)
    @   00007FF7618C1D79  (unknown)
    @   00007FF761C105BD  (unknown)
    @   00007FF761C101F3  (unknown)
    @   00007FF761BE273B  (unknown)
    @   00007FF761BE327A  (unknown)
    @   00007FF761BE3B1A  (unknown)
    @   00007FF761BE9FD8  (unknown)
    @   00007FF761C107AD  (unknown)
    @   00007FF761C10503  (unknown)
    @   00007FF761BE4301  (unknown)
    @   00007FF7621074E3  (unknown)
    @   00007FF7621074B5  (unknown)
    @   00007FF7620FE779  (unknown)
    @   00007FF7620FE65E  (unknown)
    @   00007FF7620FE51E  (unknown)
    @   00007FF7620FE80E  (unknown)
    @   00007FFE4AA07374  (unknown)
    @   00007FFE4C95CC91  (unknown)

Back when we still used protobuf v21 (through grpc 1.52.0), this was no issue yet.

The reason for this seems to be: Since v22, the protobuf (hash) map uses absl::hash that uses a non-deterministic seed that varies across dlls (v21 still used the std map).

While this is stated in https://abseil.io/docs/cpp/guides/hash ("NOTE: the hash codes computed by absl::Hash are not guaranteed to be stable across different runs of your program, or across different dynamically loaded libraries in your program."), it is quite a dangerous and hidden behavior when using protobuf.

Could you please provide guidance if this is expected behavior and one must not use proto messages across dll boundaries? Or if this a bug or if there is another way to handle this situation?

Anything else we should know about your project / environment

freydaniel commented 2 months ago

Since using protobuf v23, I also get a C4275 compiler warning (non dll-interface class 'google::protobuf::Message' used as base for dll-interface class) when compiling the protoc-generated files for the above exported proto messsage.

Does this want to warn me that EXPORT_MACRO should no longer be used with protobuf? (I didn't get the warning with v21 while already statically linking protobuf)

googleberg commented 1 month ago

@acozzette can you take a look at this?

acozzette commented 1 month ago

@freydaniel That is an interesting failure mode, but I don't think this is a bug in protobuf. It sounds me to like the root cause is an ODR violation since you have multiple definitions of absl::Hash. I believe the right fix would be to change your build setup so that you link against the necessary Abseil dynamic libraries instead of including Abseil code in your own DLLs.

freydaniel commented 1 month ago

@acozzette Thanks for the quick answer, I am trying to adapt our build accordingly.

freydaniel commented 1 month ago

@acozzette I got the build working and abseil as dll fixes the above mentioned hash issue.

Regarding my other comment about the C4275 compiler warning: Is there a way around that? Or can the warning be ignored?

The only way I'd see is using protobuf as DLL as well, but as I read this is not recommended.

acozzette commented 1 month ago

Hmm, I'm not sure what that error is about. We tend to statically link things most of the time, so I'm not super familiar with dynamic linking.

Maybe I'm forgetting something, but I don't think there is necessarily anything wrong with using libprotobuf as a DLL as well.

googleberg commented 1 month ago

In general, if you are building an application using DLLs and you plan to pass objects across DLL boundaries, the code that supports those objects also needs to be in a DLL. So if you are planning to pass proto messages between functions built into DLLs you will need to use protobuf as a DLL or you'll likely run into problems similar to your initial one.