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

appsec: fix recv/writev calls in the face of interrupting signals #3008

Merged
merged 2 commits into from
Jan 6, 2025

Conversation

cataphract
Copy link
Contributor

Description

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@cataphract cataphract requested a review from a team as a code owner December 19, 2024 17:13
@cataphract cataphract force-pushed the glopes/recv-writev-eintr branch from 5dd79af to f93a341 Compare December 19, 2024 17:16
@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2024

Codecov Report

Attention: Patch coverage is 51.94805% with 37 lines in your changes missing coverage. Please review.

Project coverage is 72.69%. Comparing base (dc9911b) to head (41d91c4).

Files with missing lines Patch % Lines
appsec/src/extension/network.c 40.67% 27 Missing and 8 partials ⚠️
appsec/src/extension/commands_helpers.c 88.88% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3008      +/-   ##
============================================
- Coverage     72.74%   72.69%   -0.05%     
  Complexity     2750     2750              
============================================
  Files           138      138              
  Lines         15060    15049      -11     
  Branches       1026     1022       -4     
============================================
- Hits          10955    10940      -15     
- Misses         3546     3556      +10     
+ Partials        559      553       -6     
Flag Coverage Δ
appsec-extension 67.82% <51.94%> (-0.14%) ⬇️
tracer-php 74.55% <ø> (-0.02%) ⬇️

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

Files with missing lines Coverage Δ
appsec/src/extension/network.h 100.00% <ø> (ø)
appsec/src/extension/commands_helpers.c 62.03% <88.88%> (-1.63%) ⬇️
appsec/src/extension/network.c 43.36% <40.67%> (-0.12%) ⬇️

... and 1 file 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 dc9911b...41d91c4. Read the comment docs.

@cataphract cataphract force-pushed the glopes/recv-writev-eintr branch from f93a341 to 2039f1b Compare December 19, 2024 17:26
@cataphract cataphract force-pushed the glopes/recv-writev-eintr branch from 2039f1b to cf7254e Compare December 19, 2024 17:26
@pr-commenter
Copy link

pr-commenter bot commented Dec 19, 2024

Benchmarks [ appsec ]

Benchmark execution time: 2024-12-20 16:33:05

Comparing candidate commit 41d91c4 in PR branch glopes/recv-writev-eintr with baseline commit dc9911b in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics.

static inline dd_result _imsg_recv(
dd_imsg *nonnull imsg, dd_conn *nonnull conn);
static inline ATTR_WARN_UNUSED dd_result _imsg_recv_cred(
// iif this returns success, _imsg_destroy must be called
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it's correct, _imsg_destroy should be called if and only if _imsg_recv returns success. I did spot a branch where it was not being called, though, so I added a new commit to use a gcc/clang extension to prevent this.

@cataphract cataphract force-pushed the glopes/recv-writev-eintr branch from fe08758 to e453e54 Compare December 20, 2024 15:48
@cataphract cataphract force-pushed the glopes/recv-writev-eintr branch from e453e54 to 41d91c4 Compare December 20, 2024 15:53
@cataphract cataphract merged commit 98428a8 into master Jan 6, 2025
719 of 752 checks passed
@cataphract cataphract deleted the glopes/recv-writev-eintr branch January 6, 2025 10:20
@github-actions github-actions bot added this to the 1.6.0 milestone Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants