[crac] RFR: Fix: arguments supplied to restore are split with whitespace

[Radim Vansa]

On Thu, 10 Aug 2023 16:02:32 GMT, YizhePKU <duke at openjdk.org> wrote:

> This PR fixes a bug that causes arguments with whitespaces to be split into multiple arguments during restore.
> 
> It contains two commits. The first commit is refactor only. It moves all side effects from `CracRestoreParameters` to the call sites, and changes the type of `CracRestoreParameters::args` from a single string to a `GrowableArray` of strings. Serialization code is also cleaned up a bit.
> 
> The second commit fixes the bug by introducing a new pseudo property, `-DCRaCJavaMainArgs`, that is set in `JavaMain`. An instance of `JavaMainArgs` (containing `argc` and `argv`) is stored as extra info of the property, which is later extracted in `Arguments::parse_options_for_restore` and passed to `crac::restore`.
> 
> Potential issues:
> 
> * We use `putenv` to modify environment variables, which expects `char*`. In this PR, I'm `const_cast`ing from `const char*` to `char*`, since I believe `putenv` doesn't actually modify the string. Is that OK? Maybe rewriting with `setenv` would be better?
> * `read_growable_array` is implemented by reading byte-at-a-time from the shared memory. Will it be too slow? I could rewrite it to read everything at once (like the original code did), but the current implementation is cute and I kinda want to keep it.

I like the more structured serialization/deserialization of the args, however the binary compatibility and reuse of structures makes me worried.

Could you also add a test that demonstrates arguments with spaces, and zero-length arguments?

src/hotspot/share/runtime/arguments.cpp line 2287:

> 2285:         }
> 2286:       }
> 2287:       else if (strcmp(key, "CRaCJavaMainArgs") == 0) {

System properties usually use the dotted lowercase notation; this looks more like a VM options. I suggest something like `jdk.internal.crac.mainArgs`.

src/hotspot/share/runtime/arguments.hpp line 151:

> 149: class ScopedVMInitArgs;
> 150: 
> 151: // Arguments passed from JavaMain via the property CRaCJavaMainArgs

Duplicating structure definition and relying on the same ABI is too fragile in my opinion. I understand that the options for passing across libraries is limited; escaped sequence as the actual value would be a better choice, though. I was also considering the `extraInfo` with a null-separated and null-null-terminated data, but individual argument can be empty so this wouldn't fly.

src/hotspot/share/runtime/crac.cpp line 551:

> 549: // Write a GrowableArray to fd.
> 550: // On error, return false.
> 551: static bool write_growable_array(int fd, GrowableArray<const char *>* array) {

Nitpick: can the array be const?

src/java.base/share/classes/jdk/crac/Core.java line 233:

> 231:         if (newArguments != null && newArguments.length > 0) {
> 232:             try {
> 233:                 Method newMain = AccessController.doPrivileged(new PrivilegedExceptionAction<Method>() {

I think that with recent changes the anonymous class could be converted back into more concise lambda, could you try?

-------------

PR Review: https://git.openjdk.org/crac/pull/101#pullrequestreview-1578139174
PR Review Comment: https://git.openjdk.org/crac/pull/101#discussion_r1294342755
PR Review Comment: https://git.openjdk.org/crac/pull/101#discussion_r1294328755
PR Review Comment: https://git.openjdk.org/crac/pull/101#discussion_r1294364359
PR Review Comment: https://git.openjdk.org/crac/pull/101#discussion_r1294311178