-
-
Notifications
You must be signed in to change notification settings - Fork 603
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
Fixes memory leak in RSA-Signer #260
Conversation
src/Signer/Rsa.php
Outdated
@@ -52,7 +55,7 @@ | |||
* | |||
* @throws InvalidArgumentException | |||
*/ | |||
private function validateKey($key): void | |||
protected function validateKey($key): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this to private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can drop 235d487 to achieve that
@Ocramius Changes done. I'ld still change this method from private to protected, thus making overloading and using it in a child-class possible. |
This class is not designed for inheritance: another interface implementation is the correct approach here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fahrenholz thanks for your patch. I have a question, though. Shouldn't we call openssl_pkey_free()
after creating the signature with the private key as well?
@lcobucci Hm, yes, that would be good as it creates a new resource. I updated my PR. |
@Fahrenholz sorry about my delay, I have just one more question =) Do you know of any reliable way to test this? |
We can try to add tests in another PR, I'll be backporting this to 3.3 now. Thanks for your help! |
@lcobucci : I don't really know of a way to test it unfortunately. I can tell you how I found out: We had a long running process, with reactPHP as a server. After every request, we verified the included JWT in order to authenticate the user. And so after every 100 requests we saw that the PHP process had slightly grown, eventually getting so big every one or two weeks that it killed the container. Maybe a strategy would be to performance test it with jmeter or gatling. |
A test that instantiates the stack a thousand times and measures memory at
0, 500 and 1000 cycles (and expexts 1000===500 in memory usage) would be
enough
…On Sun, 9 Sep 2018, 20:49 Vincent Fahrenholz, ***@***.***> wrote:
@lcobucci </~https://github.com/lcobucci> : I don't really know of a way to
test it unfortunately. I can tell you how I found out: We had a long
running process, with reactPHP as a server. After every request, we
verified the included JWT in order to authenticate the user. And so after
every 100 requests we saw that the PHP process had slightly grown,
eventually getting so big every one or two weeks that it killed the
container.
Maybe a strategy would be to performance test it with jmeter or gatling.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#260 (comment)>, or mute
the thread
</~https://github.com/notifications/unsubscribe-auth/AAJakJr02rKGgXmCZZHnNy20-WPe4kiIks5uZWKfgaJpZM4WFUbt>
.
|
A SSL-Key-resource was created and never freed, thus creating a memory leak. This pull request fixes it by freeing the resource.