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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting-review PR is awaiting review from an assigned reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ijar should preserve Multi-Release manifest entry

1 participant