Last Comment Bug 168316 - Violating same-origin with Java
: Violating same-origin with Java
Status: VERIFIED FIXED
[sg:blocker]
:
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Mitchell Stoltz (not reading bugmail)
: bsharma
Mentors:
Depends on:
Blocks: blackbird 1.2
  Show dependency treegraph
 
Reported: 2002-09-12 13:33 PDT by Mitchell Stoltz (not reading bugmail)
Modified: 2002-12-23 11:06 PST (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Demo part 1 - the html file (704 bytes, text/html)
2002-09-12 13:34 PDT, Mitchell Stoltz (not reading bugmail)
no flags Details
Demo part 2a - Source of the applet (1.11 KB, text/plain)
2002-09-12 13:35 PDT, Mitchell Stoltz (not reading bugmail)
no flags Details
Demo part 2b - compiled applet (1.17 KB, application/octet-stream)
2002-09-12 13:35 PDT, Mitchell Stoltz (not reading bugmail)
no flags Details
Patch - work in progress (10.84 KB, patch)
2002-10-15 20:33 PDT, Mitchell Stoltz (not reading bugmail)
jst: superreview+
Details | Diff | Splinter Review
Patch v2 - above issues addressed. (10.72 KB, patch)
2002-10-24 17:16 PDT, Mitchell Stoltz (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v3 (12.05 KB, patch)
2002-10-24 19:07 PDT, Mitchell Stoltz (not reading bugmail)
security-bugs: superreview+
Details | Diff | Splinter Review
patch v4 (12.45 KB, patch)
2002-10-25 16:22 PDT, Mitchell Stoltz (not reading bugmail)
no flags Details | Diff | Splinter Review
patch v5 (12.56 KB, patch)
2002-10-28 18:29 PST, Mitchell Stoltz (not reading bugmail)
jst: review+
dveditz: superreview+
Details | Diff | Splinter Review
patch v6 (12.47 KB, patch)
2002-10-29 17:56 PST, Mitchell Stoltz (not reading bugmail)
security-bugs: review+
security-bugs: superreview+
chofmann: approval+
Details | Diff | Splinter Review

Description Mitchell Stoltz (not reading bugmail) 2002-09-12 13:33:20 PDT
There is a serious bug with Java/Javascript which affects at least Netscape 7
beta and the latest AOL build I have.
The problem is Java can access properties from other windows.
The collected data may be sent either from Java or from Javascript with
Liveconnect.
Comment 1 Mitchell Stoltz (not reading bugmail) 2002-09-12 13:34:20 PDT
Created attachment 98925 [details]
Demo part 1 - the html file
Comment 2 Mitchell Stoltz (not reading bugmail) 2002-09-12 13:35:16 PDT
Created attachment 98926 [details]
Demo part 2a - Source of the applet
Comment 3 Mitchell Stoltz (not reading bugmail) 2002-09-12 13:35:48 PDT
Created attachment 98927 [details]
Demo part 2b - compiled applet
Comment 4 georgi - hopefully not receiving bugspam 2002-09-13 01:22:45 PDT
On further testing, latest mozilla trunk with the netscape 7beta java plugin is
affected by this bug.
Comment 5 Mitchell Stoltz (not reading bugmail) 2002-10-04 13:28:27 PDT
From Georgi:
With a combination of bugs 59767 and 168316 it is possible to read local
files. Check attached files. Inline comments show what to change to read another
local file.
Comment 6 Joe Chou 2002-10-14 08:36:17 PDT
Ccing some OJI folks to look at this serious bug.
Comment 7 Joe Chou 2002-10-14 18:43:59 PDT
Seemed WFM with mozilla 1.1beta (released on 07/21) and JRE 1.4.1 on Windows2000
and Solaris: no source (of mozilla.org page) was displayed in the alert box,
instead, only a small alert box was popped up. In the java console, it showed
that a "null" was retrived, instead of the source page. But no js security
exception shown in the javascript console though.

On the other hand, the problem was reproduced with the later builds: mozilla 1.1
(released on 08/26), netscape7.0, all with the same JRE build, regardless on
Windows or Solaris.

Comment 8 Joe Chou 2002-10-14 18:55:54 PDT
The problem was also reproced with mozill 1.1alpha (released on 06/11).

This is odd, so far it seems only mozilla 1.1 beta (released on 07/21) working
OK (no showing the page source of mozilla.org). What makes it different?
Comment 9 georgi - hopefully not receiving bugspam 2002-10-15 02:03:37 PDT
The problem is also present ot latest trunk at least on linux.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2002-10-15 09:38:42 PDT
This is super serious ... it should block 1.2.
Comment 11 georgi - hopefully not receiving bugspam 2002-10-15 10:19:20 PDT
Agreed, this is serious.
Comment 12 Mitchell Stoltz (not reading bugmail) 2002-10-15 20:33:37 PDT
Created attachment 103019 [details] [diff] [review]
Patch - work in progress

it compiles - that's all I can say right now. We still need to make sure
security exceptions get handled right. Needs lots of testing too.
Comment 13 Joe Chou 2002-10-22 18:34:58 PDT
I applied the patch to my trunk build, and it seemed working OK, and the source
page did not show, instead, a "java.lang.NullPointerException" was displayed in
the java console.
Comment 14 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-10-23 16:02:21 PDT
Comment on attachment 103019 [details] [diff] [review]
Patch - work in progress

This is mostly style review for now, I would need someone to explain what some
of the things really do before I can give review.

A general comment first: there are some tab chars here which mess up the
indentation, please get rid of them.

>Index: caps/idl/nsIScriptSecurityManager.idl
>===================================================================
>Index: caps/src/nsScriptSecurityManager.cpp
>===================================================================
>+    nsCOMPtr<nsIScriptContext> scriptContext =
>+        NS_REINTERPRET_CAST(nsIScriptContext*, JS_GetContextPrivate(cx));

Does this require reinterpret cast or would static cast do?

>+        scriptContext->GetGlobalObject(getter_AddRefs(global));
>+        if (global)
>+        {
>+            nsCOMPtr<nsIScriptObjectPrincipal> globalData(do_QueryInterface(global));
>+            if (globalData)

You can avoid the |if (global)| statement because QI is null safe.

>+                nsCOMPtr<nsIPrincipal> principal;

This line is unneeded.

>Index: js/src/liveconnect/Makefile.in
>===================================================================
>+		  caps \
>+		  necko \
>+		  string \
>+		  pref \

I don't really like adding this many new deps, but I must say I am surprised
liveconnect was not already depending on CAPS. It seems like we could easily
remove some of these direct deps.

>Index: js/src/liveconnect/nsCLiveconnect.cpp
>===================================================================
>+#include "nsScriptSecurityManager.h"

Why is this needed?

>+    char origin[4096];
>+    rv = securityContext->GetOrigin(origin, sizeof(origin));
>+    if (NS_FAILED(rv)) return rv;

Does this method fail if there is not enough space?

>+		memset(&mFrame, 0, sizeof mFrame);

Missing parenthesis?

>+        goto done;

When using goto, be careful with potentially uninitialized variables, we have a
few in the tree and want to get rid of them. Compiling on Linux will show them,
but you can check that manually as well (look for variables that are defined
after that goto, or assigned values after the goto).
Comment 15 Joe Chou 2002-10-23 17:42:36 PDT
Did some more testing, using some of Java plugin folks' test cases, and the
patch still seemed working OK.
So as far as fixing the bug, the patch seems doing its job.
Comment 16 Mitchell Stoltz (not reading bugmail) 2002-10-24 17:16:28 PDT
Created attachment 104065 [details] [diff] [review]
Patch v2 - above issues addressed.

> You can avoid the |if (global)| statement because QI is null safe.
I'm not sure what you mean by this. It's possible for the QI to return null,
and if so we don't want to pass the result on.

The additional dependencies are unfortunate but apparently unavoidable. I
managed to remove the "pref" dependency, but there seems to be no way to create
an NSIURI without the "string" dependency. This could be solved by putting this
whole function in NSScriptSecurityManager, but I don't want to do that, since
that interface is getting cluttered with special functions that are only called
in one place.

> Does this method fail if there is not enough space?
Yes.

> Missing parenthesis?
sizeof doesn't actually require parenthesis. beard wrote it without the parens,
but I think that looks weird, so I put them back in.
Comment 17 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-10-24 17:40:57 PDT
I meant that instead of:

+        scriptContext->GetGlobalObject(getter_AddRefs(global));
+        if (global)
+        {
+            nsCOMPtr<nsIScriptObjectPrincipal>
globalData(do_QueryInterface(global));
+            if (globalData)
+                globalData->GetPrincipal(result);
+        }

you can do:

+        scriptContext->GetGlobalObject(getter_AddRefs(global));
+        nsCOMPtr<nsIScriptObjectPrincipal> globalData(do_QueryInterface(global));
+        if (globalData)
+            globalData->GetPrincipal(result);

because if global==0 then QI of that will still give 0, and you can just test
for the QId value.
Comment 18 Johnny Stenback (:jst, jst@mozilla.com) 2002-10-24 18:19:11 PDT
Comment on attachment 103019 [details] [diff] [review]
Patch - work in progress

- In nsScriptSecurityManager::GetPrincipalFromContext():

+    nsCOMPtr<nsIScriptContext> scriptContext =
+	 NS_REINTERPRET_CAST(nsIScriptContext*, JS_GetContextPrivate(cx));

This code, as well as all other code that gets an nsIScriptContext out of a
JSContext, should check that ::JS_GetOptions(aContext) &
JSOPTION_PRIVATE_IS_NSISUPPORTS is true before assuming that the context
private data is an nsISupports. And to be correct, the above code should cast
to nsISupports, and then QI to nsIScriptContext.

+    if (scriptContext)
+    {
+	 nsCOMPtr<nsIScriptGlobalObject> global;
+	 scriptContext->GetGlobalObject(getter_AddRefs(global));
+	 if (global)
+	 {
+	     nsCOMPtr<nsIScriptObjectPrincipal>
globalData(do_QueryInterface(global));
+	     if (globalData)
+	     {
+		 nsCOMPtr<nsIPrincipal> principal;
+		 globalData->GetPrincipal(result);
+	     }
+	 }
+    }
+    return NS_OK;

Alternatively, you could call nsIXPConnect's GetWrappedNativeOfJSObject() on
::JS_GetGlobalObject(cx) and get the native out of that wrapper, and then QI it
to nsIScriptObjectPrincipal, that way you'd avoid having to check that the
context's private really is an nsIScriptContext.

- In nsCLiveconnect.cpp:

-    AutoPushJSContext(JSContext *cx)
+    AutoPushJSContext(nsISupports* aSecuritySupports, JSContext *cx, nsresult*
rv)
+		:	mContext(cx)

Remove the tabs, fix the indentation...

And reading on I think that, given that you're adding a fair amount of new code
to the AutoPushJSContext class, you should un-inline the constructor, i.e.
break the code out of the class declaration to avoid duplicating the code all
over when compiled.

@@ -99,6 +133,53 @@
		     // Leave the reference to the mContextStack to
		     // indicate that we need to pop it in our dtor.	        
		 }
+
+	     }
+	 }
+
+		memset(&mFrame, 0, sizeof mFrame);

