Skip to content

fix(agent): fix potential memleak when calling newrelic_end_transaction(true) #1072

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

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

ZNeumann
Copy link
Contributor

@ZNeumann ZNeumann commented May 30, 2025

Some transaction globals were leaked when calling newrelic_end_transaction(true). The freeing of these transaction globals has been moved to join all the other transaction globals to alleviate this.

In so doing, additional rshutdown methods are needed. This is because the transaction globals contain reference incremented zvals. If we wait until postdeactivate, those zvals will be in an undefined state and the data structures containing those references cannot be properly freed. In line with other transaction globals, we trigger a shutdown method during rshutdown to free these globals while the zvals are still valid.

@newrelic-php-agent-bot
Copy link

newrelic-php-agent-bot commented May 30, 2025

Test Suite Status Result
Multiverse 8/8 passing
SOAK 37/85 passing

@codecov-commenter
Copy link

codecov-commenter commented May 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.27%. Comparing base (aedc770) to head (e71d566).

Additional details and impacted files
@@              Coverage Diff              @@
##           zjn/valgrind    #1072   +/-   ##
=============================================
  Coverage         77.27%   77.27%           
=============================================
  Files               199      199           
  Lines             28327    28327           
=============================================
  Hits              21890    21890           
  Misses             6437     6437           
Flag Coverage Δ
agent-for-php-7.2 77.47% <100.00%> (ø)
agent-for-php-7.3 77.49% <100.00%> (ø)
agent-for-php-7.4 77.36% <100.00%> (ø)
agent-for-php-8.0 76.56% <100.00%> (ø)
agent-for-php-8.1 76.88% <100.00%> (ø)
agent-for-php-8.2 76.50% <100.00%> (ø)
agent-for-php-8.3 76.50% <100.00%> (ø)
agent-for-php-8.4 76.52% <100.00%> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ZNeumann ZNeumann force-pushed the zjn/valgrind branch 2 times, most recently from 5deff4d to e953a97 Compare June 16, 2025 15:05
@ZNeumann ZNeumann changed the base branch from zjn/valgrind to dev June 18, 2025 11:58
@ZNeumann ZNeumann marked this pull request as ready for review June 18, 2025 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants