Skip to content

Conversation

@benjaminp
Copy link
Collaborator

The output of ijar manifest now includes Multi-Release: true if the input jar manifest has it.

Fixes #28316

@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Jan 16, 2026
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds support for preserving the Multi-Release: true attribute in the manifest of generated interface jars. The implementation involves refactoring manifest handling to read the input jar's manifest upfront and pass it to the processing logic. The changes are well-structured and include corresponding tests. I've identified a critical null-pointer-dereference bug due to an unchecked malloc and a high-severity correctness issue in the manifest parsing logic. Please see my comments for details.

Comment on lines +84 to +85
manifest_buf_ = (u1 *)malloc(size);
memmove(manifest_buf_, data, size);

Choose a reason for hiding this comment

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

critical

malloc can return NULL if memory allocation fails. The subsequent call to memmove would then dereference a null pointer, leading to a crash. You should check the return value of malloc and handle the error, for example by calling abort() which is consistent with error handling in other parts of this file.

Also, since the source and destination memory areas do not overlap, memcpy is more appropriate and potentially more performant than memmove.

    manifest_buf_ = (u1 *)malloc(size);
    if (manifest_buf_ == nullptr && size > 0) {
      fprintf(stderr, "ijar: Failed to allocate memory for manifest.\n");
      abort();
    }
    memcpy(manifest_buf_, data, size);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ignoring allocation failures is the preexisting style of this file.

The output of ijar manifest now includes Multi-Release: true if the input jar manifest has it.

Fixes bazelbuild#28316
@meisterT meisterT requested a review from cushon January 19, 2026 11:13
@hvadehra hvadehra added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jan 20, 2026
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jan 21, 2026
@benjaminp benjaminp deleted the ijar-multirelease branch January 21, 2026 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ijar should preserve Multi-Release manifest entry

3 participants