Loose the tabs, and put parens around the mFrame 'argument' to sizeof, for
consistency, if nothing else...

+	 PRBool hasScript = PR_FALSE;
+	 JSStackFrame* tempFP = cx->fp;
+	 while (tempFP && !hasScript)
+	 {
+	     if (tempFP->script)
+		 hasScript = PR_TRUE;
+	     tempFP = tempFP->down;
+	 };

Checking !hasScript in every iteration of that loop is not necessary, I'd
suggest this in stead:

+	 PRBool hasScript = PR_FALSE;
+	 JSStackFrame* tempFP = cx->fp;
+	 while (tempFP)
+	 {
+	     if (tempFP->script) {
+		 hasScript = PR_TRUE;
+
+		 break;
+	     }
+
+	     tempFP = tempFP->down;
+	 };

...

+	     if(aSecuritySupports)

Add a space after 'if'.

...
+			JSPrincipals* jsprinc;
+			principal->GetJSPrincipals(&jsprinc);
+		
+			mFrame.script = JS_CompileScriptForPrincipals(cx,
JS_GetGlobalObject(cx),
+									       
				  jsprinc, "", 0, "", 1);

More tabs...

+			JSPRINCIPALS_DROP(cx, jsprinc);
+
+			if (mFrame.script)
+		 {
+				mFrame.down = cx->fp;
+				cx->fp = &mFrame;
+			}

