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.

Sign up for GitHub

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

Jump to bottom

Fix symbol visibility issues, add test for it #1359

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix symbol visibility issues, add test for it #1359

wants to merge 5 commits into from
+164 −9

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jun 26, 2023 •
edited
Loading

Closes #1181.

Catches the actual symbol visibility issue.

Replaces #1135.

@hebasto
Copy link
Member Author

hebasto commented Jun 27, 2023

Rebased on top of the merged #1356.

@hebasto
Copy link
Member Author

hebasto commented Jul 13, 2023

Rebased f816729 -> d7b1cea (pr1359.02 -> pr1359.03) due to the conflict with #1313.

@hebasto hebasto marked this pull request as draft July 13, 2023 17:36
@hebasto hebasto marked this pull request as ready for review July 13, 2023 17:47
@hebasto hebasto marked this pull request as draft August 31, 2023 14:35
@hebasto hebasto marked this pull request as ready for review February 12, 2024 18:31
@hebasto
Copy link
Member Author

hebasto commented Feb 12, 2024

@maflcko

Is it possible for a Cirrus persistent worker to do not override environment variables set with the ENV command in the Dockerfile?

@maflcko
Copy link
Contributor

maflcko commented Feb 13, 2024

Maybe --env-file drops ENV?

@hebasto
Copy link
Member Author

hebasto commented Apr 23, 2024

Rebased to resolve conflicts.

@real-or-random real-or-random mentioned this pull request Aug 24, 2024
Open
@hebasto hebasto force-pushed the 230626-visibility branch 5 times, most recently from 7f53ffc to 4ec4220 Compare August 25, 2024 16:18
@hebasto
Copy link
Member Author

hebasto commented Aug 25, 2024

#1595 (comment):

This reminds me that we should move forward with #1359.

Agreed.

Rebased and ready to move forward :)

@hebasto hebasto marked this pull request as draft October 16, 2024 13:07
@hebasto hebasto marked this pull request as ready for review October 16, 2024 14:34
@hebasto
Copy link
Member Author

hebasto commented Oct 16, 2024

Rebased.

theuni
theuni approved these changes Oct 16, 2024
View reviewed changes
Copy link
Contributor

@theuni theuni left a comment

Choose a reason for hiding this comment

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

Code review and approach ACK.

Cool that this caught a real (new) symbol leak.

@fanquake suggested that the test could potentially be simplified for c-i.

src/util_defs.h Outdated
/* Global variable visibility */
/* See: https://github.com/bitcoin-core/secp256k1/issues/1181 */
#if !defined(_WIN32) && defined(__GNUC__) && (__GNUC__ >= 4)
# define SECP256K1_LOCAL_VAR extern __attribute__ ((visibility ("hidden")))
Copy link
Contributor

Choose a reason for hiding this comment

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

The combo of extern + hidden isn't completely intuitive, but the gcc visibility guide confirms that it makes sense.

src/util_defs.h Outdated
@@ -0,0 +1,12 @@
#ifndef SECP256K1_UTIL_DEFS_H
#define SECP256K1_UTIL_DEFS_H
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: util_defs.h is pretty ambiguous. How about util_local_visibility.h or so?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Reworked.

CMakeLists.txt
@@ -267,8 +267,6 @@ else()
try_append_c_flags(-Wundef)
endif()

set(CMAKE_C_VISIBILITY_PRESET hidden)
Copy link
Contributor

Choose a reason for hiding this comment

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

In case it's not obvious to other reviewers, this is no longer needed because every non-static function should be externally visible.

If we ever need to share private symbols between compilation units this won't work. But I agree that it makes sense to take advantage of that if we can.

@hebasto
Copy link
Member Author

hebasto commented Oct 16, 2024

Cool that this caught a real (new) symbol leak.

To clarify, the fix is in the first commit. The excerpt from the CI log with an error message from the previous iteration of this branch looks as follows:

+ python3 ./tools/symbol-check.py .libs/libsecp256k1.so
.libs/libsecp256k1.so: export of symbol secp256k1_musig_nonce_gen_internal not expected
.libs/libsecp256k1.so: export of symbol secp256k1_musig_nonce_gen_internal not expected
.libs/libsecp256k1.so: failed EXPORTED_SYMBOLS
Error: Process completed with exit code 1.

@fanquake suggested that the test could potentially be simplified for c-i.

Thanks! The tools/symbol-check.py script has been simplified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theuni theuni theuni approved these changes

Assignees
No one assigned
Labels
assurance build
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Symbol visibility
4 participants
@hebasto @maflcko @theuni @real-or-random