RFR 8020675: invalid jar file in the bootclasspath could lead to jvm fatal error

[Calvin Cheung]

On 8/22/2013 11:54 PM, David Holmes wrote:
> On 23/08/2013 4:05 PM, Calvin Cheung wrote:
>> On 8/22/2013 5:21 PM, David Holmes wrote:
>>> Hi Calvin,
>>>
>>> I'm having trouble keeping track of this one ...
>>>
>>> On 23/08/2013 8:55 AM, Calvin Cheung wrote:
>>>> Note that the synopsis of the bug has been changed from:
>>>> "Trying to print to a pdf file with PDFill PDF&Image Writer 10.0
>>>> Build 4"
>>>>
>>>> bug link: https://bugs.openjdk.java.net/browse/JDK-8020675
>>>>
>>>> I've included the suggestions from Coleen and Ioi in this version of
>>>> webrev:
>>>>      http://cr.openjdk.java.net/~ccheung/8020675/webrev/
>>>
>>> I don't understand your _has_error logic:
>>>
>>>  325   ClassPathEntry* cpe = resolve_entry(CHECK_NULL);
>>>  326   if (cpe == NULL) {
>>>  327     _has_error = true;
>>>  328     return NULL;
>>>
>>> we will only hit line 327 if resolve_entry returns NULL _without_
>>> there being an exception pending. How does that occur? What is
>>> different about the two cases regardless?
>> The _has_error flag was suggested by Ioi and is to avoid opening the
>> invalid jar file again.
>
> Can't we just remove it from the classpath representation?
I think it can be done if the error happens during vm initialization.
But for this test scenario, the error happens after vm init.
Perhaps this should be addressed as a separate bug fix?
>
>> The call sequence is as follows:
>> ClassLoader::load_classfile()
>>      -> LazyClassPathEntry::open_stream()
>>          -> LazyClassPathEntry::resolve_entry()
>>              -> ClassLoader::create_class_path_entry()
>>
>> For second time, it'll return NULL without calling resolve_entry() 
>> again.
>>
>> The LazyClassPathEntry instance is instantiated by the following calls:
>> ClassLoader::setup_bootstrap_search_path()
>>      ->ClassLoader::update_class_path_entry_list()
>>          ->ClassLoader::create_class_path_entry(path, &st,
>> LazyBootClassLoader, CHECK)
>>
>> LazyBootClassLoader is set to true by default.
>
> Sorry but I don't see how that answers my question. Based on the below 
> there isn't a second time because the first time will cause the 
> pending exception which will terminate the VM. Even if we dont 
> terminate and somehow call this again, we didn't set _has_error the 
> first time anyway! ???
Let's not confused with the classes we're preloading during vm 
initialization and the applications classes after vm initialization.

Consider the following test case:
public class test3 {
     public static void main(String[] args) {
         try {
             //Class cls = Class.forName("javax.crypto.KeyGenerator");
             //System.out.println("Class = " + cls.getName());
             Class cls = Class.forName("xxx");
             System.out.println("Class = " + cls.getName());
         } catch (ClassNotFoundException cnfe) {
             cnfe.printStackTrace();
         }
         try {
             Class cls2 = Class.forName("yyy");
             System.out.println("Class = " + cls2.getName());
         } catch (ClassNotFoundException cnfe) {
             cnfe.printStackTrace();
         }
     }
}

Let's say we have a dummy.jar with 0-byte and test3.class in the same 
directory.
And command line as follows:
java -Xbootclasspath/a:dummy.jar -cp . test3

During vm init. (as in the above 
ClassLoader::setup_bootstrap_search_path() call sequence), there's no 
problem because vm doesn't need to read dummy.jar as all other preload 
classes can be found in jdk's jar files (rt.jar, jce.jar, etc.) (Note: 
this isn't the case with 7u25, we were trying to preload a non-existing 
class, please refer to my comment in the bug report.)

When vm tries to load the class for the test case (test3.class), it'll 
call into LazyClassPathEntry::open_stream(). This is the first time 
resolve_entry() returns NULL and the _has_error flag will be set to 
true. The next time vm will try to load xxx.class. Since _has_error was 
set to NULL, it will return NULL without calling resolve_entry(). Same 
story for the third time for the yyy.class.