And again more tabs...

@@ -110,10 +191,16 @@
	     mContextStack->Pop(nsnull);
	     mContextStack = nsnull;
	 }
+
+		if (mFrame.script) {
+			mContext->fp = mFrame.down;
+		}
     }

Tabs... There's a ton of these, please search for all of them (at least the
ones you added) and get rid of them.

...
-    AutoPushJSContext autopush(cx);
+    nsresult rv;
+    AutoPushJSContext autopush(securitySupports, cx, &rv);
+    if (NS_FAILED(rv)) {
+	 JS_ReportError(cx, "failed to get a principal");
+	 goto done;
+    }

Given that the above code appears in a lot of places, you may want to store rv
in AutoPushJSContext and put a little DidSucceed() inline method in
AutoPushJSContext() and hide the JS_ReportError() call in there too.

Other than that, this looks fine with me. sr=jst, but if there's time, I'd like
to see the new patch (post-checkin, if nothing else).
Comment 19 Mitchell Stoltz (not reading bugmail) 2002-10-24 19:07:34 PDT
Created attachment 104086 [details] [diff] [review]
Patch v3
Comment 20 Mitchell Stoltz (not reading bugmail) 2002-10-24 19:08:11 PDT
Comment on attachment 104086 [details] [diff] [review]
Patch v3

