diff: reject negative context values

Negative values for -U and --inter-hunk-context are silently accepted and produce structurally invalid diff output.

Malformed hunk headers:

$ wc -l GIT-VERSION-GEN
106
$ 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.

Overlapping hunks that cannot be applied:

$ git log -1 -p -U3 --inter-hunk-context=100 791aeddfa2 \                                                      
    -- git-compat-util.h | git apply --check --reverse
(success)                                                                                                      

$ git log -1 -p -U3 --inter-hunk-context=-100 791aeddfa2 \                                                     
    -- git-compat-util.h | git apply --check --reverse                                                       
error: patch failed: git-compat-util.h:118                                                                     
error: git-compat-util.h: patch does not apply                                                               

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 d473e2e0e8 (diff.c: convert -U|--unified, 2019-01-27) and 16ed6c97cc (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:

1/4 diff: reject negative values for --inter-hunk-context
Change type to unsigned int, switch to OPT_UNSIGNED.

2/4 diff: reject negative values for -U/--unified
Change type to unsigned int, add range check in callback.

3/4 xdiff: guard against negative context lengths
BUG() in xdl_get_hunk() as defense in depth.

4/4 parse-options: clarify PARSE_OPT_NONEG does not reject
negative numbers
Documentation fix.

The config variables diff.context and diff.interHunkContext have always rejected negative values. This series brings the CLI options in line.

Michael Montalbo (4):
diff: reject negative values for --inter-hunk-context
diff: reject negative values for -U/--unified
xdiff: guard against negative context lengths
parse-options: clarify PARSE_OPT_NONEG does not reject negative
numbers

 diff.c                             | 25 ++++++++++++++-----------
 diff.h                             |  4 ++--
 parse-options.h                    |  5 ++++-
 t/t4032-diff-inter-hunk-context.sh |  6 ++++++
 t/t4055-diff-context.sh            |  5 +++++
 xdiff/xemit.c                      | 16 ++++++++++++----
 6 files changed, 43 insertions(+), 18 deletions(-)

base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2105%2Fmmontalbo%2Fmm%2Freject-negative-interhunk-context-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2105/mmontalbo/mm/reject-negative-interhunk-context-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2105

gitgitgadget

From: Michael Montalbo <[email protected]>

Negative values for --inter-hunk-context produce structurally invalid diff output with overlapping hunks:

    $ git log -1 -p -U3 --inter-hunk-context=-100 791aeddfa2 \
        -- 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.

---
 diff.c                             | 13 ++++++-------
 diff.h                             |  2 +-
 t/t4032-diff-inter-hunk-context.sh |  6 ++++++
 3 files changed, 13 insertions(+), 8 deletions(-)

index 397e38b41c..5df28e49c5 100644
@@ -61,7 +61,7 @@ static enum git_colorbool diff_use_color_default = GIT_COLOR_UNKNOWN;
 static int diff_color_moved_default;
 static int diff_color_moved_ws_default;
 static int diff_context_default = 3;
-static int diff_interhunk_context_default;
+static unsigned int diff_interhunk_context_default;
 static char *diff_word_regex_cfg;
 static struct external_diff external_diff_cfg;
 static char *diff_order_file_cfg;

@@ -388,10 +388,10 @@ int git_diff_ui_config(const char *var, const char *value,
 		return 0;
 	}
 	if (!strcmp(var, "diff.interhunkcontext")) {
-		diff_interhunk_context_default = git_config_int(var, value,
-								ctx->kvi);
-		if (diff_interhunk_context_default < 0)
+		int val = git_config_int(var, value, ctx->kvi);
+		if (val < 0)
 			return -1;
+		diff_interhunk_context_default = val;
 		return 0;
 	}
 	if (!strcmp(var, "diff.renames")) {

@@ -6111,9 +6111,8 @@ struct option *add_diff_options(const struct option *opts,
 		OPT_CALLBACK_F(0, "default-prefix", options, NULL,
 			       N_("use default prefixes a/ and b/"),
 			       PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_default_prefix),
-		OPT_INTEGER_F(0, "inter-hunk-context", &options->interhunkcontext,
-			      N_("show context between diff hunks up to the specified number of lines"),
-			      PARSE_OPT_NONEG),
+		OPT_UNSIGNED(0, "inter-hunk-context", &options->interhunkcontext,
+			     N_("show context between diff hunks up to the specified number of lines")),
 		OPT_CALLBACK_F(0, "output-indicator-new",
 			       &options->output_indicators[OUTPUT_INDICATOR_NEW],
 			       N_("<char>"),

index 7eb84aadf4..033d633db4 100644
@@ -296,7 +296,7 @@ struct diff_options {
 	/* Number of context lines to generate in patch output. */
 	int context;
 
-	int interhunkcontext;
+	unsigned int interhunkcontext;
 
 	/* Affects the way detection logic for complete rewrites, renames and
 	 * copies.

index bada0cbd32..bec1676f8d 100755
@@ -114,4 +114,10 @@ test_expect_success 'diff.interHunkContext invalid' '
 	test_must_fail git diff
 '
 
+test_expect_success '--inter-hunk-context rejects negative value' '
+	test_unconfig diff.interHunkContext &&
+	test_must_fail git diff --inter-hunk-context=-1 2>err &&
+	test_grep "expects a non-negative integer" err
+'
+
 test_done

gitgitgadget

From: Michael Montalbo <[email protected]>

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.

---
 diff.c                  | 12 ++++++++----
 diff.h                  |  2 +-
 t/t4055-diff-context.sh |  5 +++++
 3 files changed, 14 insertions(+), 5 deletions(-)

index 5df28e49c5..1771b2c444 100644
@@ -60,7 +60,7 @@ static int diff_suppress_blank_empty;
 static enum git_colorbool diff_use_color_default = GIT_COLOR_UNKNOWN;
 static int diff_color_moved_default;
 static int diff_color_moved_ws_default;
-static int diff_context_default = 3;
+static unsigned int diff_context_default = 3;
 static unsigned int diff_interhunk_context_default;
 static char *diff_word_regex_cfg;
 static struct external_diff external_diff_cfg;

@@ -382,9 +382,10 @@ int git_diff_ui_config(const char *var, const char *value,
 		return 0;
 	}
 	if (!strcmp(var, "diff.context")) {
-		diff_context_default = git_config_int(var, value, ctx->kvi);
-		if (diff_context_default < 0)
+		int val = git_config_int(var, value, ctx->kvi);
+		if (val < 0)
 			return -1;
+		diff_context_default = val;
 		return 0;
 	}
 	if (!strcmp(var, "diff.interhunkcontext")) {

@@ -5924,9 +5925,12 @@ static int diff_opt_unified(const struct option *opt,
 	BUG_ON_OPT_NEG(unset);
 
 	if (arg) {
-		options->context = strtol(arg, &s, 10);
+		long val = strtol(arg, &s, 10);
 		if (*s)
 			return error(_("%s expects a numerical value"), "--unified");
+		if (val < 0)
+			return error(_("%s expects a non-negative integer"), "--unified");
+		options->context = val;
 	}
 	enable_patch_output(&options->output_format);
 

index 033d633db4..bb5cddaf34 100644
@@ -294,7 +294,7 @@ struct diff_options {
 	enum git_colorbool use_color;
 
 	/* Number of context lines to generate in patch output. */
-	int context;
+	unsigned int context;
 
 	unsigned int interhunkcontext;
 

index 1384a81957..b26f6eea7c 100755
@@ -82,6 +82,11 @@ test_expect_success 'negative integer config parsing' '
 	test_grep "bad config variable" output
 '
 
+test_expect_success '-U-1 is rejected' '
+	test_must_fail git diff -U-1 2>err &&
+	test_grep "expects a non-negative integer" err
+'
+
 test_expect_success '-U0 is valid, so is diff.context=0' '
 	test_config diff.context 0 &&
 	git diff >output &&

gitgitgadget

From: Michael Montalbo <[email protected]>

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.

---
 xdiff/xemit.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

index 04f7e9193b..7cd9cf0a44 100644
@@ -46,12 +46,20 @@ static long saturating_add(long a, long b)
 xdchange_t *xdl_get_hunk(xdchange_t **xscr, xdemitconf_t const *xecfg)
 {
 	xdchange_t *xch, *xchp, *lxch;
-	long max_common = saturating_add(saturating_add(xecfg->ctxlen,
-							xecfg->ctxlen),
-					 xecfg->interhunkctxlen);
-	long max_ignorable = xecfg->ctxlen;
+	long max_common;
+	long max_ignorable;
 	long ignored = 0; /* number of ignored blank lines */
 
+	if (xecfg->ctxlen < 0)
+		BUG("negative context length: %ld", xecfg->ctxlen);
+	if (xecfg->interhunkctxlen < 0)
+		BUG("negative inter-hunk context length: %ld", xecfg->interhunkctxlen);
+
+	max_common = saturating_add(saturating_add(xecfg->ctxlen,
+						   xecfg->ctxlen),
+				    xecfg->interhunkctxlen);
+	max_ignorable = xecfg->ctxlen;
+
 	/* remove ignorable changes that are too far before other changes */
 	for (xchp = *xscr; xchp && xchp->ignore; xchp = xchp->next) {
 		xch = xchp->next;

gitgitgadget

From: Michael Montalbo <[email protected]>

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 d473e2e0e8 and 16ed6c97cc, 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.

---
 parse-options.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index 706de9729f..c0a3a3dcae 100644
@@ -116,7 +116,10 @@ typedef int parse_opt_subcommand_fn(int argc, const char **argv,
  *   mask of parse_opt_option_flags.
  *   PARSE_OPT_OPTARG: says that the argument is optional (not for BOOLEANs)
  *   PARSE_OPT_NOARG: says that this option does not take an argument
- *   PARSE_OPT_NONEG: says that this option cannot be negated
+ *   PARSE_OPT_NONEG: says that this option cannot be negated (i.e.
+ *                   prevents --no-<option> boolean form). Does not reject
+ *                   negative numeric values like --option=-1. Use
+ *                   OPT_UNSIGNED for options that must be non-negative.
  *   PARSE_OPT_HIDDEN: this option is skipped in the default usage, and
  *                     shown only in the full usage.
  *   PARSE_OPT_LASTARG_DEFAULT: says that this option will take the default

gitgitgadget

I do not think _NONEG has anything to do with the bug. It was purely to reject --no-unified and --no-inter-hunk-context.

And there was no change to remove PARSE_OPT_NONEG from anywhere and use OPT_UNSIGNED instead to fix any of the bugs fixed in this series, ...

It may not matter in the cover letter, but why do you need ~60 whitespace characters at the end of the command line, and many other lines in the message?

You are right this was a mistaken assumption on my part.

Right, PARSE_OPT_NONEG is still enabled implicitly by OPT_UNSIGNED so PARSE_OPT_NONEG really has no part in the story.

Ok, will remove the latter portion of the change in a follow-up.

Apologies, this is a mistake I made between formatting my cover letter and moving
it to GitGitGadget. I will clean up the cover letter and be more careful about that in
the future. Thank you for pointing this out.

Negative values for -U and --inter-hunk-context are silently accepted and produce structurally invalid diff output.

Malformed hunk headers:

$ wc -l GIT-VERSION-GEN 106 $ 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.

Overlapping hunks that cannot be applied:

$ git log -1 -p -U3 --inter-hunk-context=100 791aeddfa2
-- git-compat-util.h | git apply --check --reverse (success)

$ git log -1 -p -U3 --inter-hunk-context=-100 791aeddfa2
-- git-compat-util.h | git apply --check --reverse error: patch failed:
git-compat-util.h:118 error: git-compat-util.h: patch does not apply

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 d473e2e0e8 (diff.c: convert -U|--unified, 2019-01-27) and 16ed6c97cc (diff-parseopt: convert --inter-hunk-context, 2019-03-24), the implicit rejection was lost.

This series restores the original invariant with stronger guarantees:

1/4 diff: reject negative values for --inter-hunk-context Change type to unsigned int, switch to OPT_UNSIGNED.

2/4 diff: reject negative values for -U/--unified Change type to unsigned int, add range check in callback.

3/4 xdiff: guard against negative context lengths BUG() in xdl_get_hunk() as defense in depth.

4/4 parse-options: clarify what "negated" means for PARSE_OPT_NONEG.

The config variables diff.context and diff.interHunkContext have always rejected negative values. This series brings the CLI options in line.

Changes since v1:

Patch 1 and 4: Rewrote commit message to not imply NONEG was related to the bug.

Patch 4: Trimmed to just clarify what "negated" means, without documenting what PARSE_OPT_NONEG does not do.

Michael Montalbo (4):
diff: reject negative values for --inter-hunk-context
diff: reject negative values for -U/--unified
xdiff: guard against negative context lengths
parse-options: clarify what "negated" means for PARSE_OPT_NONEG

 diff.c                             | 25 ++++++++++++++-----------
 diff.h                             |  4 ++--
 parse-options.h                    |  1 +
 t/t4032-diff-inter-hunk-context.sh |  6 ++++++
 t/t4055-diff-context.sh            |  5 +++++
 xdiff/xemit.c                      | 16 ++++++++++++----
 6 files changed, 40 insertions(+), 17 deletions(-)

base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2105%2Fmmontalbo%2Fmm%2Freject-negative-interhunk-context-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2105/mmontalbo/mm/reject-negative-interhunk-context-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2105

Range-diff vs v1:

1: cca75eca0e ! 1: f2ebb3a72b diff: reject negative values for --inter-hunk-context
@@ Commit message
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".
+ values, but the command line option does not.

Change the type of diff_options.interhunkcontext and its static default from int to unsigned int, and switch the option parser from

 2:  f0478d434c = 2:  fc3d2bc31e diff: reject negative values for -U/--unified
 3:  f9cfa0c55d = 3:  020ca774c0 xdiff: guard against negative context lengths
 4:  05ff821e6f ! 4:  3a656f8c0f parse-options: clarify PARSE_OPT_NONEG does not reject negative numbers

@@ Metadata
Author: Michael Montalbo <[email protected]>

## Commit message ##
- parse-options: clarify PARSE_OPT_NONEG does not reject negative numbers
+ parse-options: clarify what "negated" means for PARSE_OPT_NONEG

- 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 d473e2e0e8
- and 16ed6c97cc, 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.
+ The documentation says the flag prevents an option from being
+ "negated" without specifying what that means. Add a parenthetical
+ to clarify that it rejects the "--no-<option>" form.

## parse-options.h ##
@@ parse-options.h: typedef int parse_opt_subcommand_fn(int argc, const char **argv,
- * mask of parse_opt_option_flags.
* PARSE_OPT_OPTARG: says that the argument is optional (not for BOOLEANs)
* PARSE_OPT_NOARG: says that this option does not take an argument
-- * PARSE_OPT_NONEG: says that this option cannot be negated
-+ * PARSE_OPT_NONEG: says that this option cannot be negated (i.e.
-+ * prevents --no-<option> boolean form). Does not reject
-+ * negative numeric values like --option=-1. Use
-+ * OPT_UNSIGNED for options that must be non-negative.
+ * PARSE_OPT_NONEG: says that this option cannot be negated
++ * (i.e. rejects "--no-<option>")
* PARSE_OPT_HIDDEN: this option is skipped in the default usage, and
* shown only in the full usage.
* PARSE_OPT_LASTARG_DEFAULT: says that this option will take the default

gitgitgadget

From: Michael Montalbo <[email protected]>

Negative values for --inter-hunk-context produce structurally invalid diff output with overlapping hunks:

    $ git log -1 -p -U3 --inter-hunk-context=-100 791aeddfa2 \
        -- 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.

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.

---
 diff.c                             | 13 ++++++-------
 diff.h                             |  2 +-
 t/t4032-diff-inter-hunk-context.sh |  6 ++++++
 3 files changed, 13 insertions(+), 8 deletions(-)

index 397e38b41c..5df28e49c5 100644
@@ -61,7 +61,7 @@ static enum git_colorbool diff_use_color_default = GIT_COLOR_UNKNOWN;
 static int diff_color_moved_default;
 static int diff_color_moved_ws_default;
 static int diff_context_default = 3;
-static int diff_interhunk_context_default;
+static unsigned int diff_interhunk_context_default;
 static char *diff_word_regex_cfg;
 static struct external_diff external_diff_cfg;
 static char *diff_order_file_cfg;

@@ -388,10 +388,10 @@ int git_diff_ui_config(const char *var, const char *value,
 		return 0;
 	}
 	if (!strcmp(var, "diff.interhunkcontext")) {
-		diff_interhunk_context_default = git_config_int(var, value,
-								ctx->kvi);
-		if (diff_interhunk_context_default < 0)
+		int val = git_config_int(var, value, ctx->kvi);
+		if (val < 0)
 			return -1;
+		diff_interhunk_context_default = val;
 		return 0;
 	}
 	if (!strcmp(var, "diff.renames")) {

@@ -6111,9 +6111,8 @@ struct option *add_diff_options(const struct option *opts,
 		OPT_CALLBACK_F(0, "default-prefix", options, NULL,
 			       N_("use default prefixes a/ and b/"),
 			       PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_default_prefix),
-		OPT_INTEGER_F(0, "inter-hunk-context", &options->interhunkcontext,
-			      N_("show context between diff hunks up to the specified number of lines"),
-			      PARSE_OPT_NONEG),
+		OPT_UNSIGNED(0, "inter-hunk-context", &options->interhunkcontext,
+			     N_("show context between diff hunks up to the specified number of lines")),
 		OPT_CALLBACK_F(0, "output-indicator-new",
 			       &options->output_indicators[OUTPUT_INDICATOR_NEW],
 			       N_("<char>"),

index 7eb84aadf4..033d633db4 100644
@@ -296,7 +296,7 @@ struct diff_options {
 	/* Number of context lines to generate in patch output. */
 	int context;
 
-	int interhunkcontext;
+	unsigned int interhunkcontext;
 
 	/* Affects the way detection logic for complete rewrites, renames and
 	 * copies.

index bada0cbd32..bec1676f8d 100755
@@ -114,4 +114,10 @@ test_expect_success 'diff.interHunkContext invalid' '
 	test_must_fail git diff
 '
 
+test_expect_success '--inter-hunk-context rejects negative value' '
+	test_unconfig diff.interHunkContext &&
+	test_must_fail git diff --inter-hunk-context=-1 2>err &&
+	test_grep "expects a non-negative integer" err
+'
+
 test_done

gitgitgadget

From: Michael Montalbo <[email protected]>

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.

---
 diff.c                  | 12 ++++++++----
 diff.h                  |  2 +-
 t/t4055-diff-context.sh |  5 +++++
 3 files changed, 14 insertions(+), 5 deletions(-)

index 5df28e49c5..1771b2c444 100644
@@ -60,7 +60,7 @@ static int diff_suppress_blank_empty;
 static enum git_colorbool diff_use_color_default = GIT_COLOR_UNKNOWN;
 static int diff_color_moved_default;
 static int diff_color_moved_ws_default;
-static int diff_context_default = 3;
+static unsigned int diff_context_default = 3;
 static unsigned int diff_interhunk_context_default;
 static char *diff_word_regex_cfg;
 static struct external_diff external_diff_cfg;

@@ -382,9 +382,10 @@ int git_diff_ui_config(const char *var, const char *value,
 		return 0;
 	}
 	if (!strcmp(var, "diff.context")) {
-		diff_context_default = git_config_int(var, value, ctx->kvi);
-		if (diff_context_default < 0)
+		int val = git_config_int(var, value, ctx->kvi);
+		if (val < 0)
 			return -1;
+		diff_context_default = val;
 		return 0;
 	}
 	if (!strcmp(var, "diff.interhunkcontext")) {

@@ -5924,9 +5925,12 @@ static int diff_opt_unified(const struct option *opt,
 	BUG_ON_OPT_NEG(unset);
 
 	if (arg) {
-		options->context = strtol(arg, &s, 10);
+		long val = strtol(arg, &s, 10);
 		if (*s)
 			return error(_("%s expects a numerical value"), "--unified");
+		if (val < 0)
+			return error(_("%s expects a non-negative integer"), "--unified");
+		options->context = val;
 	}
 	enable_patch_output(&options->output_format);
 

index 033d633db4..bb5cddaf34 100644
@@ -294,7 +294,7 @@ struct diff_options {
 	enum git_colorbool use_color;
 
 	/* Number of context lines to generate in patch output. */
-	int context;
+	unsigned int context;
 
 	unsigned int interhunkcontext;
 

index 1384a81957..b26f6eea7c 100755
@@ -82,6 +82,11 @@ test_expect_success 'negative integer config parsing' '
 	test_grep "bad config variable" output
 '
 
+test_expect_success '-U-1 is rejected' '
+	test_must_fail git diff -U-1 2>err &&
+	test_grep "expects a non-negative integer" err
+'
+
 test_expect_success '-U0 is valid, so is diff.context=0' '
 	test_config diff.context 0 &&
 	git diff >output &&

gitgitgadget

From: Michael Montalbo <[email protected]>

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.

---
 xdiff/xemit.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

index 04f7e9193b..7cd9cf0a44 100644
@@ -46,12 +46,20 @@ static long saturating_add(long a, long b)
 xdchange_t *xdl_get_hunk(xdchange_t **xscr, xdemitconf_t const *xecfg)
 {
 	xdchange_t *xch, *xchp, *lxch;
-	long max_common = saturating_add(saturating_add(xecfg->ctxlen,
-							xecfg->ctxlen),
-					 xecfg->interhunkctxlen);
-	long max_ignorable = xecfg->ctxlen;
+	long max_common;
+	long max_ignorable;
 	long ignored = 0; /* number of ignored blank lines */
 
+	if (xecfg->ctxlen < 0)
+		BUG("negative context length: %ld", xecfg->ctxlen);
+	if (xecfg->interhunkctxlen < 0)
+		BUG("negative inter-hunk context length: %ld", xecfg->interhunkctxlen);
+
+	max_common = saturating_add(saturating_add(xecfg->ctxlen,
+						   xecfg->ctxlen),
+				    xecfg->interhunkctxlen);
+	max_ignorable = xecfg->ctxlen;
+
 	/* remove ignorable changes that are too far before other changes */
 	for (xchp = *xscr; xchp && xchp->ignore; xchp = xchp->next) {
 		xch = xchp->next;

gitgitgadget

From: Michael Montalbo <[email protected]>

The documentation says the flag prevents an option from being "negated" without specifying what that means. Add a parenthetical to clarify that it rejects the "--no-<option>" form.

---
 parse-options.h | 1 +
 1 file changed, 1 insertion(+)

index 706de9729f..0d1f738f8d 100644
@@ -117,6 +117,7 @@ typedef int parse_opt_subcommand_fn(int argc, const char **argv,
  *   PARSE_OPT_OPTARG: says that the argument is optional (not for BOOLEANs)
  *   PARSE_OPT_NOARG: says that this option does not take an argument
  *   PARSE_OPT_NONEG: says that this option cannot be negated
+ *                   (i.e. rejects "--no-<option>")
  *   PARSE_OPT_HIDDEN: this option is skipped in the default usage, and
  *                     shown only in the full usage.
  *   PARSE_OPT_LASTARG_DEFAULT: says that this option will take the default

gitgitgadget

Thanks. Will queue. I have nothing more to add, but I will hold off on marking it for 'next' to give others a chance to comment.