Skip to content

Make libprotobuf symbols local on OSX#8346

Merged
TeBoring merged 1 commit intoprotocolbuffers:masterfrom
rowillia:hide_protobuf_symbols
Apr 24, 2021
Merged

Make libprotobuf symbols local on OSX#8346
TeBoring merged 1 commit intoprotocolbuffers:masterfrom
rowillia:hide_protobuf_symbols

Conversation

@rowillia
Copy link
Copy Markdown
Contributor

@rowillia rowillia commented Feb 25, 2021

@gnossen gave a great overview in grpc/grpc#24992 of the overall problem.

If a python process using both protobuf and another native library linking in libprotobuf
frequently can cause crashes. This seems to frequently affect tensorflow as well:

tensorflow/tensorflow#8394,
tensorflow/tensorflow#9525 (comment)
tensorflow/tensorflow#24976,
tensorflow/tensorflow#35573,
https://github.com/tensorflow/tensorflow/blob/v2.0.0/tensorflow/contrib/makefile/rename_protobuf.sh,
tensorflow/tensorflow#16104

This fixes both crashes when linking in multiple versions of protobuf
and fixes DescriptorPool clashes as well (e.g. Python and Native code import different versions of the same message).

Make libprotobuf symbols local on OSX
8f26959 
@gnossen gave a great overview in grpc/grpc#24992 of the overall problem.

If a python process using both protobuf _and_ another native library linking in libprotobuf
frequently can cause crashes.  This seems to frequently affect tensorflow as well:

tensorflow/tensorflow#8394,
tensorflow/tensorflow#9525 (comment)
tensorflow/tensorflow#24976,
tensorflow/tensorflow#35573,
https://github.com/tensorflow/tensorflow/blob/v2.0.0/tensorflow/contrib/makefile/rename_protobuf.sh,
tensorflow/tensorflow#16104

Testing locally this fixes both crashes when linking in multiple versions of protobuf
and fixes `DescriptorPool` clashes as well (e.g. Python and Native code import different versions of the same message).
@google-cla google-cla Bot added the cla: yes label Feb 25, 2021
This was referenced Feb 25, 2021
Closed
Closed
TeBoring
TeBoring reviewed Apr 23, 2021
View reviewed changes
Comment thread python/setup.py

extra_compile_args = []

message_extra_link_args = None
Copy link
Copy Markdown
Contributor

@TeBoring TeBoring Apr 23, 2021

Choose a reason for hiding this comment

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

https://github.com/grpc/grpc/pull/24992/files
Could you add a comment like the one in grpc's PR to explain the reason?

@TeBoring
Copy link
Copy Markdown
Contributor

TeBoring commented Apr 23, 2021

  1. Could you explain a little more about, before this change, how does mixture of different versions of symbols caused the problem? Seems to me, previously, each versions protobuf symbol was local weak symbols, which is just used in the module locally. If so, seems nothing bad would happen.
  2. Seems your fix makes the local weak symbol defined by protobuf public. How does that fix the problem?
  3. what will happen if another library links protobuf with this fix? And what will happen if another library links protobuf without this fix?

@TeBoring TeBoring self-assigned this Apr 23, 2021
@TeBoring
Copy link
Copy Markdown
Contributor

TeBoring commented Apr 24, 2021 •
edited
Loading

Reply to my own questions:

  1. https://anadoxin.org/blog/control-over-symbol-exports-in-gcc.html/
    This gives a pretty good explanation. By default, all symbols are exposed. If both B1 and B2 statically link A and expose A's
    symbols, on the other hand, C dynamically links A will get different versions of symbols from B1 and B2, which mess things up.
  2. This fix restrict exported symbol from all to those specified.
  3. Other libraries need to do the same. Otherwise, those other libraries still create conflicts.