carrying forward jst's sr.
Comment 21 Brendan Eich [:brendan] 2002-10-24 19:11:51 PDT
Feel free to cc: me on any such bug.

/be
Comment 22 Brendan Eich [:brendan] 2002-10-24 19:57:19 PDT
Comment on attachment 104086 [details] [diff] [review]
Patch v3

>+#include "jsinterp.h"  // XXX private API so we can auto-push a JSStackFrame
>+#include "jsscript.h"  // XXX private declaration of JSScript

You don't need jsscript.h, but you should include jscntxt.h explicitly, as you
dereference JSContext pointers (cx->fp, e.g.).

>+    char origin[4096];

That seems overlarge.  Domain names are restricted to some smaller length, 256
if memory serves (maybe the limit has been raised since the old days when I
hacked on 4.3bsd Unix kernel code).  URI schemes can't be that long, either. 
I'm concerned that this will blow the stack on stack-challenged platforms. 
Anyway, having a magic size like 4096 seems wrong.

>@@ -81,40 +114,107 @@
> class AutoPushJSContext
> {
> public:
>-    AutoPushJSContext(JSContext *cx)
>-    {
>-        mContextStack = do_GetService("@mozilla.org/js/xpc/ContextStack;1");
>+    AutoPushJSContext(nsISupports* aSecuritySupports, JSContext *cx);
>+
>+    ~AutoPushJSContext();
>+
>+    nsresult ResultOfPush() { return mPushResult; };
> 
>-        if(mContextStack)
>+private:
>+    nsCOMPtr<nsIJSContextStack> mContextStack;
>+    JSContext*                  mContext;
>+    JSStackFrame                mFrame;
>+    nsresult                    mPushResult;
>+};
>+
>+AutoPushJSContext::AutoPushJSContext(nsISupports* aSecuritySupports,
>+                                     JSContext *cx) 
>+                                     : mContext(cx), mPushResult(NS_OK)
>+{
>+    mContextStack = do_GetService("@mozilla.org/js/xpc/ContextStack;1");
>+
>+    if(mContextStack)
>+    {
>+        JSContext* currentCX;
>+        if(NS_SUCCEEDED(mContextStack->Peek(&currentCX)))
>         {
>-            JSContext* currentCX;
>-            if(NS_SUCCEEDED(mContextStack->Peek(&currentCX)))
>+            // Is the current context already on the stack?
>+            if(cx == currentCX)
>+                mContextStack = nsnull;
>+            else
>             {
>-                // Is the current context already on the stack?
>-                if(cx == currentCX)
>-                    mContextStack = nsnull;
>-                else
>-                {
>-                    mContextStack->Push(cx);
>-                    // Leave the reference to the mContextStack to
>-                    // indicate that we need to pop it in our dtor.                                               
>-                }
>+                mContextStack->Push(cx);
>+                // Leave the reference to the mContextStack to
>+                // indicate that we need to pop it in our dtor.                                               
>             }
>+
>         }
>     }
> 
>-    ~AutoPushJSContext()
>+    memset(&mFrame, 0, sizeof(mFrame));

I wrote the original version of this code, so I'm to blame for the lack of
parens around mFrame in 'sizeof mFrame' -- but c'mon, reviewers -- sizeof is a
unary operator, not a function.  Anyway, blame me, and use caps prevailing
style, or do both -- I don't care, but I wanted to say my peace.

>+            mFrame.script = JS_CompileScriptForPrincipals(cx, JS_GetGlobalObject(cx),
>+                                                          jsprinc, "", 0, "", 1);
>+            JSPRINCIPALS_DROP(cx, jsprinc);
>+
>+            if (mFrame.script)
>+            {
>+                mFrame.down = cx->fp;
>+                cx->fp = &mFrame;
>+            }
>+            else
>+                mPushResult = NS_ERROR_FAILURE;

NS_ERROR_OUT_OF_MEMORY is more precise, but...

>         }
>     }
>+    
>+    if (NS_FAILED(mPushResult))
>+        JS_ReportError(cx, "failed to get a principal");

