Closed Bug 337763 Opened 14 years ago Closed 14 years ago

Memory leak in ZIP_OpenArchive (modules/libjar/nsZipArchive.cpp)

Categories

(Core :: Networking: File, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED

People

(Reporter: kherron+mozilla, Assigned: ehsan)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, memory-leak, Whiteboard: [good first bug])

Attachments

(2 files, 5 obsolete files)

This is coverity CID 887. Please see the sample URL. The memory allocated at line 172 is leaked if the |PR_Open()| at line 176 fails.
Whiteboard: [good first bug]
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → rflint
Status: NEW → ASSIGNED
Attachment #222766 - Flags: review?(jwalden+bmo)
Comment on attachment 222766 [details] [diff] [review]
Patch

>   nsZipArchive* zip = new nsZipArchive();
...
>-  if (!fd)
>+  if (!fd) {
>+    free(zip);
>     return ZIP_ERR_DISK;
>+  }

You're relying upon |new| using malloc() internally to allocate space for the nsZipArchive, which isn't guaranteed to be a valid assumption; use |delete| instead of |free|.
Attachment #222766 - Flags: review?(jwalden+bmo) → review-
Or change:
177   if (!fd)
178     return ZIP_ERR_DISK;
179 
180   status = zip->OpenArchive(fd);
181   if (status == ZIP_OK)
182     *hZip = NS_STATIC_CAST(void*,zip);
183   else
184     delete zip;
into:
177   status = fd ? zip->OpenArchive(fd) : ZIP_ERR_DISK;
181   if (status == ZIP_OK)
182     *hZip = NS_STATIC_CAST(void*,zip);
183   else
184     delete zip;
Attached patch Correction to attachment #222766 (obsolete) — Splinter Review
Since no one has posted a patch on this...

