JVMTI events system and _jvmtiEnvironments uninitialised memory problem? [was Re: [PATCH PR42811,4.5 regression] java.lang.ExceptionInInitializerError in ecj1]

[Dave Korn <dave.korn.cygwin@googlemail.com>]

    Good evening all,

  I've been trying to understand why my patch to try and fix PR42811 caused
failures in the "events" test from jvmti.exp:

On 06/02/2010 06:29, Dave Korn wrote:
> On 05/02/2010 22:26, Dave Korn wrote:

>> 	PR java/42811
>> 	* configure.ac (libgcj_spec_lgcj_override): Add undefined reference
>> 	to arbitrary symbol in noncore library.
>> 	(libgcj_spec_lgcj_bc_override): Likewise.

  I'm using configure flags:  '--enable-libjava' '--enable-interpreter'
'--disable-libgcj-debug' '--with-ecj-jar=/usr/share/java/ecj.jar', DW2 EH,
native target i686-pc-cygwin, languages c/c++/java, graphite enabled but no
LTO.  Don't suppose those last two are relevant.

>   It caused a regression:
> 
>> FAIL: events output
> 
>   For some reason, a couple of extra jvm event callbacks appeared:
> 
>> $ diff -pu events.out .e.o
>> --- events.out  2009-03-14 06:47:02.984375000 +0000
>> +++ .e.o        2010-02-06 06:04:50.218750000 +0000
>> @@ -4,9 +4,9 @@ created JVMTI environment #0
>>  created JVMTI environment #1
>>  created JVMTI environment #2
>>  setting callbacks for envs
>> -RequestedEvents:
>> +RequestedEvents: VMInit,VMDeath,
>>  enable VM_INIT for env0, env1, env2
>> -RequestedEvents: VMInit,
>> +RequestedEvents: VMInit,VMDeath,
>>  enable VM_DEATH for env1,env2
>>  RequestedEvents: VMInit,VMDeath,
>>  enable THREAD_END for env2

  Well, I've been tracking it down under the debugger (and it doesn't show up
if you launch the program directly under the debugger, you have to launch it
naturally and intercept it at an early breakpoint, which smells like a clue)
and it looks like it's caused by use of uninitialised memory.  Valgrind
doesn't run on windows, which is the only platform where the sublib-enabled
build currently works, so I don't have mechanical verification, but here's
what appears to be the sequence of events:

  The testcase creates three VM environments, and tests enabling and disabling
their VM event callbacks:

> static void
> do_enable_tests ()
> {
>   printf ("- enable tests -\n");
>   JavaVM *vm = _Jv_GetJavaVM ();
>   jvmtiEnv *env[3];
>   int i;
>   for (i = 0; i < 3; ++i)
>     {
>       vm->GetEnv (reinterpret_cast<void **> (&env[i]), JVMTI_VERSION_1_0);
>       printf ("created JVMTI environment #%d\n", i);
>     }
> 
>   jvmtiEventCallbacks callbacks;
>   memset (&callbacks, 0, sizeof (jvmtiEventCallbacks));
> 
>   printf ("setting callbacks for envs\n");
>   callbacks.VMInit = VMInitCB;
>   env[0]->SetEventCallbacks (&callbacks, sizeof (callbacks));
>   callbacks.VMDeath = VMDeathCB;
>   env[1]->SetEventCallbacks (&callbacks, sizeof (callbacks));
>   callbacks.ThreadEnd = ThreadEndCB;
>   env[2]->SetEventCallbacks (&callbacks, sizeof (callbacks));
>   print_events ();

  At this point in the test, none of the callbacks should actually be enabled;
even though some of the event callbacks have been set, the test hasn't called
SetEventNotificationMode to enable them yet; but as the failure diff above
shows, print_events(), which is where the "RequestedEvents:" trace comes from,
thinks that some of the callbacks are indeed enabled.  For reference, the
contents of the environment struct look like this, from include/jvmti_md.h:

> /* Contents of the jvmtiEnv; but only inside the implementation. */
> #define _CLASSPATH_JVMTIENV_CONTENTS					\
>   /* Event handlers registered via SetEventCallbacks */			\
>   jvmtiEventCallbacks callbacks;					\
> 									\
>   /* Array of event thread for which to report event. */		\
>   /* NULL means all threads. One for each callback.   */		\
>   jthread thread[EVENT_SLOTS];						\
>   									\
>   /* Array of notification modes for callbacks. */			\
>   /* One for each callback.                     */			\
>   bool enabled[EVENT_SLOTS];

  The vm->GetEnv() call that allocates the VMs passes through _Jv_JNI_GetEnv()
and ends up in _Jv_GetJVMTIEnv() in libjava/jvmti.cc:

> _Jv_JVMTIEnv *
> _Jv_GetJVMTIEnv (void)
> {
>   _Jv_JVMTIEnv *env
>     = (_Jv_JVMTIEnv *) _Jv_MallocUnchecked (sizeof (_Jv_JVMTIEnv));
>   env->p = &_Jv_JVMTI_Interface;
>   struct jvmti_env_list *element
>     = (struct jvmti_env_list *) _Jv_MallocUnchecked (sizeof (struct jvmti_env_list));
>   element->env = env;
>   element->next = NULL;
> 
>   _envListLock->writeLock ()->lock ();
>   if (_jvmtiEnvironments == NULL)
>     _jvmtiEnvironments = element;
>   else
>     {
>       struct jvmti_env_list *e;
>       for (e = _jvmtiEnvironments; e->next != NULL; e = e->next)
> 	;
>       e->next = element;
>     }
>   _envListLock->writeLock ()->unlock ();

  As far as I can see, that just mallocs a bunch of heap space, then
immediately shoves it live onto the global list of environments - still full
of uninitialised heap crud!  Sure enough, that's also how it's returned to the
calling code from do_enable_tests(), and although the calls to
SetEventCallbacks() fully initialise the callbacks member, the thread[] and
enabled[] arrays remain full of rubbish by the time we get to this loop in
libjava/jvmti.cc#check_enabled_event():

>   if (_jvmtiEnvironments != NULL)
>     {
>       _envListLock->readLock ()->lock ();
>       struct jvmti_env_list *e;
>       FOREACH_ENVIRONMENT (e)
> 	{
> 	  char *addr
> 	    = reinterpret_cast<char *> (&e->env->callbacks) + offset;
> 	  void **callback = reinterpret_cast<void **> (addr);
> 	  if (e->env->enabled[index] && *callback != NULL)
> 	    {
> 	      *enabled = true;
> 	      _envListLock->readLock ()->unlock ();
> 	      return;
> 	    }
> 	}
> 
>       _envListLock->readLock ()->unlock ();
>     }

and as a result it believes various events are enabled even though we haven't
yet called SetEventNotificationMode() to turn them on.

  I set a breakpoint in _Jv_GetJVMTIEnv(), just after the call to
_Jv_MallocUnchecked(), and manually called memset to zero out the
freshly-returned space, and it fixed the bug, so I'm just bootstrapping and
testing a patch to add an explicit memset call there (will post separately),
but I just wondered if there is supposed to be an assumption that the
_Jv_Malloc* routines return zeroed memory, or if possibly there should be a
_Jv_Calloc interface added?  Or have I somehow missed some place that should
have zeroed out the thread[] and enabled[] arrays?

  BTW, assuming this proposed patch passes testing and is agreed to be the
right thing to do, I'll be resurrecting my proposed fix for PR42811(*).  Heads
up, and maybe a little pre-emptive "ping!" in advance :)

    cheers,
      DaveK
-- 
(*) - http://gcc.gnu.org/ml/gcc-patches/2010-02/msg00261.html