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

[jcwinters]

https://issues.apache.org/jira/browse/LANG-1772

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?

[garydgregory]

Hi all,

Where are we on this one? @ppkarwasz do you still plan on providing an alternative solution?

[ppkarwasz]

Where are we on this one? @ppkarwasz do you still plan on providing an alternative solution?

This pull request appears to be nearly ready. It includes the following changes:

• Sets the maximum size of the internal CachedRandomBits class to Integer.MAX_VALUE / 8.
• Limits the maximum requested size for CachedRandomBits to approximately Integer.MAX_VALUE / 5.

I could switch the bitIndex field to long, which would allow increasing the maximum size of CachedRandomBits up to Integer.MAX_VALUE. However, it's unclear whether this would provide any practical benefit. A benchmark would likely show that caching such a large number of random bits offers no significant performance advantage.

[jcwinters]

I believe I've now incorporated most of the suggestions (I didn't use constants for the divide by 3 for instance) - sorry for the length of time this has taken

[garydgregory]

Hi @jcwinters,

No need to apologize, we're all busy 😃

[garydgregory]

Hi @jcwinters
I still don't understand what the magic numbers mean, so constants with comments or a better code comment is needed IMO.

[jcwinters]

I think I'm there now

[ppkarwasz]

Thank you for diligently addressing all of my feedback — I have no further suggestions.

As far as I’m concerned, the only remaining task is to add a changelog entry to src/changes/changes.xml.

@garydgregory, have your concerns been addressed as well? If so, I believe we're ready to merge this PR.