If you failed because JS_CompileScriptForPrincipals failed, you don't want this
JS_ReportError call -- an error was already reported by JS_CompileScript.... 
Keep track of that case and avoid a double-report.

Also, this if (NS_FAILED(mPushResult)) ...; can go in the then-part of the if
above it, because there's no way mPushResult could be other than its initial
value, NS_OK, if we do have a script (if (hasScript)).	Moving it up may help
avoid the double report, too.

Fix those little issues in a new patch, and r=brendan@mozilla.org.

/be
Comment 23 Michael Buckland 2002-10-25 13:33:51 PDT
adt approval once drivers approve
Comment 24 Mitchell Stoltz (not reading bugmail) 2002-10-25 16:22:35 PDT
Created attachment 104208 [details] [diff] [review]
patch v4

This should do it...
Comment 25 Joe Chou 2002-10-25 17:26:17 PDT
When applying the second file of the patch, got the following error when compiling:
"...
nsScriptSecurityManager.cpp", line 1783: Error:
nsScriptSecurityManager::GetPrincipalFromContext(JSContext*, nsIPrincipal**)
already had a body defined.
..."

After removed the other (older?) GetPrincipalFromContext (), the compiling went OK. 
Is the patch missing the part that removing the old GetPrincipalFromContext()?
Comment 26 Joe Chou 2002-10-25 17:42:03 PDT
Never mind, it looks like I was applying the new patch to the file with the
previous patch. The patch file is OK.
In the process of re-testing the new patch. I will let you know about the results.
Comment 27 Joe Chou 2002-10-25 18:39:59 PDT
The new patch seems to have some problem. When I ran a javascript-to-java test
after applying the patch, the browser crashed (or exited) with the following
error:
"Assertion failure: !(*jEnv)->ExceptionOccurred(jEnv), at jsj_utils.c:69".

If I swapped back the previous patch (the patches in
js/src/liveconnect/Makefile.in and js/src/liveconnect/nsCLiveconnect.cpp), then
the test went OK.

Please let me know if wanting me to post my js-to-java test case.
Comment 28 Mitchell Stoltz (not reading bugmail) 2002-10-28 14:33:01 PST
Joe,
   Could you send me the testcase that's crashing, or post a link to it here?
Comment 29 Joe Chou 2002-10-28 15:09:00 PST
Here is the javascript-to-java test case I used:

Html file:

<html>
  <head>
    <title>JS to Java Test</title>
  </head>

  <body>
    <center>
      <h1>LiveConnect JS to Java Test</h1>
        <APPLET NAME="MyApplet" CODE = HelloWorld.class WIDTH= 100
                HEIGHT = 100 ALIGN = testtop HSPACE = 50 VSPACE = 100>
        </APPLET>
    </center>

    <INPUT TYPE="Button" VALUE="Print" NAME="Print Button"
        onClick="document.MyApplet.print()">
  </body>
</html>

public class HelloWorld extends Applet {

        public void init()
        {
                System.err.println("calling HelloWorld.init().");
                System.out.println("calling getClass().");
                Class c = getClass();
                System.out.println("got class: " + c);
                System.out.println("calling getClassLoader().");
                ClassLoader cl = c.getClassLoader();
                System.err.println("got class loader: "+cl);
                System.err.println("HelloWorld.init() called.");
        }

        public void print()
        {
                System.err.println("HelloWorld.print() called.");

        }

        public void paint(Graphics g) 
        {
                System.err.println("HelloWorld.paint() called.");
                g.drawString("Hello World!", 25, 50);
        }

}
Java code (HolloWorld.java):




Comment 30 Mitchell Stoltz (not reading bugmail) 2002-10-28 15:50:25 PST
Joe,
   The testcase you just sent works fine for me, without crashing, on Win XP
