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 a possible crash in zai config #2906

Merged
merged 1 commit into from
Oct 21, 2024
Merged

Fix a possible crash in zai config #2906

merged 1 commit into from
Oct 21, 2024

Conversation

bwoebi
Copy link
Collaborator

@bwoebi bwoebi commented Oct 21, 2024

We might leave a dangling pointer when we update orig_value, but not an identical value.

(Also removing a couple redundant copy+release operations.)

@bwoebi bwoebi requested a review from a team as a code owner October 21, 2024 12:54
@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.

Project coverage is 78.21%. Comparing base (46173ca) to head (d8a275c).

Files with missing lines Patch % Lines
zend_abstract_interface/config/config_ini.c 71.42% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2906      +/-   ##
============================================
- Coverage     80.91%   78.21%   -2.70%     
  Complexity     2526     2526              
============================================
  Files           146      173      +27     
  Lines         14713    18752    +4039     
  Branches          0      988     +988     
============================================
+ Hits          11905    14667    +2762     
- Misses         2808     3544     +736     
- Partials          0      541     +541     
Flag Coverage Δ
appsec-extension 68.37% <ø> (?)
tracer-extension 78.11% <71.42%> (+<0.01%) ⬆️
tracer-php 82.09% <ø> (ø)

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

Files with missing lines Coverage Δ
zend_abstract_interface/config/config_ini.c 73.13% <71.42%> (+0.13%) ⬆️

... and 27 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 46173ca...d8a275c. Read the comment docs.

@pr-commenter
Copy link

pr-commenter bot commented Oct 21, 2024

Benchmarks [ tracer ]

Benchmark execution time: 2024-10-21 17:29:06

Comparing candidate commit d8a275c in PR branch bob/fix-zai-cfg-crash with baseline commit 46173ca in branch master.

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

scenario:ComposerTelemetryBench/benchTelemetryParsing-opcache

  • 🟥 execution_time [+1.644µs; +3.556µs] or [+3.168%; +6.851%]

scenario:EmptyFileBench/benchEmptyFileBaseline

  • 🟩 execution_time [-184.526µs; -68.134µs] or [-6.186%; -2.284%]

scenario:PDOBench/benchPDOBaseline-opcache

  • 🟥 execution_time [+12.776µs; +13.581µs] or [+7.136%; +7.585%]

scenario:TraceFlushBench/benchFlushTrace

  • 🟥 execution_time [+1000.000ns; +1000.000ns] or [+100.000%; +100.000%]

Fix a possible crash in zai config

We might leave a dangling pointer when we update orig_value, but not an identical value.

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
@bwoebi bwoebi force-pushed the bob/fix-zai-cfg-crash branch from e4450b4 to d8a275c Compare October 21, 2024 17:01
@bwoebi bwoebi merged commit 9809185 into master Oct 21, 2024
717 of 742 checks passed
@bwoebi bwoebi deleted the bob/fix-zai-cfg-crash branch October 21, 2024 18:23
@github-actions github-actions bot added this to the 1.5.0 milestone Oct 21, 2024
bwoebi added a commit that referenced this pull request Oct 21, 2024
We might leave a dangling pointer when we update orig_value, but not an identical value.

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
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