diff: reject negative context values#2105
Open
mmontalbo wants to merge 4 commits intogitgitgadget:masterfrom
Open
diff: reject negative context values#2105mmontalbo wants to merge 4 commits intogitgitgadget:masterfrom
mmontalbo wants to merge 4 commits intogitgitgadget:masterfrom
Conversation
mmontalbo
force-pushed
the
mm/reject-negative-interhunk-context
branch
from
7899c59 to
6a3f871
Compare
Negative values for --inter-hunk-context produce structurally invalid
diff output with overlapping hunks:
$ git log -1 -p -U3 --inter-hunk-context=-100 791aedd \
-- git-compat-util.h | grep '^@@'
@@ -110,6 +110,9 @@
@@ -115,6 +118,9 @@
@@ -116,6 +122,7 @@
Hunk 1 covers lines 110-115, hunk 2 starts at 115 (overlap), hunk 3
starts at 116 (overlaps both). The resulting patch cannot be applied.
The config variable diff.interHunkContext already rejects negative
values, but the command line option does not. The option currently
uses OPT_INTEGER_F with PARSE_OPT_NONEG, but PARSE_OPT_NONEG only
prevents the "--no-inter-hunk-context" boolean negation form. It does
not reject negative numeric arguments like "--inter-hunk-context=-1".
Change the type of diff_options.interhunkcontext and its static
default from int to unsigned int, and switch the option parser from
OPT_INTEGER_F to OPT_UNSIGNED. This rejects negative values at parse
time via git_parse_unsigned() and enforces the correct type at compile
time via BARF_UNLESS_UNSIGNED.
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
mmontalbo
force-pushed
the
mm/reject-negative-interhunk-context
branch
5 times, most recently
from
09e8bb9 to
03558ce
Compare
Passing a negative value to -U is silently accepted and produces
corrupt unified diff output with malformed hunk headers:
$ git log -1 -p -U-500 -- GIT-VERSION-GEN | grep '^@@'
@@ -503,999- +503,999- @@
Line 503 of a 106-line file, count "999-" is not a valid integer.
The config variable diff.context already rejects negative values, but
the command line callback diff_opt_unified() uses strtol() with no
range check.
Change the type of diff_options.context and its static default from
int to unsigned int, matching the change to interhunkcontext in the
previous commit. The type change requires reworking the callback and
config parsing to validate in a local variable before assigning to
the now-unsigned field.
Unlike --inter-hunk-context which could be converted to OPT_UNSIGNED,
-U needs OPT_CALLBACK_F for PARSE_OPT_OPTARG (bare -U with no value
enables patch output). Add a range check in the callback instead.
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
mmontalbo
force-pushed
the
mm/reject-negative-interhunk-context
branch
3 times, most recently
from
137bef2 to
9df24fc
Compare
The xdemitconf_t fields ctxlen and interhunkctxlen are typed as long
(signed), but negative values are not meaningful for context line
counts. Unlike the diff_options fields changed in the previous two
commits, these cannot be converted to unsigned because the xdiff
arithmetic relies on signed subtraction:
s1 = XDL_MAX(xch->i1 - xecfg->ctxlen, 0);
If ctxlen were unsigned long, the signed operand would be implicitly
converted to unsigned, and the subtraction would wrap to a large
positive value when i1 < ctxlen, defeating the XDL_MAX clamp. The
signed type is required for correct context-window calculations.
The previous two commits reject negative values at the parse layer
for --inter-hunk-context and -U/--unified, so negative values should
no longer reach xdiff in normal use. Add BUG() guards at the top of
xdl_get_hunk() as defense in depth to catch programming errors in
current or future callers that bypass option parsing.
xdl_get_hunk() is called by both xdl_emit_diff() and
xdl_call_hunk_func(), so a single guard covers all xdiff consumers.
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
mmontalbo
force-pushed
the
mm/reject-negative-interhunk-context
branch
2 times, most recently
from
cde6d59 to
57f913f
Compare
The name "NONEG" can be misread as "no negative [values]" when it actually means "no [boolean] negation" (the --no-* form). When --inter-hunk-context and -U/--unified were converted from a custom parser to OPT_INTEGER_F with PARSE_OPT_NONEG in d473e2e and 16ed6c9, the implicit rejection of negative values (via isdigit() in the old opt_arg() parser) was silently lost. The previous commits in this series fix the resulting bugs. Add a clarifying note to the flag documentation. Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
mmontalbo
force-pushed
the
mm/reject-negative-interhunk-context
branch
from
57f913f to
05ff821
Compare
Author
|
/preview |
|
Preview email sent as pull.2105.git.1778021368.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2105.git.1778022144.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Negative values for -U and --inter-hunk-context are silently accepted
and produce structurally invalid diff output.
Malformed hunk headers:
Line 503 of a 106-line file, count "999-" is not a valid integer.
Overlapping hunks that cannot be applied:
Both options were originally parsed via opt_arg() which gated on
isdigit(), making negative values impossible. When they were converted
to OPT_INTEGER_F / OPT_CALLBACK in d473e2e (diff.c: convert
-U|--unified, 2019-01-27) and 16ed6c9 (diff-parseopt: convert
--inter-hunk-context, 2019-03-24), the implicit rejection was lost.
PARSE_OPT_NONEG was added but only prevents the --no-* boolean form,
not negative numeric arguments.
This series restores the original invariant with stronger guarantees:
The config variables diff.context and diff.interHunkContext have
always rejected negative values. This series brings the CLI options
in line.