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

Fix improper symbol exports in appsec ext/helper #2854

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

cataphract
Copy link
Contributor

Description

Both the extension and the helper need version scripts to prevent symbols from being exported. -fvisibility=hidden is not sufficient because it's not being applied to some of the static libraries they link against.

On the extensions side: the path to the version script was wrong, as this error was ultimately ignored because invalid ld options were being ingored. This resulted in some extra zai_* functions being exported.

On the helper side: symbols from libunwind, libc++ and possibly others were being exported.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

Both the extension and the helper need version scripts to prevent
symbols from being exported. -fvisibility=hidden is not sufficient
because it's not being applied to some of the static libraries they link
against.

On the extensions side: the path to the version script was wrong, as
this error was ultimately ignored because invalid ld options were being
ingored. This resulted in some extra zai_* functions being exported.

On the helper side. Symbols from libunwind, libc++ and possibly others
were being exported.
@cataphract cataphract requested a review from a team as a code owner September 16, 2024 14:12
Copy link
Contributor

@Anilm3 Anilm3 left a comment

Choose a reason for hiding this comment

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

lgtm

@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.49%. Comparing base (31382ec) to head (7f4e779).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2854      +/-   ##
============================================
- Coverage     81.05%   78.49%   -2.57%     
  Complexity     2517     2517              
============================================
  Files           146      173      +27     
  Lines         14654    18683    +4029     
  Branches          0      975     +975     
============================================
+ Hits          11878    14665    +2787     
- Misses         2776     3481     +705     
- Partials          0      537     +537     
Flag Coverage Δ
appsec-extension 69.12% <ø> (?)
tracer-extension 78.18% <ø> (ø)
tracer-php 82.27% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 28 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31382ec...7f4e779. Read the comment docs.

Copy link
Collaborator

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

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

Works for me, thanks :-)

@cataphract cataphract merged commit 501f386 into master Sep 16, 2024
482 of 493 checks passed
@cataphract cataphract deleted the glopes/appsec-symbol-visi branch September 16, 2024 16:45
@github-actions github-actions bot added this to the 1.4.0 milestone Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants