337120 - Memory leaks fix for manifest parsing in libjar

Status: NEW

(Core :: Networking: JAR, defect, P3)

Description

[Alfred Kayser]

Split from bug 214672:
Fixes for memory leaks and such in manifest parsing in libjar.
This code has been partial reviewed in bug 214672. 

Overview:
Postpone the ManifestTable creation from nsJAR creation to when it is really needed.
Replaced 'mTotalItemsInManifest' with ManifestTable->Count.
Optimized ReadLine (not strtok nor strlen, just search for \r(\n)
Only nsJARManifestItem when really needed.
Allocate storedSectionItem together with nsJARManifestItem.
Replaced Equals(NS_LIT... with corresponding EqualsLiteral.

Comment 1

[Alfred Kayser]

Attachment: Fix memory leaks in manifest parsing in libjar

Comment 2

[Reed Loden [:reed]]

Alfred, do you think you could unbitrot this and request review?

Comment 3

[Alfred Kayser]

Attachment: V3: Unbitrotted and <static_cast>.

Comment 4

[Alfred Kayser]

Tested with:
http://www.mozilla.org/projects/security/components/signed-script-example.html
jar:http://www.mozilla.org/projects/security/components/signed-script-demo.jar!/signed-script-demo.html

Comment 5

[Mike Schroepfer]

Any chance we can get a smaller patch isolated to the core memory changes (there are a bunch of other changes in this patch).

Any idea how big the leak is?

Comment 6

[Alfred Kayser]

Attachment: V4: Removed the unnecessary fixes from the patch

Removed the unneeded parts.

The 'leaks' are in multiple places:
1. When an nsJAR is allocated there is always an hashtable allocated for the manifest, even if it is totally not used.
2. During parsing of the manifest, nsJARManifestItem's will be leaked (especially when there are errors in the manifest).
3. Strings allocated for the manifest items are not always freed.
4. Use of autostring for CalculateDigest will also prevent leakage there.

Some other related issues: 
A. Passing a null pointer to GetManifestEntriesCount would result in a crash without this patch


Note, until recently signed jars where very rare, but now that FF3 requires/prefers signed jars for installation of extensions and themes, this will happen more and more.

Comment 7

[Mike Schroepfer]

Thnx alfred for the expln and the new patch.  Can we whip up some unit tests for this? 

Comment 8

[Alfred Kayser]

Attachment: V5: Updated to current trunk (hg), and use nsClassHashTable

See descriptions of earlier versions.
This removes potential memory leaks in manifest parsing, makes the manifest table lazily initialized (as most jar/zips don't have a manifest at all), and replaces nsObjectHashtable with nsClassHashtable.

Comment 9

[(dormant account)]

Comment on attachment 419544 [details] [diff] [review]
V5: Updated to current trunk (hg), and use nsClassHashTable

Since nsJARManifestItems were leaking, consider putting MOZ_COUNT_CTOR/DTOR stuff in the class.

>   return mZip.Test(aEntryName);
>@@ -354,59 +345,71 @@ nsJAR::GetInputStreamWithSpec(const nsAC
> NS_IMETHODIMP
> nsJAR::GetCertificatePrincipal(const char* aFilename, nsIPrincipal** aPrincipal)
> {
>   //-- Parameter check
>   if (!aPrincipal)
>     return NS_ERROR_NULL_POINTER;
>   *aPrincipal = nsnull;

We are trying to move away from setting outparams AND returning an error code.

>+/*
>+ * Moves pointer to beginning of next line and returns line length
>+ *   not including CR/LF.
>+ */
> PRInt32
> nsJAR::ReadLine(const char** src)
> {

This should be a static utility function.

The rest of the patch looks ok to me. I think this could also benefit from testcases.

Comment 10

[(dormant account)]

Comment on attachment 419544 [details] [diff] [review]
V5: Updated to current trunk (hg), and use nsClassHashTable

this needs a basic testcase for valid/missing/corrupt manifest cases

Comment 11

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 12

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258