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 memory leak that occurs when exceptions occur in stacked segments #123

Merged
merged 1 commit into from
Mar 29, 2021

Conversation

joshuabenuck
Copy link
Contributor

This PR fixes a couple of memory leaks introduced in v9.12 of the agent.

  • A string that wasn't freed in nr_php_error_record_exception_segment.
  • The memory pointed to by segment->error when an exception occurs in a stacked segment that is not converted to a heap segment.

The fix for the latter involves converting the stacked segment to a heap allocated segment prior to deallocation if segment->error is set. The pre-existing code does the same thing if segment->attributes is set.

@joshuabenuck
Copy link
Contributor Author

ok jenkins

@joshuabenuck
Copy link
Contributor Author

I was unable to create an automated test to reproduce this. Here is the manual approach I used should we want to try again in the future.

Create a php file with the following contents:

<?php    
function recursiveCount(int $level): void    
{    
    if ($level <= 1) {    
        //throw new \Exception('Oops');    
        return;    
    }    
    
    recursiveCount($level - 1);    
}    
    
for($i= 0; $i<2; ++$i) {    
    try {    
        recursiveCount(1);    
    } catch(\Exception $e) {    
        // continue    
    }    
}    

Ensure that distributed tracing is enabled and that tracer details are set to 1.

Run the script under valgrind (and remove the changes from this PR in your agent build). If successful, you should see a memory leak after running the script a couple of times as the agent must be able to successfully connect to New Relic for the memory leak to occur.

Increasing the recursion count and the number of iterations through the loop will also help to increase the chances of causing a memory leak. The leak occurs when there are short lived segments that remain on the stack and that throw an exception.

@joshuabenuck
Copy link
Contributor Author

ok jenkins

@joshuabenuck
Copy link
Contributor Author

ok jenkins

1 similar comment
@joshuabenuck
Copy link
Contributor Author

ok jenkins

Copy link
Contributor

@kneitinger kneitinger left a comment

Choose a reason for hiding this comment

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

Well done! Thanks for investigating and fixing this!

@joshuabenuck joshuabenuck merged commit caf3847 into newrelic:dev Mar 29, 2021
@joshuabenuck joshuabenuck deleted the fix-segment-memleak branch March 29, 2021 18:32
joshuabenuck pushed a commit to joshuabenuck/newrelic-php-agent that referenced this pull request Apr 19, 2021
joshuabenuck pushed a commit that referenced this pull request Apr 20, 2021
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.

None yet

3 participants