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

Allow getting and setting values from different threads in parallel #41

Merged
merged 1 commit into from
Jan 31, 2019
Merged

Allow getting and setting values from different threads in parallel #41

merged 1 commit into from
Jan 31, 2019

Conversation

MarkVillacampa
Copy link
Contributor

This solves #40

@patrickfav
Copy link
Owner

LGTM

@patrickfav patrickfav merged commit d47aca0 into patrickfav:master Jan 31, 2019
@MarkVillacampa
Copy link
Contributor Author

Taking another look at ByteArrayRuntimeObfuscator, I don't see a reason to add synchronized to createAndEncrypt, since it is only called in two places:

  • When instantiating the class, which should be thread-safe.
  • Inside getBytes, which is already synchronized.

I don't think it'd make much difference in performance, but I'll create If you agree I'll create another PR removing it, just for clarity and cleanliness :)

Also, according to Android studio, the wipe method in EncryptionFingerprint is never called. Is this right? shouldn't it be called after calling getBytes to wipe the unobfuscated bytes from memory?

If this is the case, EncryptionFingerprint would not be thread-safe if wipe and getBytes would be called simultaneously from two threads.

Lastly, any chance you could release a new version including this fix? It'd be really helpful for me, otherwise I'll have to find a way to compile and use a version from master in my app.

@patrickfav
Copy link
Owner

I don't see a reason to add synchronized to createAndEncrypt

True. Only thing it would guard is the constructor, but constructing is atomic afaik, ie. if it is not constructed nobody can call a method on it.

Also, according to Android studio, the wipe method in EncryptionFingerprint is never called. Is this right? shouldn't it be called after calling getBytes to wipe the unobfuscated bytes from memory?

Yes this is correct. This is more for the user of this library so it easily possible to wipe the fingerprint for them. In ByteArrayRuntimeObfuscatorthe createAndEncrypt after getting the bytes overwrites the internal byte array with a re-obfuscated version, so wipe is not necessary (if we are talking about the same thing)

If you want to apply another patch, pls open another PR. I forgot to tell you before last time, but please add the change in the CHANELOG. I will could release 0.8.0 today.

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.

2 participants