678745 - resource leak (not closed file) in js/src/shell/js.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Volgyes]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110622232440

Steps to reproduce:

Cppcheck 1.49 found some resource (and memory) leaks, this is one of them.



Actual results:

In js/src/shell/js.cpp
in this function:
static JSObject *
FileAsTypedArray(JSContext *cx, const char *pathname)

there is an opened file, and it is not closed before "return"


Expected results:

You should close all opened file (and release all unnecessary allocated memory) before return.
I attached my proposed patch.
Please, review it.

Comment 1

[David Volgyes]

I am so sorry, I did something wrong. I have some opened file for bug reports, and I mixed the texts. Skip the whole original report (and patch), they are mixed.
In the next comment, I will do the fix. I am sorry.

Comment 2

[David Volgyes]

Attachment: my proposed fix.

Comment 3

[David Volgyes]

So actually, I am a little bit tired. js.cpp does not close a file, and hopefully I attached the right solution. (First, it seemed that I attached a wrong patch. But somehow, the file attach process canceled, so I did not upload the wrong patch, but I thought that (comment #1). When I realized that, I attached the right solution (I hope). And now, I try to clarify my plenty of comments.)
And finally, I marked the wrong version, because it was from trunk,and not from branch 5.) I am sorry for the lot of confusion I did.

Comment 4

[David Mandelin [:dmandelin]]

The patch looks correct. If you're up for it, you may want to refactor the function to use a C++ destructor to clean up the file--there's only one return now, but the destructor style is more robust to future changes.

Anyway, whether you want to subit this patch or one with a destructor, the next step is to request review on the patch.

Comment 5

[Alex Miller]

Hey, I'm working on this bug. Don't officially assign me anything, but don't hand it off to a random volunteer please.

Comment 6

[:aceman]

Alex, what is your progress on this? Are you still on it?

Comment 7

[Dan Gohman [:sunfish]]

Attachment: shell-leak.patch

Thank you for finding this bug and submitting a patch!

Sine the patch was submitted, js.cpp has gained an AutoCloseInputFile class which implements closing files in a destructor, which allows fixing this in the way suggested in comment 4. There hasn't been any activity here for a while, and I have an interest in getting this fixed, so I'm just going ahead and taking it.

Comment 8

[:aceman]

Comment on attachment 8344632 [details] [diff] [review]
shell-leak.patch

Review of attachment 8344632 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I am not a reviewer in JS module.

Comment 9

[Dan Gohman [:sunfish]]

Comment on attachment 8344632 [details] [diff] [review]
shell-leak.patch

Just a simple patch to use AutoCloseInputFile in a few more places, fixing some FILE leaks on error paths.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8344632 [details] [diff] [review]
shell-leak.patch

Review of attachment 8344632 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!

Comment 11

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b965f401c4f

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3b965f401c4f