Closed Bug 1174922 Opened 4 years ago Closed 4 years ago

NativeZip does not null-terminate zip entry comparisons correctly

Categories

(Core :: Widget: Android, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: nalexander, Assigned: capella)

Details

Attachments

(1 file)

Using the GeckoJarReader/NativeZip code, I find that

jar:jar:file:/data/app/org.mozilla.fennec_nalexander-2.apk!/assets/omni.ja!/BADchrome.manifest correctly

fails to read, while

jar:jar:file:/data/app/org.mozilla.fennec_nalexander-2.apk!/assets/omni.ja!/chrome.manifestBAD

returns the stream for chrome.manifest.  jchen confirms that we're comparing non-null-terminated file names without specifying a length, effectively accepting a prefix.
rnewman: this was from your request for failure cases in extractStream.  My code was fine, but this bug was lurking ;)
More specifically, StringBuf::Equals [1] compares the zip filename with the given filename up until the last character of the zip filename, so we accept the given filename even if it has extra characters at the end. This will potentially be a bigger problem if we ever have a file whose name starts with another file's name.

[1] http://mxr.mozilla.org/mozilla-central/source/mozglue/linker/Zip.h?rev=3c26bef95d54#271
Attached patch bug1174922.diffSplinter Review
Quick tweak to account for an exact string length match ...

Fixed up the robocop test [testJarReader.java] to include a new testcase and correct an apparent issue (see bug Bug 1187168).

Also fixed up TestJarReader.java to include a new testcase. This seems to be a |./mach gradle ...| test that does much of the same as [testJarReader.java].
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #8638567 - Flags: review?(nalexander)
Comment on attachment 8638567 [details] [diff] [review]
bug1174922.diff

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

Redirecting to Jim.  Jim, I don't feel confident in my C buffer handling without thinking a while.  Is there a way to test this without iterating the string again using strlen()?  Would it be safe to check, after the strncmp, that str[length] == 0?

The Java test changes look fine.
Attachment #8638567 - Flags: review?(nalexander) → review?(nchen)
(In reply to Nick Alexander :nalexander (PTO July 17-27) from comment #4)
> Comment on attachment 8638567 [details] [diff] [review]
> bug1174922.diff
> 
> Review of attachment 8638567 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Redirecting to Jim.  Jim, I don't feel confident in my C buffer handling
> without thinking a while.  Is there a way to test this without iterating the
> string again using strlen()?  Would it be safe to check, after the strncmp,
> that str[length] == 0?

Yes, if strncmp returns 0.
Comment on attachment 8638567 [details] [diff] [review]
bug1174922.diff

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

What glandium said, i.e. strncmp(str, buf, length) == 0 && str[length] == '\0'
Attachment #8638567 - Flags: review?(nchen) → review+
https://hg.mozilla.org/mozilla-central/rev/237122da5354
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.