Disallow Random/SplittableRandom in the image heap and initialize relevant classes at runtime

[pivovarit]

Disallowing Random and SplittableRandom presence in the image heap since it can result in undesired side-effects.

The "failure" happens at run time but is caused by an unwanted build-time initialization of one class which results in unwanted caching.

---

Reproducer:

echo "public class Hello { public static void main(String[] args) { System.out.println(StrictMath.random()); }}" >> Hello.java
javac Hello.java
native-image Hello

and then the fun begins:

❯ ./hello  
0.7712305575721234
❯ ./hello  
0.7712305575721234
❯ ./hello  
0.7712305575721234

[graalvmbot]

Hello Grzegorz Piwowarek, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address gpiwowarek -(at)- gmail -(dot)- com. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

[vjovanov]

@pivovarit thanks for fixing this! I am wondering does the failure happens at build-time or at runtime?

I think we should really detect this case at build-time.

[vjovanov]

Could you please sign the OCA and we can merge?

[vjovanov]

I can't think of case where one would like to have a Random initialized at build-time, but I can think of a million cases where one dosn't want to do that.

Can we add a check to DisallowedImageHeapObjects that verifies that RandomNumbers don't end up in the heap?

@christianwimmer @cstancu do you see a reason not to disallow Random in the image heap?

[pivovarit]

The only reason I could think of was demoing the unwanted side-effects of build-time initialization.

As soon as I get a green light, I'll add it to disallowed objects.

[christianwimmer]

Thanks for detecting this issue.

And yes, we should disallow instances of Random and SplittableRandom in the image heap. Better to fail at image build time than to have non-random values at run time.

[pivovarit]

Well, that's unfortunate: https://github.com/openjdk/jdk/blob/65393a093c6a43a0bd8ff9f79c29c3bc5756104f/src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java#L104-L109

Will take care of that in a separate commit soon.

[vjovanov]

Can't wait to run tests to see what other bugs we found :)

[vjovanov]

Thoughtfull--thanks!

[vjovanov]

LGTM. Running the tests now.

[pivovarit]

OCA signed. Got a confirmation from Dalibor

[pivovarit]

@vjovanov any results yet?

[vjovanov]

Yes, it has discovered a few issues in the internal tests. I will merge this until the end of the year. Is that OK?

[pivovarit]

Sure!

[vjovanov]

Will merge it in the next day or two (due to failing tests in downstream projects). Will make a backport PR without the strict check to 23.1. Hope it makes it.

[zakkak]

This has been merged in a975ef5 and backported to 20.3.1 in 19298b7

[jas88]

I can't think of case where one would like to have a Random initialized at build-time, but I can think of a million cases where one dosn't want to do that.

I happened to come across this thread working on exactly such a case - I'm trying to build a text parsing tool which uses a library which happens to use a SplittableRandom somewhere in its internal data structures. No interest at all in making the values non-deterministic across runs - indeed probably slightly preferably not to - but very interested in being able to use the library from compiled code, and pre-building the lookup table rather than re-doing it each startup. Would it be possible to suppress this check for such cases please? --DisableRandomChecks or similar, if there isn't already an analogous option somewhere.

[pivovarit]

@jas88 why would you ever want to do that? this is also how standard JVM behaves by default

[jas88]

@pivovarit When did the standard JVM start precompiling to native executables at all, let alone with filtering whether or not you've used a pseudo-random number? What you described at the start of this thread as "undesired side-effects" is entirely harmless for my use case (arguably beneficial in some ways), unlike the compilation failure I'm dealing with as a result of this patch.

If the behaviour involved _Secure_Random (which implies the caller actually needs something genuinely non-predictable), or a situation where you genuinely want non-deterministic "randomness", fair enough, but I for one want code that actually compiles and works rather than a spurious error message.

Presumably the best workaround for my case in the mean time is to pin to building with Graal 21.3.0? I could try sending a patch upstream to that library to remove the use of SplittableRandom when loading as a longer term fix for my case.

[pivovarit]

Are you sure your code is not compiling due to this patch? At the time of implementation, it was just changing the runtime behaviour and not rejecting anything that uses SplittableRandom (and any other Random)

[jas88]

I was getting a compilation error: Error: Detected an instance of Random/SplittableRandom class in the image heap. Instances created during image generation have cached seed values and don't behave as expected. To see how this object got instantiated use --trace-object-instantiation=java.util.Random. The object was probably created by a class initializer and is reachable from a static field. You can request class initialization at image runtime by using the option --initialize-at-run-time=<class-name>. Or you can write your own initialization methods and call them explicitly from your main entry point.

IMO that should be a warning rather than an error?

Reverting to 20.3.0 gets past this build stage at least, but fails in compiling some glue code in PosixDirectives.c which seems unrelated. I'll try building a newer GraalVM from source with this check removed at some point.