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

Fix ijar failing on empty jars. #12893

Closed
wants to merge 4 commits into from
Closed

Conversation

comius
Copy link
Contributor

@comius comius commented Jan 25, 2021

The problem happened when all the files are filtered out. In such case a dummy file is created, but it was not accounted for in the size estimation.
Fixes #10162

The problem happens when all the files are filtered out. In such case a dummy file is created, but not accounted for in the size estimation.
Fixes bazelbuild#10162
@google-cla google-cla bot added the cla: yes label Jan 25, 2021
@comius comius requested a review from cushon January 25, 2021 19:57
@comius comius marked this pull request as ready for review January 25, 2021 19:57
Copy link
Contributor

@cushon cushon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a test?

I also commented in the bug--it's not clear if this is something that happened in a real build, it should be rare to see completely empty jar files that don't even contain a manifest.

in->CalculateOutputLength() +
EstimateManifestOutputSize(target_label, injecting_rule_kind);
u8 output_length = in->CalculateOutputLength();
if (output_length < 103ull + DUMMY_PATH_LENGTH) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does 103 come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From measurement, error message tells 108 bytes are needed. "dummy" appears twice in the output, so 98+2*strlen("dummy") should work even if somebody changes "dummy". (I fixed 103).

Somehow I cannot get the same number by adding together numbers used EstimateManifestOutputSize. And I consider reverse engineering zip code and figuring out header sizes a bit too much effort for this fix.

@@ -368,9 +370,12 @@ static void OpenFilesAndProcessJar(const char *file_out, const char *file_in,
strerror(errno));
abort();
}
u8 output_length =
in->CalculateOutputLength() +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder of CalculateOutputLength should be handling this instead of fixing up the size later?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a test?

Done

I also commented in the bug--it's not clear if this is something that happened in a real build, it should be rare to see completely empty jar files that don't even contain a manifest.

True that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CalculateOutputLength is part of zip.cc and has no knowledge that we're adding dummy file.

Copy link

@ahmadasiri-24 ahmadasiri-24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

third_party/ijar/ijar.cc

@bazel-io bazel-io closed this in 7610755 Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ijar fails on empty jars
3 participants