RFR(XS) 8190809 JVM crashes while generating appcds for classpath with empty directory entry

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(XS) 8190809 JVM crashes while generating appcds for classpath with empty directory entry

Ioi Lam
Hi

Please review this very small change. I hope to invoke the trivial rule
so that I can push before today's 8PM PDT deadline for JDK 10 integration.

https://bugs.openjdk.java.net/browse/JDK-8190809
http://cr.openjdk.java.net/~iklam/jdk10/8190809-cds-crash-empty-cp.v01/

The crash happened because CDS was trying to use a classpath as a JAR
file, before checking it's actually a JAR file.

I added test case for the use of directories in -cp during CDS dumping,
both for empty and non-empty cases.

I will test with tier1/tier2 testing before pushing.

Thanks
- Ioi






PS: the check for non-emptiness is done in here:

void ClassLoader::check_shared_classpath(const char *path) {
   if (strcmp(path, "") == 0) {
     exit_with_path_failure("Cannot have empty path in archived
classpaths", NULL);
   }

   struct stat st;
   if (os::stat(path, &st) == 0) {
     if ((st.st_mode & S_IFMT) != S_IFREG) { // is not a regular file
       if (!os::dir_is_empty(path)) {
         tty->print_cr("Error: non-empty directory '%s'", path);
         exit_with_path_failure("CDS allows only empty directories in
archived classpaths", NULL);
       }
     }
   }
}


Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS) 8190809 JVM crashes while generating appcds for classpath with empty directory entry

Calvin Cheung
Hi Ioi,

The fix looks good.

thanks,
Calvin

On 12/1/17, 10:53 AM, Ioi Lam wrote:

> Hi
>
> Please review this very small change. I hope to invoke the trivial
> rule so that I can push before today's 8PM PDT deadline for JDK 10
> integration.
>
> https://bugs.openjdk.java.net/browse/JDK-8190809
> http://cr.openjdk.java.net/~iklam/jdk10/8190809-cds-crash-empty-cp.v01/
>
> The crash happened because CDS was trying to use a classpath as a JAR
> file, before checking it's actually a JAR file.
>
> I added test case for the use of directories in -cp during CDS
> dumping, both for empty and non-empty cases.
>
> I will test with tier1/tier2 testing before pushing.
>
> Thanks
> - Ioi
>
>
>
>
>
>
> PS: the check for non-emptiness is done in here:
>
> void ClassLoader::check_shared_classpath(const char *path) {
>   if (strcmp(path, "") == 0) {
>     exit_with_path_failure("Cannot have empty path in archived
> classpaths", NULL);
>   }
>
>   struct stat st;
>   if (os::stat(path, &st) == 0) {
>     if ((st.st_mode & S_IFMT) != S_IFREG) { // is not a regular file
>       if (!os::dir_is_empty(path)) {
>         tty->print_cr("Error: non-empty directory '%s'", path);
>         exit_with_path_failure("CDS allows only empty directories in
> archived classpaths", NULL);
>       }
>     }
>   }
> }
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS) 8190809 JVM crashes while generating appcds for classpath with empty directory entry

Ioi Lam
Thanks Calvin!

- Ioi


On 12/1/17 11:05 AM, Calvin Cheung wrote:

> Hi Ioi,
>
> The fix looks good.
>
> thanks,
> Calvin
>
> On 12/1/17, 10:53 AM, Ioi Lam wrote:
>> Hi
>>
>> Please review this very small change. I hope to invoke the trivial
>> rule so that I can push before today's 8PM PDT deadline for JDK 10
>> integration.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8190809
>> http://cr.openjdk.java.net/~iklam/jdk10/8190809-cds-crash-empty-cp.v01/
>>
>> The crash happened because CDS was trying to use a classpath as a JAR
>> file, before checking it's actually a JAR file.
>>
>> I added test case for the use of directories in -cp during CDS
>> dumping, both for empty and non-empty cases.
>>
>> I will test with tier1/tier2 testing before pushing.
>>
>> Thanks
>> - Ioi
>>
>>
>>
>>
>>
>>
>> PS: the check for non-emptiness is done in here:
>>
>> void ClassLoader::check_shared_classpath(const char *path) {
>>   if (strcmp(path, "") == 0) {
>>     exit_with_path_failure("Cannot have empty path in archived
>> classpaths", NULL);
>>   }
>>
>>   struct stat st;
>>   if (os::stat(path, &st) == 0) {
>>     if ((st.st_mode & S_IFMT) != S_IFREG) { // is not a regular file
>>       if (!os::dir_is_empty(path)) {
>>         tty->print_cr("Error: non-empty directory '%s'", path);
>>         exit_with_path_failure("CDS allows only empty directories in
>> archived classpaths", NULL);
>>       }
>>     }
>>   }
>> }
>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS) 8190809 JVM crashes while generating appcds for classpath with empty directory entry

Jiangli Zhou
In reply to this post by Ioi Lam
Looks good.

Thanks,
Jiangli

> On Dec 1, 2017, at 10:53 AM, Ioi Lam <[hidden email]> wrote:
>
> Hi
>
> Please review this very small change. I hope to invoke the trivial rule so that I can push before today's 8PM PDT deadline for JDK 10 integration.
>
> https://bugs.openjdk.java.net/browse/JDK-8190809
> http://cr.openjdk.java.net/~iklam/jdk10/8190809-cds-crash-empty-cp.v01/
>
> The crash happened because CDS was trying to use a classpath as a JAR file, before checking it's actually a JAR file.
>
> I added test case for the use of directories in -cp during CDS dumping, both for empty and non-empty cases.
>
> I will test with tier1/tier2 testing before pushing.
>
> Thanks
> - Ioi
>
>
>
>
>
>
> PS: the check for non-emptiness is done in here:
>
> void ClassLoader::check_shared_classpath(const char *path) {
>   if (strcmp(path, "") == 0) {
>     exit_with_path_failure("Cannot have empty path in archived classpaths", NULL);
>   }
>
>   struct stat st;
>   if (os::stat(path, &st) == 0) {
>     if ((st.st_mode & S_IFMT) != S_IFREG) { // is not a regular file
>       if (!os::dir_is_empty(path)) {
>         tty->print_cr("Error: non-empty directory '%s'", path);
>         exit_with_path_failure("CDS allows only empty directories in archived classpaths", NULL);
>       }
>     }
>   }
> }
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS) 8190809 JVM crashes while generating appcds for classpath with empty directory entry

Leonid Mesnik
In reply to this post by Ioi Lam
Fix looks good.
The only one question. Why don’t just always use current dir in 39?
39         File dir = new File(System.getProperty("test.classes", "."));
I think that jtreg always run test in new scratch directory however “test.classes” might be re-used between runs and people could have fail to create new directory if they re-run test locally.

Leonid

> On Dec 1, 2017, at 10:53 AM, Ioi Lam <[hidden email]> wrote:
>
> Hi
>
> Please review this very small change. I hope to invoke the trivial rule so that I can push before today's 8PM PDT deadline for JDK 10 integration.
>
> https://bugs.openjdk.java.net/browse/JDK-8190809
> http://cr.openjdk.java.net/~iklam/jdk10/8190809-cds-crash-empty-cp.v01/
>
> The crash happened because CDS was trying to use a classpath as a JAR file, before checking it's actually a JAR file.
>
> I added test case for the use of directories in -cp during CDS dumping, both for empty and non-empty cases.
>
> I will test with tier1/tier2 testing before pushing.
>
> Thanks
> - Ioi
>
>
>
>
>
>
> PS: the check for non-emptiness is done in here:
>
> void ClassLoader::check_shared_classpath(const char *path) {
>   if (strcmp(path, "") == 0) {
>     exit_with_path_failure("Cannot have empty path in archived classpaths", NULL);
>   }
>
>   struct stat st;
>   if (os::stat(path, &st) == 0) {
>     if ((st.st_mode & S_IFMT) != S_IFREG) { // is not a regular file
>       if (!os::dir_is_empty(path)) {
>         tty->print_cr("Error: non-empty directory '%s'", path);
>         exit_with_path_failure("CDS allows only empty directories in archived classpaths", NULL);
>       }
>     }
>   }
> }
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS) 8190809 JVM crashes while generating appcds for classpath with empty directory entry

Ioi Lam
Hi Leonid,

Thanks for the review. I've changed that line to

         File dir = new File(System.getProperty("user.dir"));

(which is similar to what TestCommon.java does with other temp files).

I'll rerun the tests now.

Thanks

- Ioi


On 12/1/17 11:32 AM, Leonid Mesnik wrote:

> Fix looks good.
> The only one question. Why don’t just always use current dir in 39?
> 39         File dir = new File(System.getProperty("test.classes", "."));
> I think that jtreg always run test in new scratch directory however
> “test.classes” might be re-used between runs and people could have
> fail to create new directory if they re-run test locally.
>
> Leonid
>
>> On Dec 1, 2017, at 10:53 AM, Ioi Lam <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>> Hi
>>
>> Please review this very small change. I hope to invoke the trivial
>> rule so that I can push before today's 8PM PDT deadline for JDK 10
>> integration.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8190809
>> http://cr.openjdk.java.net/~iklam/jdk10/8190809-cds-crash-empty-cp.v01/
>>
>> The crash happened because CDS was trying to use a classpath as a JAR
>> file, before checking it's actually a JAR file.
>>
>> I added test case for the use of directories in -cp during CDS
>> dumping, both for empty and non-empty cases.
>>
>> I will test with tier1/tier2 testing before pushing.
>>
>> Thanks
>> - Ioi
>>
>>
>>
>>
>>
>>
>> PS: the check for non-emptiness is done in here:
>>
>> void ClassLoader::check_shared_classpath(const char *path) {
>>   if (strcmp(path, "") == 0) {
>>     exit_with_path_failure("Cannot have empty path in archived
>> classpaths", NULL);
>>   }
>>
>>   struct stat st;
>>   if (os::stat(path, &st) == 0) {
>>     if ((st.st_mode & S_IFMT) != S_IFREG) { // is not a regular file
>>       if (!os::dir_is_empty(path)) {
>>         tty->print_cr("Error: non-empty directory '%s'", path);
>>         exit_with_path_failure("CDS allows only empty directories in
>> archived classpaths", NULL);
>>       }
>>     }
>>   }
>> }
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS) 8190809 JVM crashes while generating appcds for classpath with empty directory entry

Leonid Mesnik
Thank you for update. Looks good.

Leonid

> On Dec 1, 2017, at 11:42 AM, Ioi Lam <[hidden email]> wrote:
>
> Hi Leonid,
>
> Thanks for the review. I've changed that line to
>
>         File dir = new File(System.getProperty("user.dir"));
> (which is similar to what TestCommon.java does with other temp files).
>
> I'll rerun the tests now.
>
> Thanks
>
> - Ioi
>
> On 12/1/17 11:32 AM, Leonid Mesnik wrote:
>> Fix looks good.
>> The only one question. Why don’t just always use current dir in 39?
>> 39         File dir = new File(System.getProperty("test.classes", "."));
>> I think that jtreg always run test in new scratch directory however “test.classes” might be re-used between runs and people could have fail to create new directory if they re-run test locally.
>>
>> Leonid
>>
>>> On Dec 1, 2017, at 10:53 AM, Ioi Lam <[hidden email] <mailto:[hidden email]>> wrote:
>>>
>>> Hi
>>>
>>> Please review this very small change. I hope to invoke the trivial rule so that I can push before today's 8PM PDT deadline for JDK 10 integration.
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8190809 <https://bugs.openjdk.java.net/browse/JDK-8190809>
>>> http://cr.openjdk.java.net/~iklam/jdk10/8190809-cds-crash-empty-cp.v01/ <http://cr.openjdk.java.net/~iklam/jdk10/8190809-cds-crash-empty-cp.v01/>
>>>
>>> The crash happened because CDS was trying to use a classpath as a JAR file, before checking it's actually a JAR file.
>>>
>>> I added test case for the use of directories in -cp during CDS dumping, both for empty and non-empty cases.
>>>
>>> I will test with tier1/tier2 testing before pushing.
>>>
>>> Thanks
>>> - Ioi
>>>
>>>
>>>
>>>
>>>
>>>
>>> PS: the check for non-emptiness is done in here:
>>>
>>> void ClassLoader::check_shared_classpath(const char *path) {
>>>   if (strcmp(path, "") == 0) {
>>>     exit_with_path_failure("Cannot have empty path in archived classpaths", NULL);
>>>   }
>>>
>>>   struct stat st;
>>>   if (os::stat(path, &st) == 0) {
>>>     if ((st.st_mode & S_IFMT) != S_IFREG) { // is not a regular file
>>>       if (!os::dir_is_empty(path)) {
>>>         tty->print_cr("Error: non-empty directory '%s'", path);
>>>         exit_with_path_failure("CDS allows only empty directories in archived classpaths", NULL);
>>>       }
>>>     }
>>>   }
>>> }
>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS) 8190809 JVM crashes while generating appcds for classpath with empty directory entry

Ioi Lam
Hi,

I had to change the test because of JDK-8192927 "os::dir_is_empty is
incorrect on Windows"

import jdk.test.lib.Platform;
import jdk.test.lib.process.OutputAnalyzer;
import java.io.File;

public class DirClasspathTest {
     public static void main(String[] args) throws Exception {
         File dir = new File(System.getProperty("user.dir"));
         File emptydir = new File(dir, "emptydir");
         emptydir.mkdir();

         // Empty dir in -cp: should be OK
         OutputAnalyzer output;
         if (!Platform.isWindows()) {
             // This block fails on Windows because of JDK-8192927
             output = TestCommon.dump(emptydir.getPath(),
TestCommon.list("DoesntMatter"), "-Xlog:class+path=info");
             TestCommon.checkDump(output);
         }

         // Non-empty dir in -cp: should fail
         // <dir> is not empty because it has at least one subdirectory,
i.e., <emptydir>
         output = TestCommon.dump(dir.getPath(),
TestCommon.list("DoesntMatter"), "-Xlog:class+path=info");
         output.shouldNotHaveExitValue(0);
         output.shouldContain("CDS allows only empty directories in
archived classpaths");
     }
}

So the test for "-cp emptydir" is now disabled on Windows.

Thanks

- Ioi



On 12/1/17 11:44 AM, Leonid Mesnik wrote:

> Thank you for update. Looks good.
>
> Leonid
>
>> On Dec 1, 2017, at 11:42 AM, Ioi Lam <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>> Hi Leonid,
>>
>> Thanks for the review. I've changed that line to
>>
>>         File dir = new File(System.getProperty("user.dir"));
>>
>> (which is similar to what TestCommon.java does with other temp files).
>>
>> I'll rerun the tests now.
>>
>> Thanks
>>
>> - Ioi
>>
>>
>> On 12/1/17 11:32 AM, Leonid Mesnik wrote:
>>> Fix looks good.
>>> The only one question. Why don’t just always use current dir in 39?
>>> 39         File dir = new File(System.getProperty("test.classes", "."));
>>> I think that jtreg always run test in new scratch directory however
>>> “test.classes” might be re-used between runs and people could have
>>> fail to create new directory if they re-run test locally.
>>>
>>> Leonid
>>>
>>>> On Dec 1, 2017, at 10:53 AM, Ioi Lam <[hidden email]
>>>> <mailto:[hidden email]>> wrote:
>>>>
>>>> Hi
>>>>
>>>> Please review this very small change. I hope to invoke the trivial
>>>> rule so that I can push before today's 8PM PDT deadline for JDK 10
>>>> integration.
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8190809
>>>> http://cr.openjdk.java.net/~iklam/jdk10/8190809-cds-crash-empty-cp.v01/
>>>>
>>>> The crash happened because CDS was trying to use a classpath as a
>>>> JAR file, before checking it's actually a JAR file.
>>>>
>>>> I added test case for the use of directories in -cp during CDS
>>>> dumping, both for empty and non-empty cases.
>>>>
>>>> I will test with tier1/tier2 testing before pushing.
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> PS: the check for non-emptiness is done in here:
>>>>
>>>> void ClassLoader::check_shared_classpath(const char *path) {
>>>>   if (strcmp(path, "") == 0) {
>>>>     exit_with_path_failure("Cannot have empty path in archived
>>>> classpaths", NULL);
>>>>   }
>>>>
>>>>   struct stat st;
>>>>   if (os::stat(path, &st) == 0) {
>>>>     if ((st.st_mode & S_IFMT) != S_IFREG) { // is not a regular file
>>>>       if (!os::dir_is_empty(path)) {
>>>>         tty->print_cr("Error: non-empty directory '%s'", path);
>>>>         exit_with_path_failure("CDS allows only empty directories
>>>> in archived classpaths", NULL);
>>>>       }
>>>>     }
>>>>   }
>>>> }
>>>>
>>>>
>>>
>>
>