with patch v4 above. Are you only seeing the crash on Solaris? Can you post a
stack trace of the crash?
Comment 31 Joe Chou 2002-10-28 15:52:36 PST
Yes, I tried it on Solaris. I'll try it on Linux, and see what happens.
Comment 32 Joe Chou 2002-10-28 17:57:20 PST
Did some debuging, and found out the problem (assertion failure) some times
occured in the js-to-java case (the first test case already posted), bug always
occured in the java-to-js case when getMember() is called (see the test case
attached below, press "click" button to reproduce the problem). After printing a
message to the console, "Assertion failure: !(*jEnv)->ExceptionOccurred(jEnv),
at jsj_utils.c:69", aborted in JS_Assert() (see below).

The assertion occured to debug builds on all platforms, but only aborted in Unix
(see JS_Assert() code belowy). On the other hand, when using the original patch
(v3), there is no assertion at all. 

Mitch,
This may not be a big problem, but please verify that the exception (which
caused the assertion) is not critical. I tried to find out what the exception
was complaining about, but since I was not familiar with the securityenv code
(where the exception seemed to occur) and could not pin point it.


***** JS_Assert() code:***********

JS_PUBLIC_API(void) JS_Assert(const char *s, const char *file, JSI
ntn ln)
{
#ifdef XP_MAC
    dprintf("Assertion failure: %s, at %s:%d\n", s, file, ln);
#else
    fprintf(stderr, "Assertion failure: %s, at %s:%d\n", s, file,
ln);
#endif
#if defined(WIN32) || defined(XP_OS2)
    DebugBreak();
#endif
#ifndef XP_MAC
    abort();
#endif
}


************ Java-to-js Test case ********************
Html file:

<html>
  <head>
    <title>LiveConnect Test 1</title>
        <SCRIPT LANGUAGE="JavaScript">
        function showAlert(txt)
        {
                alert("JS showAlert: being called " + txt);
        }
        function click_doit()
        {
                document.MyApplet.getMemberTest();
        }
        </SCRIPT>
  </head>

  <body>
    <center>
      <h1>Liveconnect Test: java to javascript</h1>
        <APPLET NAME=MyApplet CODE = java2js.class WIDTH= 100
                HEIGHT = 100 ALIGN = testtop HSPACE = 50 VSPACE = 100 MAYSCRIPT>
        </APPLET>
    </center>

    <form name=myForm>
        Test with getMember called in init:
        <input type=button name=myButton value="Click"
          onClick=click_doit()>
    </form>
  </body>
</html>


Java code (java2js.java):
import java.applet.*;
import java.awt.*;
import netscape.javascript.*;

public class java2js extends Applet {
        JSObject win, doc, app;
        Object app0;

        public void init()
        {
                System.err.println("calling getWindow().");
                win = JSObject.getWindow(this);
                System.err.println("calling getWindow().");
                if (win == null) {
                        System.err.println("getWindow() returned null.");
                        return;
                }
                System.err.println("getWindow() returned non null.");
        }

        public void getMemberTest()
        {

                System.err.println("getMemberTest called.");

                System.err.println("calling getMember() doc.");
                doc = (JSObject) win.getMember("document");
                if (doc == null) {
                        System.err.println("getMember() doc returned null.");
                        return;
                }
                System.err.println("getMember() doc returned non null.");
        }

        public void paint(Graphics g)
        {
                System.err.println("Hi from Java.");
                g.drawString("Hello World 1", 25, 50);
        }

}
Comment 33 Mitchell Stoltz (not reading bugmail) 2002-10-28 18:29:50 PST
Created attachment 104437 [details] [diff] [review]
patch v5

Joe,
   I think I found the problem - I was passing a bad size param to
securityContext->GetOrigin. Please try this patch instead, and let me know if
it still asserts - it works for me.
Comment 34 Daniel Veditz [:dveditz] 2002-10-29 12:49:44 PST
Comment on attachment 104437 [details] [diff] [review]
patch v5

Please remove the extra tabs, thanks.

>+    char originBuf1[512];
>+    char* origin = originBuf1;
>+	size_t originSize = sizeof(originBuf1);
>+    rv = securityContext->GetOrigin(origin, sizeof(origin));

sizeof(origin) should be originSize

sr=dveditz
Comment 35 Joe Chou 2002-10-29 13:10:35 PST
Mitch,
Patch v5 seems working fine, passed all my test cases.
Comment 36 Johnny Stenback (:jst, jst@mozilla.com) 2002-10-29 15:09:39 PST
Comment on attachment 104437 [details] [diff] [review]
patch v5

