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

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

Closed
freydaniel opened this issue Sep 4, 2024 · 7 comments
Closed

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

freydaniel opened this issue Sep 4, 2024 · 7 comments
Assignees

Comments

@freydaniel
Copy link

freydaniel commented Sep 4, 2024

What version of protobuf and what language are you using?

Version: v23.4 (2c5fa07) 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 freydaniel added the untriaged auto added to all issues by default when created. label Sep 4, 2024
@freydaniel
Copy link
Author

freydaniel commented Sep 4, 2024

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
Copy link
Member

@acozzette can you take a look at this?

@googleberg googleberg removed the untriaged auto added to all issues by default when created. label Sep 9, 2024
@acozzette
Copy link
Member

@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
Copy link
Author

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

@freydaniel
Copy link
Author

@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
Copy link
Member

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
Copy link
Member

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.

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

No branches or pull requests

3 participants