Add Java-side cache with large-request bypass for native RNG#1221
Add Java-side cache with large-request bypass for native RNG#1221taoliult wants to merge 1 commit into
Conversation
|
The code looks okay for what it is meant to do. However, this could be a security issue. What is the use case here that we are trying to resolve? |
| final long ockPRNGContextId; | ||
|
|
||
| // 128KB cache ? | ||
| private static final int MEGABYTE_CACHE_SIZE = 128 * 1024; |
There was a problem hiding this comment.
Can this be a configurable size. By default one megabyte seems reasonable to me given your findings.
There was a problem hiding this comment.
Yes, I will work on making the cache size configurable. From my testing, using a cache size between 128 KB and 1 MB shows no noticeable difference in the RandomBenchmark results. So, I did not see any performance difference from a larger cache. So, using a smaller size would likely be the better option?
| private static final int MEGABYTE_CACHE_SIZE = 128 * 1024; | ||
|
|
||
| // 16KB threshold ? | ||
| private static final int BYPASS_THRESHOLD = 16 * 1024; |
There was a problem hiding this comment.
What is the threshold trying to accomplish? Seems like if the user wants more randoms then is in the cache then we would then make a trip to the native layer to fill the cache at that time and then return the bytes to the caller. This would save some complexity for now.
There was a problem hiding this comment.
About the threshold, the purpose of it, is to bypass the cache for requests whose size closes the cache capacity. In such cases, the request can fetch bytes directly from the native code, avoiding unnecessary cache copying.
For example, if the payload size exceeds roughly half of the cache size, getting it from the cache would likely require repeated refills, resulting in additional native calls and copying. In such cases, bypassing the cache should be more efficient.
But, the exact threshold value still needs to be tuned — for example, MEGABYTE_SIZE / 2 or MEGABYTE_SIZE / 4?
| @@ -16,6 +16,16 @@ public final class ExtendedRandom { | |||
| OCKContext ockContext; | |||
There was a problem hiding this comment.
Do we need any additional testing in our test buckets to test randoms from the cache right around the boundary limits for our cache?
There was a problem hiding this comment.
I ran the JMH RandomBenchmark in both single-threaded and multi-threaded modes, and the results are similar. I also ran the OpenJCEPlus functional tests, which all passed. For this Java-level change, I think the existing benchmark setup should be good enough.
There was a problem hiding this comment.
What i meant here was that for whatever default size we choose it would be good to generate that size - 1 byte, that size, and that size +1 byte in order to do boundary testing on the cache.
There was a problem hiding this comment.
Yes, I added the boundary tests around the cache limits: size - 1, size, size + 1, exact drain then one more byte, multi-step crossing, repeated refill, and a large bypass request. I think we now have a good coverage around the cache boundary.
@johnpeck-us-ibm Thanks for raising the security concern. We discussed this within the team and believe that, from a threat perspective, if an attacker is able to read JVM heap memory, they would already need a high level of access to the process or the host system. In that scenario, the attacker could already read the application’s own output buffers and other heap objects. So, storing the megabyte cache does not change the exposure compared with the behavior without the cache. Please advise if there are any particular security concerns that need to be considered. |
| // 16KB threshold ? | ||
| private static final int BYPASS_THRESHOLD = 16 * 1024; | ||
|
|
||
| private byte[] megaByteCache; |
There was a problem hiding this comment.
I guess we need a more size agnostic name here.
There was a problem hiding this comment.
Name was changed since the cache no needs to be megabyte-sized.
| NativeInterface.EXTRAND_nextBytes(ockContext.getId(), ockPRNGContextId, bytes); | ||
|
|
||
| // Invalidate cache so next small request refills fresh. | ||
| cachePos = megaByteCacheLength = 0; |
There was a problem hiding this comment.
Not sure why you want to do this. If the cache wasn't touched why invalidate it?
There was a problem hiding this comment.
Yes, if there are still cached bytes that have not been used, there is no need to reset the cache. Removed this reset.
| // 2) SMALL/MEDIUM REQUEST: | ||
| // Serve from cache, refilling as needed. | ||
| int outPos = 0; | ||
| int remaining = len; |
There was a problem hiding this comment.
The name is not very clear. Maybe something like needed would make it easier to read.
b6604b5 to
b7d7ee9
Compare
| @@ -16,6 +16,16 @@ public final class ExtendedRandom { | |||
| OCKContext ockContext; | |||
There was a problem hiding this comment.
What i meant here was that for whatever default size we choose it would be good to generate that size - 1 byte, that size, and that size +1 byte in order to do boundary testing on the cache.
1c39578 to
63f61ad
Compare
I have two security concerns:
|
d4f4097 to
ada8a73
Compare
Introduce a Java-side cache to reduce native RNG calls for small and medium SecureRandom requests. Previously, each nextBytes() call delegated directly to the native RNG, causing frequent calls and extra overhead for workloads that repeatedly request small buffers. With this change: • A large cache is filled from the native RNG and serves small and medium requests, reducing native invocations. • Large requests bypass the cache and are filled directly from the native RNG to avoid extra copy overhead. • After a bypass, the cache is invalidated so later small requests will refill from fresh native call. Signed-off-by: Tao Liu <tao.liu@ibm.com>
4dbf198 to
28828e9
Compare
On the first point, I agree that the Java-side cache increases both the lifetime and the amount of random output resident in heap memory. Without the cache, typically only the bytes already requested by the caller are exposed. With the cache, unread future output may also remain in memory and could be disclosed in a heap dump. But, my understanding is that an attacker capable of reading JVM heap memory would already have a very high level of access to the system. And I am thinking, another option which could help reduce this risk is, maybe provide a flag to enable or disable the Java-side cache, and to document the behavior clearly so clients can decide whether to use it based on their security requirements and deployment environment? On the second point, I agree that the cache cannot remain valid across setSeed(). To address that, I added a clearRandomByteCache() method so that the cache is invalidated and cleared whenever setSeed() is invoked. |
Introduce a Java-side cache to reduce native RNG calls for small and medium SecureRandom requests.
Previously, each nextBytes() call delegated directly to the native RNG, causing frequent calls and extra overhead for workloads that repeatedly request small buffers.
With this change:
• A large cache is filled from the native RNG and serves small and medium requests, reducing native invocations.
• Large requests bypass the cache and are filled directly from the native RNG to avoid extra copy overhead.
• After a bypass, the cache is invalidated so later small requests will refill from fresh native call.