Closed Bug 53593 Opened 19 years ago Closed 17 years ago

uninitialized variables in jsj.c

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: Brade, Assigned: beard)

References

Details

Attachments

(1 file, 1 obsolete file)

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;
Blocks: 59652
to default
Assignee: warren → rogerl
Reassigning to Patrick -
Assignee: rogerl → beard
Status: NEW → ASSIGNED
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...
Blocks: 59659
Keywords: mozilla1.0
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.
Keywords: patch, review
Comment on attachment 102553 [details] [diff] [review]
Fix two warnings (that also appear to be bugs!)

r=brade
Attachment #102553 - Flags: review+
Can I get an sr for the patch? (And can somebody check it in?) Thanks!
Whiteboard: sr needed
Comment on attachment 102553 [details] [diff] [review]
Fix two warnings (that also appear to be bugs!)

Yow.  How'd that happen?

sr=brendan@mozilla.org, please mail drivers for 1.2 branch permission.	Does
this need to be fixed on the 1.0 branch also?

/be
Attachment #102553 - Flags: superreview+
> 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...
Whiteboard: sr needed
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;
I'd be happy to get this patch checked in.
Attachment #102553 - Flags: in?(beard)
Attachment #102553 - Flags: checked
Created a cleaner version of the fix, similar to the one requested in comment
#9.
Attachment #102553 - Attachment is obsolete: true
Comment on attachment 111677 [details] [diff] [review]
Fix two warnings (that also appear to be bugs), v2

Can you check this in? Thanks!
Attachment #111677 - Flags: review?(beard)
Comment on attachment 111677 [details] [diff] [review]
Fix two warnings (that also appear to be bugs), v2

r=beard
Attachment #111677 - Flags: review?(beard) → review+
Checked into trunk.
Per comment #14.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
TBox shows the warnings are gone.
Status: RESOLVED → VERIFIED
Attachment #102553 - Flags: checkin?(beard)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.