RFR: 8339460: CDS error when module is located in a directory with space in the name [v2]

[Calvin Cheung]

On Tue, 24 Sep 2024 05:22:39 GMT, Maxim Kartashev <mkartashev at openjdk.org> wrote:

>> src/hotspot/share/classfile/classLoader.cpp line 1236:
>> 
>>> 1234:     if (str[index] == '%'
>>> 1235:             && isxdigit(str[index + 1])
>>> 1236:             && isxdigit(str[index + 2])) {
>> 
>> I think you need to bound check the index because the `uri` from `ClassLoaderExt::process_module_table `could just be a directory name, e.g. dir%, without the jar file name.
>
> `str` is still zero-terminated, so if `str[index] == '%'` is true, then `str[index + 1]` at least exists and might be `0`. If `isxdigit(str[index + 1])` is true then `str[index + 2]` at least exists and might be `0`, in which case `isxdigit()` is going to be false for it.

So if `isxdigit(str[index + 1])` is false, it won't proceed to check `isxdigit(str[index + 2])`?
The bound check I'm thinking about is like the following:

if (index >= strlen(str) - 2) {
  return str[index];
}

>> test/hotspot/jtreg/runtime/cds/appcds/complexURI/ComplexURITest.java line 46:
>> 
>>> 44: public class ComplexURITest {
>>> 45:     final static String listFileName = "test-classlist.txt";
>>> 46:     final static String archiveName  = "test-dynamic.jsa";
>> 
>> You're using the same `archiveName` for both static and dynamic test cases. I'd suggest declare a different archive name for the static case. E.g.
>> 
>> final static String staticArchiveName = "test-static.jsa";
>> final static String dynamicArchiveName = "test-dynamic.jsa";
>> 
>> Also, I think this test should be excluded from the `hotspot_appcds_dynamic` test group in the `open/test/hotspot/jtreg/TEST.groups` file.
>
> Done. I also added the code to safeguard against `IOException` on Windows in the case of deleting the read-only archive created previously.

Test changes look good. I will run some testing on your patch.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20987#discussion_r1773735662
PR Review Comment: https://git.openjdk.org/jdk/pull/20987#discussion_r1773735810