Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

archive/zip: can't handle certain Java-generated zip (apk) files #8186

Closed
bradfitz opened this issue Jun 11, 2014 · 14 comments
Closed

archive/zip: can't handle certain Java-generated zip (apk) files #8186

bradfitz opened this issue Jun 11, 2014 · 14 comments
Assignees
Milestone

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jun 11, 2014

We received a bug report from an internal user that archive/zip can't open certain
Android apk (zip) files.

The problem is the zip's Extra field appears to be short.

The extra field is "\xfe\xca\x00\x00\x00\x00".

An Extra field is supposed to be a repetition of (2 byte tag, 2 byte size, size-bytes of
data).

So that's tag 51966 ("CAFE", apparently: a Java thing?
https://issues.apache.org/bugzilla/show_bug.cgi?id=32649), size 0, and then we have
"\x00\x00" left over, but those two bytes aren't long enough (we always expect
at least 4 bytes: 2 for tag, 2 for size).

But the "zip" tool handles it fine, so perhaps we should just skip over extra
fields when they're short.

The zipdetails program is also confused with it a bit (despite exiting with status 0),
with an"Unexpecded" (sic) status:

$ zipdetails Contacts.apk

000000 LOCAL HEADER #1       04034B50
000004 Extract Zip Spec      0A '1.0'
000005 Extract OS            00 'MS-DOS'
000006 General Purpose Flag  0800
       [Bit 11]              1 'Language Encoding'
000008 Compression Method    0000 'Stored'
00000A Last Mod Time         3F339D41 'Mon Sep 19 19:42:02 2011'
00000E CRC                   1E85E138
000012 Compressed Length     00000086
000016 Uncompressed Length   00000086
00001A Filename Length       0030
00001C Extra Length          0006
00001E Filename              '...(redacted)....'
00004E Extra ID #0001        CAFE 'Java Executable'
000050   Length              0000
000052 PAYLOAD               ...PNG........IHDR....................4n
                             (redacted)

Unexpecded END at offset 000000D8, value 4B508260
Done




.... Perhaps we need to special case that extra field type, and say that at least a
"\x00\x00" extra tag without a size is an acceptable EOF, either always, or
only following a CAFE tag.
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 17, 2014

Comment 1 by ag@ashishgandhi.org:

