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

Memory leak in Prf #53

Closed
behrooz-stripe opened this issue Feb 28, 2025 · 6 comments
Closed

Memory leak in Prf #53

behrooz-stripe opened this issue Feb 28, 2025 · 6 comments
Assignees

Comments

@behrooz-stripe
Copy link
Contributor

Help us help you

We'd like to know more about
your Tink deployment.

Describe the bug:

Memory leak in Tink Prf Java See here for detailed description: #52

What was the expected behavior?

Not to drain memory in repetitive calls to prf.compute().

How can we reproduce the bug?

See here for reproducing steps: #52

Do you have any debugging information?
Using memory profiler this is the leak stacktrace:

 java.lang.Thread.State: RUNNABLE
	at jdk.internal.misc.Unsafe.allocateMemory0(java.base@17.0.14/Native Method)
	at jdk.internal.misc.Unsafe.allocateMemory(java.base@17.0.14/Unsafe.java:630)
	at java.nio.DirectByteBuffer.<init>(java.base@17.0.14/DirectByteBuffer.java:125)
	at java.nio.ByteBuffer.allocateDirect(java.base@17.0.14/ByteBuffer.java:332)
	at com.google.crypto.tink.subtle.prf.HkdfStreamingPrf$HkdfInputStream.initialize(HkdfStreamingPrf.java:88)
	at com.google.crypto.tink.subtle.prf.HkdfStreamingPrf$HkdfInputStream.read(HkdfStreamingPrf.java:129)
	at com.google.crypto.tink.subtle.prf.PrfImpl.readBytesFromStream(PrfImpl.java:45)
	at com.google.crypto.tink.subtle.prf.PrfImpl.compute(PrfImpl.java:67)
	at com.google.crypto.tink.prf.PrfSet.computePrimary(PrfSet.java:44)

See here for
What version of Tink are you using?

For example, 1.7.0. but it's present on master too.

Can you tell us more about your development environment?

Such as JDK 8

Is there anything else you'd like to add?

@tholenst tholenst self-assigned this Mar 3, 2025
@tholenst
Copy link
Contributor

tholenst commented Mar 3, 2025

Thanks for the detailed report and the proposed fix!

As you can see on the PR we need a CLA to merge the PR. If this is a problem, let me know, and I can simply fix it myself.

I think it shouldn't really be a full leak since eventually the cleaner in DirectByteBuffer should clean it up -- but clearly this is just picking on wording, and in the end I believe you it will run out of memory when this is called in a tight loop. In any case, this is just a wrong use of a direct byte buffer.

@behrooz-stripe
Copy link
Contributor Author

Hi @tholenst
Thanks for looking into this. I believe we (Stripe Inc) have a corporate CLA in place with Google that I am a member of. Not sure why the check doesn't reflect this, maybe it needs to be rerun since I recently joined this CLA?

Image

@behrooz-stripe
Copy link
Contributor Author

Alright should be good now, my commit associated email was wonky.

@behrooz-stripe
Copy link
Contributor Author

@tholenst it should be ready to go.

@tholenst
Copy link
Contributor

tholenst commented Mar 7, 2025

Thank you, yes. This should be merged. I had to guide it through our review process. Also, please note that I'm not very familiar with Github and our tooling, so if something didn't work as expected let me know here.

@behrooz-stripe
Copy link
Contributor Author

Closed by this commit.
Thank you for your help @tholenst

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

No branches or pull requests

2 participants