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: OpenReader(zipfile) simply says "zip: not a valid zip file" for jar files with extra bytes, which Java and unzip handle fine #51337

Closed
davidmichaelkarr opened this issue Feb 23, 2022 · 9 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.

Comments

@davidmichaelkarr
Copy link

I wrote a small tool that uses "archive/zip" to look at some aspects of zip files. I've tested it with many zip and Java jar files, and it appears to work perfectly fine. I built it with Go v1.17 (not sure what patch version).

today I started working with a jar file from the "opentelemetry java" project, version 1.10.0. The jar file was provided to me by a member of a team in my organization. I don't know whether they got this directly from the opentelemetry Java github site or whether they assembled it themselves somehow. The opentelemetry Java site has that release (https://github.com/open-telemetry/opentelemetry-java/releases/tag/v1.10.0 ), but I don't see any binary artifacts.

When I run my tool against this jar file, it simply says

zip: not a valid zip file

with no other information. I stepped into the code, and that is the error returned from zip.OpenReader(zipfile).

I tried opening this jar file with "unzip", and it says the following:

warning [opentelemetry-javaagent.jar]:  3624 extra bytes at beginning or within zipfile
  (attempting to process anyway)

Except for this warning, it processes it perfectly fine. Java and the "jar" tool in the Java distribution don't even present a warning. It works fine in Java.

@davidmichaelkarr
Copy link
Author

Note that you may have trouble repeating this if you just use the opentelemetry 1.10.0 jar file. I just tried downloading that jar file directly and running this test, and it doesn't have that problem. Somehow the jar file that ended up getting into my build process is slightly altered. We can deal with that, but I still think it's worth making archive/zip a little more robust, to let it deal with this somewhat nonstandard zip file.

@davidmichaelkarr
Copy link
Author

opentelemetry-javaagent.zip

I'm attaching the jar file (renamed to zip so I can upload it). Again, I don't know how it got into this state, but except for archive/zip OpenReader failing on it, Java is perfectly fine with it, and unzip processes it with a warning.

@raff
Copy link

raff commented Feb 24, 2022

Given that the extra garbage at the beginning are HTTP headers I suspect you downloaded the file with something like curl -i -o output.zip <url> instead of curl -s -o output.zip <url>

@seankhliao
Copy link
Member

I don't think anything should be done here, as noted above, the file has extra data (http headers) which isn't present in upstream source (https://github.com/open-telemetry/opentelemetry-java-instrumentation/releases/tag/v1.10.0), pointing to an error in your retrieval of the artifact.
Other tools like file also don't recognize it as a zip file.

@thanm thanm added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 24, 2022
@thanm
Copy link
Contributor

thanm commented Feb 24, 2022

@dsnet per owners.

As others have pointed out, the file in question has extra junk tacked onto the beginning, so it's not at all clear whether we would want to support this behavior. If we do allow extra junk, what are the rules? Is it ok to have extra junk just at the beginning, or can we have extra junk elsewhere as well? How many bytes of extra junk is OK and how much is too much? In your case you have about 3.6kbytes of extra junk-- what would happen if there were 5 MB of extra junk? If we allow extra junk in archive/zip files, what about ELF files or Go object files, should that be supported too?

I'm going to close out this issue; feel free to reopen if you disagree. Thanks.

@thanm thanm closed this as completed Feb 24, 2022
@davidmichaelkarr
Copy link
Author

Just so it's clear, both Java and Python can open and process this file without complaint.

@ericchiang
Copy link
Contributor

I believe this is a dup of #10464

@ianlancetaylor
Copy link
Member

@ericchiang Thanks, I agree.

With https://go.dev/cl/387976 the prefixed zip file can be read. I verified that the Linux unzip program can read it, and the patch that I wrote should act the same way.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/387976 mentions this issue: archive/zip: permit zip files to have prefixes

gopherbot pushed a commit that referenced this issue May 8, 2022
A Java jar file is a zip file, but it can have a prefix that is a bash
script that unpacks the zip file. Most zip programs ignore such prefixes.
This CL changes the archive/zip package to do the same.

Fixes #10464
Fixes #51337

Change-Id: I976e9c64684644317bd21077bc5b4a2baf626ee6
Reviewed-on: https://go-review.googlesource.com/c/go/+/387976
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Joseph Tsai <joetsai@digital-static.net>
Auto-Submit: Ian Lance Taylor <iant@google.com>
@golang golang locked and limited conversation to collaborators Feb 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

7 participants