Closed Bug 678745 Opened 9 years ago Closed 7 years ago

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


(Core :: JavaScript Engine, defect)

Not set





(Reporter: david.volgyes, Assigned: sunfish)




(1 file, 1 obsolete file)

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.
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.
Attached patch my proposed fix. (obsolete) — Splinter Review
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.
Version: 5 Branch → Trunk
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
Hardware: x86_64 → x86
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.
Ever confirmed: true
Blocks: cppcheck
Hey, I'm working on this bug. Don't officially assign me anything, but don't hand it off to a random volunteer please.
Alex, what is your progress on this? Are you still on it?
Attached patch shell-leak.patchSplinter Review
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.
Assignee: general → sunfish
Attachment #552890 - Attachment is obsolete: true
Attachment #8344632 - Flags: review?(acelists)
Comment on attachment 8344632 [details] [diff] [review]

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

Sorry, I am not a reviewer in JS module.
Attachment #8344632 - Flags: review?(acelists)
Comment on attachment 8344632 [details] [diff] [review]

Just a simple patch to use AutoCloseInputFile in a few more places, fixing some FILE leaks on error paths.
Attachment #8344632 - Flags: review?(jorendorff)
Comment on attachment 8344632 [details] [diff] [review]

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

Thank you!
Attachment #8344632 - Flags: review?(jorendorff) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.