53593 - uninitialized variables in jsj.c

Status: VERIFIED FIXED

(Core Graveyard :: Java: Live Connect, defect, P3)

Description

[Kathleen :Brade]

In the warnings logs on Linux tinderbox builds, I see these (potentially?) serious bugs:

3.     js/src/liveconnect/jsj.c:681 (See build log excerpt)
     `jEnv' might be used uninitialized in this function
     679 JSJ_AttachCurrentThreadToJava(JSJavaVM *jsjava_vm, const char *name, JNIEnv **java_envp)
     680 {
     681     JNIEnv *jEnv;
     682     SystemJavaVM *java_vm;
     683     JSJavaThreadState *jsj_env;

4.     js/src/liveconnect/jsj.c:730 (See build log excerpt)
     `java_vm' might be used uninitialized in this function
     728 {
     729     JSJavaThreadState *jsj_env;
     730     SystemJavaVM *java_vm;
     731     JSJavaVM *jsjava_vm;

Comment 1

[Doug Turner (:dougt)]

to default

Comment 2

[Phil Schwartau]

Reassigning to Patrick -

Comment 3

[Aleksey Nogin]

Currently TBox shows 3 warnings in Live Connect:

js/src/liveconnect/jsj.c:698
 `jEnv' might be used uninitialized in this function

js/src/liveconnect/jsj.c:753
 `java_vm' might be used uninitialized in this function

js/src/liveconnect/jsj_JSObject.c:253
 `java_wrapper_obj' might be used uninitialized in this function

P.S. trying to make 1.0 as warnings-free as possible...

Comment 4

[Aleksey Nogin]

Attachment: Fix two warnings (that also appear to be bugs!)

This patch fixes two warnings:

jsj.c: In function `JSJ_AttachCurrentThreadToJava':
jsj.c:680: warning: `jEnv' might be used uninitialized in this function
jsj.c: In function `jsj_MapJavaThreadToJSJavaThreadState':
jsj.c:735: warning: `java_vm' might be used uninitialized in this function

In both cases the code structure is about the same:

... * var;
...
if (condition)
   var = ...;
if (var == NULL)
   rerutn NULL;

Of course, if "condition" happens to be false, we'll end up comparing an
uninitialized portion of memory to NULL with un[predictable results! The patch
preinitializes variables to NULL and gets rid of uncertanty.

Comment 5

[Kathleen :Brade]

Comment on attachment 102553 [details] [diff] [review]
Fix two warnings (that also appear to be bugs!)

r=brade

Comment 6

[Aleksey Nogin]

Can I get an sr for the patch? (And can somebody check it in?) Thanks!

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 102553 [details] [diff] [review]
Fix two warnings (that also appear to be bugs!)

Yow.  How'd that happen?

sr=[e-mail], please mail drivers for 1.2 branch permission.	Does
this need to be fixed on the 1.0 branch also?

/be

Comment 8

[Aleksey Nogin]

> please mail drivers for 1.2 branch permission

Can somebody with CVS permissions take care of this?

> Does this need to be fixed on the 1.0 branch also?

The warnings do exist on 1.0 branch. As far as "need to be fixed" goes, I do not
know if they ever cause anything bad to happen...

Comment 9

[Patrick C. Beard]

This problem is largely academic, since the only way jEnv ends up being unitialized is if (JSJ_callbacks && 
JSJ_callbacks->attach_current_thread) is FALSE. A clearer change might instead be:

Index: mozilla/js/src/liveconnect/jsj.c
===================================================================
RCS file: /cvsroot/mozilla/js/src/liveconnect/jsj.c,v
retrieving revision 1.27
diff -b -u -2 -r1.27 jsj.c
--- mozilla/js/src/liveconnect/jsj.c	1 Aug 2002 11:04:18 -0000	1.27
+++ mozilla/js/src/liveconnect/jsj.c	7 Nov 2002 23:54:26 -0000
@@ -688,6 +692,6 @@
     /* Try to attach a Java thread to the current native thread */
     java_vm = jsjava_vm->java_vm;
-    if (JSJ_callbacks && JSJ_callbacks->attach_current_thread)
-        jEnv = JSJ_callbacks->attach_current_thread(java_vm);
+    jEnv = (JSJ_callbacks && JSJ_callbacks->attach_current_thread) ?
+           JSJ_callbacks->attach_current_thread(java_vm) : NULL;
     if (jEnv == NULL) 
         return NULL;
@@ -745,6 +749,6 @@
 
     /* First, figure out which Java VM is calling us */
-    if (JSJ_callbacks && JSJ_callbacks->get_java_vm)
-        java_vm = JSJ_callbacks->get_java_vm(jEnv);
+    java_vm = (JSJ_callbacks && JSJ_callbacks->get_java_vm) ?
+              JSJ_callbacks->get_java_vm(jEnv) : NULL;
     if (java_vm == NULL)
         return NULL;

Comment 10

[Patrick C. Beard]

I'd be happy to get this patch checked in.

Comment 11

[Aleksey Nogin]

Attachment: Fix two warnings (that also appear to be bugs), v2

Created a cleaner version of the fix, similar to the one requested in comment
#9.

Comment 12

[Aleksey Nogin]

Comment on attachment 111677 [details] [diff] [review]
Fix two warnings (that also appear to be bugs), v2

Can you check this in? Thanks!

Comment 13

[Patrick C. Beard]

Comment on attachment 111677 [details] [diff] [review]
Fix two warnings (that also appear to be bugs), v2

r=beard

Comment 14

[Patrick C. Beard]

Checked into trunk.

Comment 15

[Aleksey Nogin]

Per comment #14.

Comment 16

[Aleksey Nogin]

TBox shows the warnings are gone.