[LANG-1772] Restrict size of cache to prevent overflow errors

[jcwinters]

Added a length restriction to RandomStringutils, limiting the cache to 60M entries. Because of rejections the bitIndex in the underling cache can overflow when right shifting. Also added a test to verify the fix.

This test takes quite a while to run, so if necessary I can create a profile for slow tests to exclude the test from the normal build.

[ppkarwasz]

Added a length restriction to RandomStringutils, limiting the cache to 60M entries. Because of rejections the bitIndex in the underling cache can overflow when right shifting. Also added a test to verify the fix.

The problem is caused by an integer overflow of bitIndex in:

commons-lang/src/main/java/org/apache/commons/lang3/CachedRandomBits.java

Line 95 in a511a16

result |= cache[bitIndex >> 3] >> (bitIndex & 0x7) & (1 << generatedBitsInIteration) - 1;

A simpler solution would be to:

• Change the type of bitIndex to long, so it can count up to 8 * Integer.MAX_VALUE
• Refactor the expression that computes the cacheSize argument in a way that it does not overflow:
  

  commons-lang/src/main/java/org/apache/commons/lang3/RandomStringUtils.java

  Line 332 in a511a16

  final CachedRandomBits arb = new CachedRandomBits((count * gapBits + 3) / 5 + 10, random);

[garydgregory]

The test as is blows up GitHub builds so let's use something like @EnabledIfSystemProperty(named = "test.large.heap", matches = "true")

[jcwinters]

A simpler solution would be to:

• Change the type of bitIndex to long, so it can count up to 8 * Integer.MAX_VALUE
• Refactor the expression that computes the cacheSize argument in a way that it does not overflow:

I considered that, changing bitindex to long adds some casting complexity I didn't want to deal with as arrays are int indexed. And I didn't particularly want to change the randomization algorithm, as to get rid of overflow possibilities I'd have to change to something that doesn't have rejections, and it made my head hurt.

[garydgregory]

Hi @ppkarwasz

You've proposed an alternative solution. Would you shows in a PR?

[ppkarwasz]

You've proposed an alternative solution. Would you shows in a PR?

I'll submit a PR by the end of the week.

[garydgregory]

@jcwinters
Thank you for your update.
I think the test should be more of a white box test and test just below and above the overflow. WDYT? @ppkarwasz ?

[ppkarwasz]

This looks OK to me.

I don’t think we need to preemptively generate more than 256 MiB of random data. The goal of generating data in bulk is to take advantage of the fact that random number generators are typically more efficient when producing large chunks of data rather than individual bytes. However, I suspect the optimal amount is significantly less than 256 MiB—we should run some benchmarks to determine the best value.

[garydgregory]

@jcwinters
Please 'mvn' by itself before you push to catch all build errors.

[jcwinters]

@jcwinters Please 'mvn' by itself before you push to catch all build errors.
@garydgregory Sorry about that, I'm so used to my workflow with pre_commit hooks running everything, I didn't even look. Thanks for the patience with the newbie, and I promise to do better 😄

[garydgregory]

If the 3 in this expression MUST match the 3 in the expression building cacheSize in the random(...) method, then it should be refactored from a magic number in to a constant IMO.

This makes me wonder about the other magic numbers 5 and 10 which beg for documentation if only to help with maintenance.

WDYT?

[ppkarwasz]

Nice, this looks better than the 0x1FFF_FFFF I proposed! 💯

Nit: following in the same direction you proposed, maybe Integer.MAX_VALUE >> 3 would be even easier to understand?

[ppkarwasz]

63_913_201 is the maximum allowed value for count, not cacheSize.

• If count is 63_913_201, then cacheSize is close to MAX_CACHE_SIZE.
• By increasing count further, the value of the expression (count * gapBits + 3) / 5 + 10 increases until it almost reaches Integer.MAX_VALUE / 5 + 10.

If we want cacheSize to be a non-decreasing function of count, we should use Integer.MAX_VALUE / 5 + 10 instead of 63_913_201`.