Skip to content

git: reject malformed conflict labels - #9897

Open
ShiroKSH wants to merge 1 commit into
jj-vcs:mainfrom
ShiroKSH:git/reject-malformed-conflict-labels
Open

git: reject malformed conflict labels#9897
ShiroKSH wants to merge 1 commit into
jj-vcs:mainfrom
ShiroKSH:git/reject-malformed-conflict-labels

Conversation

@ShiroKSH

@ShiroKSH ShiroKSH commented Aug 1, 2026

Copy link
Copy Markdown

Summary

  • Reject malformed jj:conflict-labels commit headers instead of panicking while importing or reading Git commits.
  • Verify that conflict labels describe the same number of sides as the root tree.

Root cause

The header was decoded as UTF-8 and passed directly to Merge, which assumes an odd number of terms. A malformed Git commit could violate those assumptions and abort jj.

Tests

  • cargo test -p jj-lib --test runner test_invalid_conflict_labels_header -- --exact
  • cargo +stable clippy -p jj-lib --test runner -- -D warnings
  • cargo +nightly fmt --all -- --check

@ShiroKSH
ShiroKSH requested a review from a team as a code owner August 1, 2026 15:20
@google-cla

google-cla Bot commented Aug 1, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@PhilipMetzger

Copy link
Copy Markdown
Contributor

Can you please put the PR description into the commit description to adhere to the project commit guidelines. Thanks.

@ShiroKSH
ShiroKSH force-pushed the git/reject-malformed-conflict-labels branch from d0ab457 to f9d7707 Compare August 1, 2026 15:24
@ShiroKSH

ShiroKSH commented Aug 1, 2026

Copy link
Copy Markdown
Author

Updated in f9d7707. The commit description now contains the PR summary, root cause, and local checks.

@martinvonz martinvonz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks.

From commit description:

  • cargo test -p jj-lib --test runner test_invalid_conflict_labels_header -- --exact
  • cargo +stable clippy -p jj-lib --test runner -- -D warnings
  • cargo +nightly fmt --all -- --check

FYI, we run all that on CI (and all tests, of course), so there's no need to include the commit description. Any manual tests you ran could be useful, however, but there's no expectations that you run manual tests.

Out of curiosity, did you run into that crash in practice? If so, do you know how the bad header ended up in the commit?

Comment thread lib/tests/test_git_backend.rs
Comment thread lib/src/git_backend.rs Outdated
@ShiroKSH
ShiroKSH force-pushed the git/reject-malformed-conflict-labels branch 2 times, most recently from 626ae3e to b5e6b9a Compare August 12, 2026 23:13
Reject malformed jj:conflict-labels headers instead of panicking while importing or reading Git commits.

The header is decoded as UTF-8 and converted to Merge, which requires an odd number of terms. Reject an invalid UTF-8 header or an even number of labels before constructing the merge.
@ShiroKSH
ShiroKSH force-pushed the git/reject-malformed-conflict-labels branch from b5e6b9a to 0015549 Compare August 12, 2026 23:13
@ShiroKSH

Copy link
Copy Markdown
Author

Addressed in 0015549.

  • lib/src/git_backend.rs: removed the root-tree/label-count comparison; malformed labels are rejected before constructing Merge.
  • lib/tests/test_git_backend.rs: added comments identifying malformed-input and read-path coverage.
  • Removed automated CI commands from the commit description.

This is defensive hardening. I do not have a concrete crash report or provenance for a malformed header to add.

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.

3 participants