Violating same-origin with Java

VERIFIED FIXED

Status

()

Core
Security
--
critical
VERIFIED FIXED
15 years ago
15 years ago

People

(Reporter: Mitchell Stoltz (not reading bugmail), Assigned: Mitchell Stoltz (not reading bugmail))

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:blocker])

Attachments

(4 attachments, 5 obsolete attachments)

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.
(Assignee)

Comment 1

15 years ago
Created attachment 98925 [details]
Demo part 1 - the html file
(Assignee)

Comment 2

15 years ago
Created attachment 98926 [details]
Demo part 2a - Source of the applet
(Assignee)

Comment 3

15 years ago
Created attachment 98927 [details]
Demo part 2b - compiled applet
On further testing, latest mozilla trunk with the netscape 7beta java plugin is
affected by this bug.
Keywords: nsbeta1
(Assignee)

Comment 5

15 years ago
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.
Severity: normal → critical

Comment 6

15 years ago
Ccing some OJI folks to look at this serious bug.

Comment 7

15 years ago
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

15 years ago
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?
The problem is also present ot latest trunk at least on linux.
This is super serious ... it should block 1.2.
Whiteboard: [sg:blocker]
Agreed, this is serious.
(Assignee)

Comment 12

15 years ago
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.
Blocks: 174647

Comment 13

15 years ago
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 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

15 years ago
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.
(Assignee)

Comment 16

15 years ago
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.
Attachment #103019 - Attachment is obsolete: true
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 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).
Attachment #103019 - Attachment is obsolete: false
Attachment #103019 - Flags: superreview+
(Assignee)

Comment 19

15 years ago
Created attachment 104086 [details] [diff] [review]
Patch v3
Attachment #103019 - Attachment is obsolete: true
Attachment #104065 - Attachment is obsolete: true
(Assignee)

Comment 20

15 years ago
Comment on attachment 104086 [details] [diff] [review]
Patch v3

carrying forward jst's sr.
Attachment #104086 - Flags: superreview+
Feel free to cc: me on any such bug.

/be
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

15 years ago
adt approval once drivers approve
Blocks: 168047
Keywords: adt1.0.2+
(Assignee)

Comment 24

15 years ago
Created attachment 104208 [details] [diff] [review]
patch v4

This should do it...
Attachment #104086 - Attachment is obsolete: true

Comment 25

15 years ago
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

15 years ago
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

15 years ago
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.
(Assignee)

Comment 28

15 years ago
Joe,
   Could you send me the testcase that's crashing, or post a link to it here?

Comment 29

15 years ago
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):




(Assignee)

Comment 30

15 years ago
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

15 years ago
Yes, I tried it on Solaris. I'll try it on Linux, and see what happens.

Comment 32

15 years ago
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);
        }

}
(Assignee)

Comment 33

15 years ago
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.
Attachment #104208 - Attachment is obsolete: true
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
Attachment #104437 - Flags: superreview+

Comment 35

15 years ago
Mitch,
Patch v5 seems working fine, passed all my test cases.
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.
Attachment #104437 - Flags: review+
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!
(Assignee)

Comment 38

15 years ago
Created attachment 104589 [details] [diff] [review]
patch v6

OK, all issues fixed, and tests are passing. Now I just need a=
Attachment #104437 - Attachment is obsolete: true
(Assignee)

Comment 39

15 years ago
Comment on attachment 104589 [details] [diff] [review]
patch v6

Carrying over review marks
Attachment #104589 - Flags: superreview+
Attachment #104589 - Flags: review+

Comment 40

15 years ago
Comment on attachment 104589 [details] [diff] [review]
patch v6

a=chofmann 1.0.2 and 1.2 trunk
Attachment #104589 - Flags: approval+
(Assignee)

Comment 41

15 years ago
Fixed on trunk and branch.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Keywords: fixed1.0.2
Resolution: --- → FIXED
Comment on attachment 104589 [details] [diff] [review]
patch v6

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

Indentation glitch.  Fix when you can.

/be

Comment 43

15 years ago
adding gayatri

Comment 44

15 years ago
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.
Status: RESOLVED → VERIFIED
Keywords: fixed1.0.2 → verified1.0.2
(Assignee)

Updated

15 years ago
Group: security
You need to log in before you can comment on or make changes to this bug.