384901 - Memory leaks in jsfile.c

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Robin Ehrlich]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322; .NET CLR 2.0.50727)
Build Identifier: 1.60

There are a few memory leaks in jsfile.c. The diff below gives the changes necessary to correct the leaks.

E:\Development Tools\mozilla.org\js-1.60\src>

diff jsfile.orig.c jsfile.c

472a473
>         JS_free(cx, path);
2045a2047
>     size_t len;
2052c2054,2055
<     urlChars = js_InflateString(cx, url, strlen(url));
---
>     len = strlen(url);
>     urlChars = js_InflateString(cx, url, &len);
2550a2554
>             PR_CloseDir(dir);


Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Brian Crowder]

Robin, would you mind providing this as a unified diff with a few lines of context, in an attachment (as opposed to pasted into the bug), please?  "cvs diff -up8" should yield precisely what we want.  Thanks!

(assigning to myself to ensure landing -- pretty sure these are legit, but it's hard to be sure without context)

Comment 2

[Robin Ehrlich]

Attachment: The attached file "jsfile.c" contains my changes to the source.

There are actually two minor sets of changes. The first change around js_InflateString corrects a compile error. The other changes are memory leaks.
The memory leaks were found by running purify. After the changes the memory leaks no longer occured.

Comment 3

[Brian Crowder]

Sorry, Robin:  Are you not able to provide a patch as described here:  http://developer.mozilla.org/en/docs/Creating_a_patch ?  The reason this is helpful is that it is easier to review, ensures that your version of the file is up-to-date, and makes it less likely that your code will "rot" (ie., become out-of-date) before being checked in.

Comment 4

[Robin Ehrlich]

Attachment: diff of the original file and my changes "-u8p jsfile.orig.c jsfile.c"

I am sorry, but I don't have CVS on my system, but I now see that my diff supports the -u option. I didn't think it did.

Comment 5

[Brian Crowder]

         if (!tmp)
             return NULL;
+        JS_free(cx, path);


I don't see how you can free "path" at this point, given that it is then used a few lines later?  Did you perhaps mean something more like this:

-        if (!tmp)
+        if (!tmp) {
+            JS_free(cx, path);
             return NULL;
+        }

Your other changes seem already to be incorporated on the main trunk jsfile.c

Comment 6

[Robin Ehrlich]

My code in jsfile.c: 

    if (!js_isAbsolute(path)) {
        tmp = js_absolutePath(cx, path);
        if (!tmp)
            return NULL;
+       JS_free(cx, path);
        path = tmp;
    }

As you can see, path is assigned the value of tmp in the next line of code. Maybe the main trunk has other changes that already corrects this problem. My base source code is from the 1.60 tar file.

Comment 7

[Brian Crowder]

Attachment: the patch

Yeah, thanks.  I scanned right past that, whoops.  :)  Here's the patch I think we need.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 268816 [details] [diff] [review]
the patch

The jsdtoa.c changes here are accidentally included, right?

The jsfile.c change looks good.

Comment 9

[Brian Crowder]

Damn, yeah.  The jsdtoa stuff is unintended.  I will attach a real patch before I land.

Comment 10

[Brian Crowder]

Attachment: what I landed.

Comment 11

[Brian Crowder]

jsfile.c: 3.48