[PATCH v2 0/7] use size_t for xdiff mmfile_t

[Jeff King <peff@peff.net>]

On Tue, Sep 29, 2026 at 02:49:36AM -0400, Jeff King wrote:

> An earlier series tried to simplify ll_ext_merge()'s code to read back
> the merge result from a temporary file, but Elijah pointed out some
> subtle integer overflow confusion:
> 
>   https://lore.kernel.org/git/CABPp-BG9Hkc7i_JxAbYfyzu+b4Mc_pZUr0jJF=vY0jHSARpHzw@mail.gmail.com/
> 
> I dug a little bit and found that similar problems exist elsewhere. So
> here's an attempt to make things at least incrementally better. And
> patch 4 is the original cleanup I set out to do. ;)

Here's a v2 that addresses review so far. The end state is the same
(plus the two bonus patches sent earlier), but it moves the xmallocz()
patch earlier, and fills in a few bits in the commit messages.

Range-diff is below.

  [1/7]: xdiff: clean up read_mmfile() allocations on error
  [2/7]: xdiff: replace mmbuffer_t with mmfile_t
  [3/7]: xdiff: use size_t for buffer sizes
  [4/7]: xdiff: NUL-terminate buffers read by read_mmfile()
  [5/7]: merge-ll: use read_mmfile() to read external merge results
  [6/7]: merge-ll: handle external driver status before reading result
  [7/7]: merge-ll: report an error when reading external merge results fails

 Documentation/technical/api-merge.adoc |  7 ++--
 apply.c                                |  2 +-
 builtin/checkout.c                     |  2 +-
 builtin/merge-file.c                   |  2 +-
 builtin/merge-tree.c                   |  2 +-
 builtin/rerere.c                       |  8 +++-
 diff.c                                 |  2 +-
 merge-blobs.c                          |  2 +-
 merge-ll.c                             | 39 +++++++-------------
 merge-ll.h                             |  4 +-
 merge-ort.c                            |  4 +-
 notes-merge.c                          |  2 +-
 rerere.c                               | 11 +++---
 t/t4200-rerere.sh                      | 51 ++++++++++++++++++++++++++
 xdiff-interface.c                      |  5 ++-
 xdiff/xdiff.h                          | 11 ++----
 xdiff/xmerge.c                         |  4 +-
 xdiff/xutils.c                         |  4 +-
 18 files changed, 100 insertions(+), 62 deletions(-)

1:  985905950f = 1:  985905950f xdiff: clean up read_mmfile() allocations on error
2:  ddcae336eb = 2:  ddcae336eb xdiff: replace mmbuffer_t with mmfile_t
3:  36d932e0ee = 3:  36d932e0ee xdiff: use size_t for buffer sizes
5:  f25902e825 ! 4:  c9cd3c7c3c xdiff: NUL-terminate buffers read by read_mmfile()
    @@ Commit message
         I don't know of any path that would benefit from this, but I noticed it
         while converting ll_ext_merge() to use read_mmfile(), since its original
         code did add a NUL byte (even though I cannot find any case where it
    -    would have mattered). Let's teach read_mmfile() to add this defensive
    -    NUL; it probably doesn't help anything, but nor should it hurt.
    +    would have mattered). Let's add the same defensive NUL in read_mmfile()
    +    by using xmallocz() instead of xmalloc().
     
         Note that the matching read_mmblob() doesn't need the same treatment.
         Its buffers already have a NUL from the object-reading code (which uses
4:  6ac0d54bda ! 5:  86fa283e00 merge-ll: use read_mmfile() to read external merge results
    @@ Commit message
         back from a temporary file. We can do the same thing with much less code
         by using read_mmfile().
     
    -    As a bonus, note that read_mmfile() correctly uses xsize_t() to detect
    -    the case when we'd truncate the result.
    +    There are also two behavior improvements.
    +
    +    One, read_mmfile() correctly uses xsize_t() to detect the case when we'd
    +    truncate the result.
    +
    +    And two, read_mmfile() will report errors to stderr if it can't read the
    +    file (whereas the existing code silently returned NULL). I think most
    +    callers would have said _something_ in this case like "failed to execute
    +    merge" (from merge-ort), but more specifics are probably helpful (e.g.,
    +    to distinguish a random system error from a badly configured merge
    +    driver).
     
         Signed-off-by: Jeff King <peff@peff.net>
     
6:  3e5f090284 = 6:  8ad0b774bf merge-ll: handle external driver status before reading result
7:  30357e6e9a = 7:  896031317b merge-ll: report an error when reading external merge results fails