Maybe I'm being stupid but can't seem to be able to reproduce the problem. Here's what I
tried:
1. Get Contacts.apk from Gingerbread. (Going by the date in the dump and the fact that
later versions don't seem to have CAFE. Source:
http://forum.xda-developers.com/showthread.php?t=923292.)
2. zipdetails Contacts.apk on the Contacts.apk attached here exhibit the same things as
the dump in this report.
3. Move Contacts.apk into src/pkg/archive/zip/testdata.
4. Add a test case in src/pkg/archive/zip/reader_test.go.
(https://gist.github.com/ashishgandhi/e88d5399a219ca1703d4)
5. All tests pass.

Attachments:

  1. Contacts.apk (585757 bytes)
@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Jun 17, 2014

Comment 2:

It's not all versions of the misc Java/Android toolchains. It's only happening more
recently with later versions, it seems. Gingerbread was fine.
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 17, 2014

Comment 3 by ag@ashishgandhi.org:

Tried with ICS Contacts.apk from
http://forum.xda-developers.com/showthread.php?t=1735507. The tests passed. What would
be a good way get hold of a misbehaving archive?

Attachments:

  1. Contacts.apk (1760677 bytes)
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 18, 2014

Comment 4 by ashish@exceptional.io:

Or if there's a way to produce these misbehaving archives that'd be best. That way I
could create a version that's smaller than a Contacts.apk would be and add it to
testdata.
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 18, 2014

Comment 5 by ag@ashishgandhi.org:

Or if there's a way to produce these misbehaving archives that'd be best. That way I
could create a version that's smaller than a Contacts.apk would be and add it to
testdata.
@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Jun 18, 2014

Comment 6:

I will create a synthetic one when I fix the bug for testdata.
The apk I have is of an unreleased Android app and is also too large.
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 19, 2014

Comment 7 by ag@ashishgandhi.org:

To document one more thing that I tried (in hope it may help someone avoid the same
pitfalls). I created an executable JAR using Oracle's latest JDK.
$ zipdetails Noop.jar
0000 LOCAL HEADER #1       04034B50
0004 Extract Zip Spec      14 '2.0'
0005 Extract OS            00 'MS-DOS'
0006 General Purpose Flag  0808
     [Bits 1-2]            0 'Normal Compression'
     [Bit  3]              1 'Streamed'
     [Bit 11]              1 'Language Encoding'
0008 Compression Method    0008 'Deflated'
000A Last Mod Time         44D28AE1 'Wed Jun 18 17:23:02 2014'
000E CRC                   00000000
0012 Compressed Length     00000000
0016 Uncompressed Length   00000000
001A Filename Length       0009
001C Extra Length          0004
001E Filename              'META-INF/'
0027 Extra ID #0001        CAFE 'Java Executable'
0029   Length              0000
002B PAYLOAD               ..
(redacted)
$ hexdump -C Noop.jar | grep "fe ca 00  00"
00000200  2d 49 4e 46 2f fe ca 00  00 50 4b 01 02 14 00 14  |-INF/....PK.....|
I failed to get tests to fail.
(https://gist.github.com/ashishgandhi/335918f19261f05de4ba)
I guess I might be being stupid. Apologies for the wasted effort.

Attachments:

  1. Noop.jar (665 bytes)
@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Aug 4, 2014

Comment 8:

Sent https://golang.org/cl/117550044
Tagging for consideration for Go 1.3.1 because there is no real workaround (short of
forking the package) and this is affecting users.

Labels changed: added release-go1.3.1, removed release-go1.4.

Owner changed to @bradfitz.

Status changed to Started.

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 4, 2014

Comment 9:

CL https://golang.org/cl/117550044 mentions this issue.
@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Aug 4, 2014

Comment 10:

This issue was closed by revision 77df26f.

Status changed to Fixed.

@rsc
Copy link
Contributor

@rsc rsc commented Aug 11, 2014

Comment 11:

Inclined to say no for Go 1.3.1.
People being affected do have a workaround: copy the zip package for now.
Nothing hard-codes a dependency on zip (like things do for net/http or
database/sql/driver) so that shouldn't be too bad.
Arguments to the contrary welcome.

Labels changed: added release-go1.4, removed release-go1.3.1.

@bradfitz bradfitz added fixed labels Aug 11, 2014
@bradfitz bradfitz self-assigned this Aug 11, 2014
@gophry
Copy link

@gophry gophry commented Feb 20, 2015

I am using:
go version go1.4.1 darwin/amd64

Facing the same issue still.

@adg
Copy link
Contributor

@adg adg commented Feb 22, 2015

@ykamat can you please provide more information?

@gophry
Copy link

@gophry gophry commented Feb 22, 2015

andrew, considering closed i had raised another ticket:
#9943

the go files, 2 apk and 2 zip files can be found at the following link for testing:
https://www.dropbox.com/sh/qwkkzdyn3g7alef/AABGTMQOsVfOLgm1ct-S7tXfa?dl=0

the issue does not seem to be with the go zip package but with the way i have handled file descriptors.

The one thing though that confuses me is the zip files open perfectly but the apk does not.
Also files in the apk like dex and xml do get uncompressed.

@rsc rsc added this to the Go1.4 milestone Apr 14, 2015
@rsc rsc removed the release-go1.4 label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
wheatman added a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018
Popular tools both add incorrect trailing zeroes to the zip
extras, and popular tools accept trailing zeros. We seemed to
be the only ones being strict here. Stop being strict. :(

Fixes golang#8186

LGTM=ruiu, adg, dave
R=adg, ruiu, dave
CC=frohrweck, golang-codereviews
https://golang.org/cl/117550044
wheatman added a commit to wheatman/go-akaros that referenced this issue Jul 9, 2018
Popular tools both add incorrect trailing zeroes to the zip
extras, and popular tools accept trailing zeros. We seemed to
be the only ones being strict here. Stop being strict. :(

Fixes golang#8186

LGTM=ruiu, adg, dave
R=adg, ruiu, dave
CC=frohrweck, golang-codereviews
https://golang.org/cl/117550044
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.