1174922 - NativeZip does not null-terminate zip entry comparisons correctly

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Nick Alexander :nalexander [he/him]]

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.

Comment 1

[Nick Alexander :nalexander [he/him]]

rnewman: this was from your request for failure cases in extractStream.  My code was fine, but this bug was lurking ;)

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

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

Comment 3

[Mark Capella [:capella]]

Attachment: bug1174922.diff

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].

Comment 4

[Nick Alexander :nalexander [he/him]]

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.

Comment 5

[Mike Hommey [:glandium]]

(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 6

[(inactive) Jim Chen [:jchen] [:darchons]]

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'

Comment 7

[Mark Capella [:capella]]

TRY - https://treeherder.mozilla.org/#/jobs?repo=try&revision=87f852bc3723

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/237122da5354

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/237122da5354