call stack as follows:
      jvm.dll!ClassLoader::create_class_path_entry(char * path, const 
stat * st, bool lazy, Thread * __the_thread__)  Line 515    C++
      jvm.dll!LazyClassPathEntry::resolve_entry(Thread * 
__the_thread__)  Line 304 + 0x19 bytes    C++
      jvm.dll!LazyClassPathEntry::open_stream(const char * name, Thread 
* __the_thread__)  Line 325 + 0xc bytes    C++
      jvm.dll!ClassLoader::load_classfile(Symbol * h_name, Thread * 
__the_thread__)  Line 909 + 0x1b bytes    C++
      jvm.dll!SystemDictionary::load_instance_class(Symbol * class_name, 
Handle class_loader, Thread * __the_thread__)  Line 1301 + 0x14 bytes    C++
      jvm.dll!SystemDictionary::resolve_instance_class_or_null(Symbol * 
name, Handle class_loader, Handle protection_domain, Thread * 
__the_thread__)  Line 779 + 0x18 bytes    C++
      jvm.dll!SystemDictionary::resolve_or_null(Symbol * class_name, 
Handle class_loader, Handle protection_domain, Thread * __the_thread__)  
Line 232 + 0x15 bytes    C++
      jvm.dll!SystemDictionary::resolve_or_null(Symbol * class_name, 
Thread * __the_thread__)  Line 237 + 0x23 bytes    C++
 >    jvm.dll!JVM_FindClassFromBootLoader(JNIEnv_ * env, const char * 
name)  Line 770 + 0x12 bytes    C++
      java.dll!70671e8c()

I've just noticed that there's a slight error in my change in line #325 
of classLoader.cpp:

ClassPathEntry* cpe = resolve_entry(CHECK_NULL);

should be

ClassPathEntry* cpe = resolve_entry(THREAD);

Otherwise, it'll skip the rest of the statements.
I'll update webrev shortly.

>
>>>
>>> And we still have this sequence:
>>>
>>> void ClassLoader::initialize() {
>>>   assert(_package_hash_table == NULL, "should have been initialized by
>>> now.");
>>>   EXCEPTION_MARK;
>>>   ...
>>>   setup_bootstrap_search_path();
>>>     -> update_class_path_entry_list(path, false);
>>>       ->  create_class_path_entry((char *)path, st, &new_entry,
>>> LazyBootClassLoader, CHECK);
>> As mentioned above, this call sequence is for instantiating the
>> LazyClassPathEntry instances.
>>>
>>> So if we return after the call to create_class_path_entry with an
>>> exception pending we will crash when we hit the EXCEPTION_MARK. Why
>>> doesn't this happen?
>> During vm initilization, it's ok to have exception pending. The
>> destructor of ExceptionMark is as follows:
>> ExceptionMark::~ExceptionMark() {
>>    if (_thread->has_pending_exception()) {
>>      Handle exception(_thread, _thread->pending_exception());
>>      _thread->clear_pending_exception(); // Needed to avoid infinite
>> recursion
>>      if (is_init_completed()) {
>>        exception->print();
>>        fatal("ExceptionMark destructor expects no pending exceptions");
>>      } else {
>>        vm_exit_during_initialization(exception);
>>      }
>>    }
>> }
>>
>> So if is_init_completed() is false, it'll just print an error and exit.
>
> So we do hit it - ok. So this yet another place where the VM should 
> not abort during initialization.
Yes and this is for preloading class scenario during vm init.
IMHO, I don't think we should change the behavior now.

thanks,
Calvin
>
> Thanks,
> David
>
>> thanks,
>> Calvin
>>
>>
>>>
>>> Thanks,
>>> David
>>>
>>>
>>>> Tests:
>>>>      jprt
>>>>          (in progress - only about 30 tests left on the windows
>>>> platforms, no failure so far;
>>>>           a previous run with only Coleen's suggestions was 
>>>> successful)
>>>>
>>>>      vm.quick.testlist on linux_x64
>>>>
>>>> Please review.
>>>>
>>>> thanks,
>>>> Calvin
>>