[PATCH v2 0/4] diff: reject negative context values

["Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com>]

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 <mmontalbo@gmail.com>
      
       ## 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.
      
          Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
      
       ## 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