By the way, is there not any smart pointer used in Mozilla code?  Wouldn't it be better to replace the usage of raw pointers such as this case with smart pointers, so that these bugs don't go undetected?
Attachment #222766 - Attachment is obsolete: true
Attachment #235866 - Flags: review?(jwalden+bmo)
there are smart pointer classes: nsAutoPtr<T>, nsAutoArrayPtr<T>
(In reply to comment #5)
> there are smart pointer classes: nsAutoPtr<T>, nsAutoArrayPtr<T>

So is it not better that try to replace raw pointer usage with smart pointers, wherever possible?  Fixes like the patch I've posted are likely top break if someone adds another dynamic memory allocation somewhere along the way later (and probably the reason for the appearance of such bugs in the first place).

I understand that this is a huge task, but maybe if it's broken down to smaller bugs in each component and module, it would be possible to do.

I tried searching the Bugzilla for possible bugs tracking this issue, but unfortunately most of what I got back was about mouse pointers.  :-)  Does anyone happen to know of any bug which is tracking this issue?
(In reply to comment #6)
> So is it not better that try to replace raw pointer usage with smart pointers,
> wherever possible?

Absolutely. Note that these smart pointers are newer than most of the codebase, which is why much code doesn't use them.

> Does
> anyone happen to know of any bug which is tracking this issue?

I don't know of such a bug...
Attached patch Correction to attachment #222766 (obsolete) — Splinter Review
According to comment #7, I changed the patch to use nsAutoPtr.  This fixes the original problem as well.  Also, check out bug #351222.
Assignee: rflint → ehsan.akhgari
Attachment #235866 - Attachment is obsolete: true
Attachment #236614 - Flags: review?(jwalden+bmo)
Attachment #235866 - Flags: review?(jwalden+bmo)
Comment on attachment 236614 [details] [diff] [review]
Correction to attachment #222766 [details] [diff] [review]

you can't delete a PRFileDesc
Attachment #236614 - Flags: review?(jwalden+bmo) → superreview-
(i.e. you should use PR_Close; so you shouldn't use an nsAutoPtr for PRFileDesc objects)
Attached patch Correction to attachment #236614 (obsolete) — Splinter Review
Addressed the issue raised in comment #9 and comment #10.
Attachment #236614 - Attachment is obsolete: true
Attachment #236653 - Flags: superreview?(cbiesinger)
Attachment #236653 - Flags: review?(jwalden+bmo)
Comment on attachment 236653 [details] [diff] [review]
Correction to attachment #236614 [details] [diff] [review]

sorry to mention another issue, but... isn't this deleting the object even when it shouldn't? i.e. consider this line:
181   if (status == ZIP_OK)
182     *hZip = NS_STATIC_CAST(void*,zip);

in that case, the object shouldn't be deleted as far as I can tell...
The issue in comment #12 was valid.  I fixed it, as well a couple of other potential issues.  I believe that this patch gets it better.
Attachment #236653 - Attachment is obsolete: true
Attachment #236673 - Flags: superreview?(cbiesinger)
Attachment #236673 - Flags: review?(jwalden+bmo)
Attachment #236653 - Flags: superreview?(cbiesinger)
Attachment #236653 - Flags: review?(jwalden+bmo)
Attachment #236673 - Flags: superreview?(cbiesinger) → superreview+
Note that ZIP_OpenArchive only gets compiled in 'STANDALONE' mode, 
so using AutoPtr in this context doesn't get much benefit, except for the overhead of using this construct.... (the STANDALONE libjar is only used in the installer, which is by the way being replaced by the NSIS installer...)

Note also that if zip->OpenArchive fails, we still leak the filedescriptor.

So in plain, portable C++, it can be coded like this:
  /*--- create the archive ---*/
  nsZipArchive* zip = new nsZipArchive();
  if (zip == 0)
    return ZIP_ERR_MEMORY;

  /*--- open the archive ---*/
  PRFileDesc * fd = PR_Open(zipname, PR_RDONLY, 0400);
  PRInt32 status = fd ? zip->OpenArchive(fd) : ZIP_ERR_DISK;
  if (status == ZIP_OK) {
    *hZip = NS_STATIC_CAST(void*,zip);
  } else {
    delete zip;
    PR_Close(fd);
  }

  return status;

This will make sure that both the zip and the fd gets freed.
So, please keep it as simple as possible.
Comment on attachment 236673 [details] [diff] [review]
Correction to attachment #236653 [details] [diff] [review]

Sigh...Alfred's right that we leak the fd, and we really should handle that while we're changing code here.
Attachment #236673 - Flags: review?(jwalden+bmo) → review-
oh, one more thing... I'd do the nullcheck as:
  if (!zip)
instead.

What overhead are you talking about?
(In reply to comment #16)
> What overhead are you talking about?

I'd assume for the stack-allocated nsAutoPtr (and in this case, for the nsAutoPtr<nsZipArchive> class definition code, since I don't think one already exists).
I'd hope that almost all of that gets optimized away, although I could be wrong there...
(the stack space required is the same as for a raw pointer, though)
(In reply to comment #14)
> Note that ZIP_OpenArchive only gets compiled in 'STANDALONE' mode, 
> so using AutoPtr in this context doesn't get much benefit, except for the
> overhead of using this construct.... (the STANDALONE libjar is only used in the
> installer, which is by the way being replaced by the NSIS installer...)

I'm not sure what a STANDALONE build is, and why using smart pointers in such builds is a bad idea.  But about the overhead, I think any decent C++ compiler would inline the smart pointer calls in a way that not much difference between using a smart pointer and a raw pointer in terms of efficiency remains.  I know VC++ does this.

But anyhow this patch fixes the issue without using a smart pointer, FWIW.  And of course, this patch handles freeing the fd with PR_Close() as well.
It is probably a personal preference, the important part is that the leaks are fixed, and that the patch is as simple as possible with minimal impact.
(In reply to comment #21)
> It is probably a personal preference, the important part is that the leaks are
> fixed, and that the patch is as simple as possible with minimal impact.

Cool.  So, I guess attachment #236775 [details] [diff] [review] is the way to go.
Attachment #236775 - Flags: superreview?(cbiesinger)
Attachment #236775 - Flags: review?(jwalden+bmo)
Comment on attachment 236775 [details] [diff] [review]
Patch without using smart pointer

sigh, ok then...

although I don't really like this line:
+  status = fd ? zip->OpenArchive(fd) : ZIP_ERR_DISK;

it emphasizes the assignment to status, while the important thing here is the function call.

this code is IMO a lot more readable with the smart pointer and the early return...
Comment on attachment 236775 [details] [diff] [review]
Patch without using smart pointer

if you want to avoid it though, I'd prefer to keep the early return after the PR_Open and add a delete zip into it instead of using the ternary operator
Attachment #236775 - Flags: superreview?(cbiesinger) → superreview-
Addressed the issue raised in comment #24.
Attachment #236775 - Attachment is obsolete: true
Attachment #236823 - Flags: superreview?(cbiesinger)
Attachment #236823 - Flags: review?(jwalden+bmo)
Attachment #236775 - Flags: review?(jwalden+bmo)
Comment on attachment 236823 [details] [diff] [review]
Patch without using smart pointer (revised)

Maybe I've been looking at Spidermonkey code too much recently, but I almost wonder if this couldn't be made clearer with well-placed gotos.  It's probably overkill here, tho, and this looks fine as it is.
Attachment #236823 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 236823 [details] [diff] [review]
Patch without using smart pointer (revised)

thanks. do you need someone to check this in for you?
Attachment #236823 - Flags: superreview?(cbiesinger) → superreview+
(In reply to comment #27)
> (From update of attachment 236823 [details] [diff] [review] [edit])
> thanks. do you need someone to check this in for you?

Yes, please.  I don't have CVS write access.
fixed on trunk
Checking in modules/libjar/nsZipArchive.cpp;
/cvsroot/mozilla/modules/libjar/nsZipArchive.cpp,v  <--  nsZipArchive.cpp
new revision: 1.87; previous revision: 1.86
done
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Keywords: qawanted
Keywords: qawanted
Keywords: verifyme
Keywords: mlk
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.