GNU bug report logs - #76613
diff -y crashes with apparent memory corruption

Previous Next

Full log

Message #11 received at 76613 <at> debbugs.gnu.org (full text, mbox):

From: Collin Funk <collin.funk1 <at> gmail.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 76613 <at> debbugs.gnu.org, Nick Smallbone <nick <at> smallbone.se>
Subject: Re: [bug-diffutils] bug#76613: bug#76613: diff -y crashes with
 apparent memory corruption
Date: Thu, 27 Feb 2025 20:35:57 -0800

[Message part 1 (text/plain, inline)]

Hi Paul,

Paul Eggert <eggert <at> cs.ucla.edu> writes:

> Thanks for the bug report. I can reproduce it with gcc
> -fsanitize=address on Ubuntu 24.10 x86-64. I plan to take a look at it
> soon.

I see -fsanitize=address and valgrind fail this test starting at this
commit f54e901c329ba7b7d98ecae2571712f43444c2bd:

    maint: use xpalloc    
    * bootstrap.conf (gnulib_modules): Add ialloc, to document
    the now-direct dependency.
    * src/diff.c (add_regexp):
    * src/diff3.c (read_diff):
    * src/dir.c (dir_read):
    * src/io.c (slurp, find_and_hash_each_line, find_identical_ends):
    * src/sdiff.c (diffarg):
    Prefer xpalloc to doing it by hand.
    * src/io.c: Include ialloc.h, for irealloc.
    (equivs_alloc): Now idx_t, not lin, for xpalloc.
    (sip): Don’t bother subtracting 2 * sizeof (word) from the
    buffer_lcm upper bound, as later code works anyway now.
    (slurp): Simplify buffer allocation so that xpalloc can be used.
    Use irealloc for speculative reallocation, since the code could
    work anyway if the irealloc fails.  Use current->eof to check
    for EOF, rather than the less-intuitive buffer size checks.

The previous commit passes it. Here are the relevant lines:

@@ -419,17 +411,16 @@ find_and_hash_each_line (struct file_data *current)
       /* Maybe increase the size of the line table.  */
       if (line == alloc_lines)
         {
-          /* Double (alloc_lines - linbuf_base) by adding to alloc_lines.  */
-          if (IDX_MAX / 3 <= alloc_lines
-              || IDX_MAX / sizeof *cureqs <= 2 * alloc_lines - linbuf_base
-              || IDX_MAX / sizeof *linbuf <= alloc_lines - linbuf_base)
-            xalloc_die ();
-          alloc_lines = 2 * alloc_lines - linbuf_base;
-          cureqs = xirealloc (cureqs, alloc_lines * sizeof *cureqs);
+         idx_t eqs_max = MIN (LIN_MAX, IDX_MAX / sizeof *cureqs);
+
+         /* Grow (alloc_lines - linbuf_base) by adding to alloc_lines.  */
+         idx_t n = alloc_lines - linbuf_base;
           linbuf += linbuf_base;
-          linbuf = xirealloc (linbuf,
-                             (alloc_lines - linbuf_base) * sizeof *linbuf);
+         linbuf = xpalloc (linbuf, &n, 1, eqs_max - linbuf_base,
+                           sizeof *linbuf);
           linbuf -= linbuf_base;
+         alloc_lines = linbuf_base + n;
+          cureqs = xirealloc (cureqs, alloc_lines * sizeof *cureqs);
         }
       linbuf[line] = ip;
       cureqs[line] = i;
@@ -445,16 +436,13 @@ find_and_hash_each_line (struct file_data *current)
          so that we can compute the length of any buffered line.  */
       if (line == alloc_lines)
         {
-          /* Double (alloc_lines - linbuf_base) by adding to alloc_lines.  */
-          if (IDX_MAX / 3 <= alloc_lines
-              || IDX_MAX / sizeof *cureqs <= 2 * alloc_lines - linbuf_base
-              || IDX_MAX / sizeof *linbuf <= alloc_lines - linbuf_base)
-            xalloc_die ();
-          alloc_lines = 2 * alloc_lines - linbuf_base;
-          linbuf += linbuf_base;
-          linbuf = xirealloc (linbuf,
-                             (alloc_lines - linbuf_base) * sizeof *linbuf);
-          linbuf -= linbuf_base;
+         /* Grow (alloc_lines - linbuf_base) by adding to alloc_lines.  */
+         idx_t n = alloc_lines - linbuf_base;
+         linbuf += linbuf_base;
+         linbuf = xpalloc (linbuf, &n, 1, MAX (0, IDX_MAX - linbuf_base),
+                           sizeof *linbuf);
+         linbuf -= linbuf_base;
+         alloc_lines = n - linbuf_base;
         }
       linbuf[line] = p;

In the original version alloc_lines is calculated as
2 * alloc_lines - linbuf_base in both hunks. Afterwards it is
linbuf_base + n in one section and n - linbuf_base in the other.

I've attached a patch that satisfies sanitizers, but maybe I am missing
something in this code...

Collin

[0001-diff-fix-allocation-size-computation-that-could-caus.patch (text/x-patch, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.