- In nsScriptSecurityManager::GetPrincipalFromContext():

+    NS_ENSURE_TRUE(::JS_GetOptions(cx) & JSOPTION_PRIVATE_IS_NSISUPPORTS,
+		    NS_ERROR_FAILURE);
+    nsCOMPtr<nsISupports> scriptContextSupports =
+	 NS_REINTERPRET_CAST(nsISupports*, JS_GetContextPrivate(cx));

No need for a strong nsISupports reference here, a raw pointer would do here
and that would avoid the unnecessary AddRef() and Release() on the context.
|scriptContext| (below) will hold a strong reference for you...

+    nsCOMPtr<nsIScriptContext>
scriptContext(do_QueryInterface(scriptContextSupports));

- In nsCLiveconnect.cpp:

>+CreatePrincipal(nsISupports* securitySupports,

+    char originBuf1[512];
+    char* origin = originBuf1;
+	size_t originSize = sizeof(originBuf1);

Tab.

+    rv = securityContext->GetOrigin(origin, sizeof(origin));
+	while (NS_FAILED(rv) && originSize < 65536U)

Tab.

+    {	// Try allocating a larger buffer on the heap
+		if (origin != originBuf1)
+			PR_Free(origin);
+		originSize *= 2;

More tabs.

+	 origin = (char*)PR_Malloc(originSize);
+	 if (!origin)
+	     return NS_ERROR_OUT_OF_MEMORY;
+	 rv = securityContext->GetOrigin(origin, originSize);
+    }

I don't like this loop, it's IMO too fragile. It assumes that GetOrigin() only
fails if the origin needs more space than what's passed in. If GetOrigin()
fails (and it does fail in cases other than the one you expect here) this loop
will run until PR_Malloc() returns null (i.e. we're out of memory), and then
it'll return NS_ERROR_OUT_OF_MEMORY (after swapping like mad, probably), and
that's wrong. IMO the API nsISecurityConext::GetOrigin() is broken, and doing
the right thing with it is hard. I'd suggest you cap off the max size you'll
try to allocate in this code, at some insanely large number (say 32k?) and
return the error code you get from GetOrigin() if you run into a case where
GetOrigin() fails and you're passing it enough space that it shouldn't fail.

- In AutoPushJSContext::~AutoPushJSContext():

+    if(mContextStack)
+    {
+	 mContextStack->Pop(nsnull);
+	 mContextStack = nsnull;

No need to explicitly null out mContextStack here, the nsCOMPtr dtor will do
that for you.

r/sr=jst if you fix those issues.
Comment 37 Johnny Stenback (:jst, jst@mozilla.com) 2002-10-29 17:53:11 PST
Duh! Never mind my ranting about that loop, it's already checked that we don't
allocate too much, I can't believe I didn't see that!
Comment 38 Mitchell Stoltz (not reading bugmail) 2002-10-29 17:56:11 PST
Created attachment 104589 [details] [diff] [review]
patch v6

OK, all issues fixed, and tests are passing. Now I just need a=
Comment 39 Mitchell Stoltz (not reading bugmail) 2002-10-29 17:58:30 PST
Comment on attachment 104589 [details] [diff] [review]
patch v6

Carrying over review marks
Comment 40 chris hofmann 2002-10-29 18:54:45 PST
Comment on attachment 104589 [details] [diff] [review]
patch v6

a=chofmann 1.0.2 and 1.2 trunk
Comment 41 Mitchell Stoltz (not reading bugmail) 2002-10-29 21:20:46 PST
Fixed on trunk and branch.
Comment 42 Brendan Eich [:brendan] 2002-10-29 22:50:20 PST
Comment on attachment 104589 [details] [diff] [review]
patch v6

+	 if (origin != originBuf1)
+	 PR_Free(origin);

Indentation glitch.  Fix when you can.

/be
Comment 43 Michael Buckland 2002-10-30 10:13:40 PST
adding gayatri
Comment 44 bsharma 2002-10-30 12:43:59 PST
Verified on 2002-10-30-branch build on Win 2000.

Ran the test cases : http://warp/u/mstoltz/bugs/168316.html
 and                 http://warp/u/mstoltz/bugs/168316a.html

And the results were correct.

Note You need to log in before you can comment on or make changes to this bug.