Skip to content

Fix a PHP memory leak with SSL root certificates#12706

Merged
ZhouyihaiDing merged 1 commit intogrpc:masterfrom
applinskinner:master
Dec 11, 2017
Merged

Fix a PHP memory leak with SSL root certificates#12706
ZhouyihaiDing merged 1 commit intogrpc:masterfrom
applinskinner:master

Conversation

@jackwakefield
Copy link
Copy Markdown
Contributor

@jackwakefield jackwakefield commented Sep 25, 2017 •
edited
Loading

I've been running into an issue with requests to PHP-FPM increasing memory by ~100MB which doesn't get freed.

I ran strace on PHP-FPM and noticed many calls to map vendor/grpc/grpc/etc/roots.pem into memory, narrowed this down to grpc_ssl_roots_override_result and PHP_METHOD(ChannelCredentials, setDefaultRootsPem) mallocing default_pem_root_certs without freeing, so replaced this with grp_realloc and made sure to make a copy of the value in grpc_ssl_roots_override_result.

I based this on the C# extension

grpc/src/csharp/ext/grpc_csharp_ext.c

Line 901 in 8399f92

static grpc_ssl_roots_override_result override_ssl_roots_handler(

This appears to be related to #11632

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Sep 25, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

14 similar comments
@grpc-kokoro
Copy link
Copy Markdown
Collaborator

grpc-kokoro commented Sep 25, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link
Copy Markdown
Collaborator

grpc-kokoro commented Sep 25, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Sep 25, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link
Copy Markdown
Collaborator

grpc-kokoro commented Sep 25, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link
Copy Markdown
Collaborator

grpc-kokoro commented Sep 25, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link
Copy Markdown
Collaborator

grpc-kokoro commented Sep 25, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link
Copy Markdown
Collaborator

grpc-kokoro commented Sep 25, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link
Copy Markdown
Collaborator

grpc-kokoro commented Sep 25, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Sep 25, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link
Copy Markdown
Collaborator

grpc-kokoro commented Sep 25, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link
Copy Markdown
Collaborator

grpc-kokoro commented Sep 25, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Sep 25, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Sep 25, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link
Copy Markdown
Collaborator

grpc-kokoro commented Sep 25, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@stanley-cheung stanley-cheung self-assigned this Sep 25, 2017
@stanley-cheung
Copy link
Copy Markdown
Contributor

stanley-cheung commented Sep 25, 2017

Ok to test

@RyanGordon
Copy link
Copy Markdown

RyanGordon commented Sep 29, 2017

Does this need a mutex around it? Not entirely sure if this is correct but potentially multiple PHP requests can be setting the default credentials at any point. Before it wouldn't be an issue since the memory would just leak and still be valid but now that it is being re'alloced it could cause problems?

@skormel
Copy link
Copy Markdown

skormel commented Nov 27, 2017

@jackwakefield Your patch solve ours memory leaks problems.

I think this patch must be included on the main tree.

Thanks you very much.

@thelinuxfoundation
Copy link
Copy Markdown

thelinuxfoundation commented Nov 27, 2017

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

Regards,
The Linux Foundation CLA GitHub bot

@jackwakefield
Copy link
Copy Markdown
Contributor Author

jackwakefield commented Nov 27, 2017

Signed.

@jackwakefield jackwakefield mentioned this pull request Nov 28, 2017
Closed
@ZhouyihaiDing
Copy link
Copy Markdown
Contributor

ZhouyihaiDing commented Dec 1, 2017 •
edited
Loading

Thank you for helping us find it.
The code looks good to me. I am going to make a test before merging it.
May I ask how you observe the memory usage? Using some php tools or read from the process directly?

@jackwakefield
Copy link
Copy Markdown
Contributor Author

jackwakefield commented Dec 2, 2017

I first picked it up in top after noticing performance degrading after each request, I then used strace as described here and noticed a large number of mmap calls mapping vendor/grpc/grpc/etc/roots.pem.

@ZhouyihaiDing
Copy link
Copy Markdown
Contributor

ZhouyihaiDing commented Dec 6, 2017

Jenkins, test this please

@ZhouyihaiDing
Copy link
Copy Markdown
Contributor

ZhouyihaiDing commented Dec 6, 2017

Sorry for the delay.
I run the tests in ubuntu and mac and it works well.
For the question about the lock, it's probably fine because setDefaultRootsPem is seldom called.
I think it's ready for merging after jenkins run the tests.
Thank you!

ZhouyihaiDing
ZhouyihaiDing approved these changes Dec 6, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@ZhouyihaiDing ZhouyihaiDing left a comment

Choose a reason for hiding this comment

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

LGTM

@ZhouyihaiDing ZhouyihaiDing merged commit f9675bb into grpc:master Dec 11, 2017
@ZhouyihaiDing ZhouyihaiDing mentioned this pull request Jan 16, 2018
Merged
nicolasnoble added a commit that referenced this pull request Jan 16, 2018
@nicolasnoble
Merge pull request #14035 from ZhouyihaiDing/v1.8.x
72da726 
Backport PR #14017 and #12706 into 1.8.x
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ZhouyihaiDing ZhouyihaiDing ZhouyihaiDing approved these changes

Assignees

@stanley-cheung stanley-cheung

Labels

cla: yes lang/php

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.