@TeBoring TeBoring merged commit 7ad0fd4 into protocolbuffers:master Apr 24, 2021
@crockeo crockeo mentioned this pull request Jun 3, 2021
Merged
goaway pushed a commit to envoyproxy/envoy-mobile that referenced this pull request Jun 8, 2021
@crockeo
python: hide symbols except for PyInit on macOS (#1506)
4ded40e 
Description: This PR modifies the build recipe for `envoy_engine.so` to hide all symbols except for `_PyInit_envoy_engine` by default on maOS, so that envoy_engine can be loaded alongside a [protobuf](http://pypi.org/project/protobuf) wheel without breaking.

Loading in protobuf alongside `envoy_engine.so` would register two sets of the same symbols and, on macOS, the runtime linker chooses the first symbol it finds causing problems. See the [similar protobuf change](protocolbuffers/protobuf#8346) and its [sister change in grpc](grpc/grpc#24992) for more information on what's going on.

Risk Level: Low
Testing: See the now-closed #1504 for how I've been testing this.
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Cerek Hillen <chillen@lyft.com>
@peteboothroyd
Copy link
Copy Markdown

peteboothroyd commented Nov 26, 2021

@TeBoring what happened to these changes? I can't see them in the current branch, nor in the git history for the setup.py file.

@peteboothroyd
Copy link
Copy Markdown

peteboothroyd commented Dec 14, 2021

@TeBoring @rowillia @jtattermusch these changes were removed here, and then some of the changes to setup.py were reverted here. However the changes in this PR were missed. I believe that this is probably still an ongoing issue with the protobuf package, unless some other change has obviated the need for this?

@peteboothroyd peteboothroyd mentioned this pull request Dec 14, 2021
Merged
@peteboothroyd peteboothroyd mentioned this pull request Jan 10, 2022
Closed
acozzette pushed a commit to acozzette/protobuf that referenced this pull request Jan 22, 2022
@rowillia
Make libprotobuf symbols local on OSX (protocolbuffers#8346)
18f8cd0 
@gnossen gave a great overview in grpc/grpc#24992 of the overall problem.

If a python process using both protobuf _and_ another native library linking in libprotobuf
frequently can cause crashes.  This seems to frequently affect tensorflow as well:

tensorflow/tensorflow#8394,
tensorflow/tensorflow#9525 (comment)
tensorflow/tensorflow#24976,
tensorflow/tensorflow#35573,
https://github.com/tensorflow/tensorflow/blob/v2.0.0/tensorflow/contrib/makefile/rename_protobuf.sh,
tensorflow/tensorflow#16104

Testing locally this fixes both crashes when linking in multiple versions of protobuf
and fixes `DescriptorPool` clashes as well (e.g. Python and Native code import different versions of the same message).
@acozzette acozzette mentioned this pull request Jan 22, 2022
Merged
acozzette pushed a commit that referenced this pull request Jan 25, 2022
@rowillia
Make libprotobuf symbols local on OSX (#8346) (#9435)
a035bd0 
@gnossen gave a great overview in grpc/grpc#24992 of the overall problem.

If a python process using both protobuf _and_ another native library linking in libprotobuf
frequently can cause crashes.  This seems to frequently affect tensorflow as well:

tensorflow/tensorflow#8394,
tensorflow/tensorflow#9525 (comment)
tensorflow/tensorflow#24976,
tensorflow/tensorflow#35573,
https://github.com/tensorflow/tensorflow/blob/v2.0.0/tensorflow/contrib/makefile/rename_protobuf.sh,
tensorflow/tensorflow#16104

Testing locally this fixes both crashes when linking in multiple versions of protobuf
and fixes `DescriptorPool` clashes as well (e.g. Python and Native code import different versions of the same message).

Co-authored-by: Roy Williams <roy.williams.iii@gmail.com>
@youchangkim youchangkim mentioned this pull request Apr 12, 2022
Closed
jpsim pushed a commit to envoyproxy/envoy that referenced this pull request Nov 28, 2022
@crockeo @jpsim
python: hide symbols except for PyInit on macOS (#1506)
c3faf5b 
Description: This PR modifies the build recipe for `envoy_engine.so` to hide all symbols except for `_PyInit_envoy_engine` by default on maOS, so that envoy_engine can be loaded alongside a [protobuf](http://pypi.org/project/protobuf) wheel without breaking.

Loading in protobuf alongside `envoy_engine.so` would register two sets of the same symbols and, on macOS, the runtime linker chooses the first symbol it finds causing problems. See the [similar protobuf change](protocolbuffers/protobuf#8346) and its [sister change in grpc](grpc/grpc#24992) for more information on what's going on.

Risk Level: Low
Testing: See the now-closed #1504 for how I've been testing this.
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Cerek Hillen <chillen@lyft.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit to envoyproxy/envoy that referenced this pull request Nov 29, 2022
@crockeo @jpsim
python: hide symbols except for PyInit on macOS (#1506)
e4d4127 
Description: This PR modifies the build recipe for `envoy_engine.so` to hide all symbols except for `_PyInit_envoy_engine` by default on maOS, so that envoy_engine can be loaded alongside a [protobuf](http://pypi.org/project/protobuf) wheel without breaking.

Loading in protobuf alongside `envoy_engine.so` would register two sets of the same symbols and, on macOS, the runtime linker chooses the first symbol it finds causing problems. See the [similar protobuf change](protocolbuffers/protobuf#8346) and its [sister change in grpc](grpc/grpc#24992) for more information on what's going on.

Risk Level: Low
Testing: See the now-closed #1504 for how I've been testing this.
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Cerek Hillen <chillen@lyft.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@TeBoring TeBoring TeBoring approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@TeBoring TeBoring

@anandolee anandolee

Labels

cla: yes python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rowillia @TeBoring @peteboothroyd @anandolee @protobuf-kokoro