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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 76613 in the body.
You can then email your comments to 76613 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: "Nick Smallbone" <nick <at> smallbone.se>
To: bug-diffutils <at> gnu.org
Subject: diff -y crashes with apparent memory corruption
Date: Thu, 27 Feb 2025 11:04:03 +0100

Hi,

I'm running diffutils-3.11, downloaded from ftp.gnu.org and built with ./configure && make (no options given).

I'm seeing the problem that diff -y is crashing with various malloc-related errors. Here is an example. First I create two files a and b like so:

% seq 1 100 > a
% seq 1 100 | grep -v 50 > b

Then I run diff -y a b, which crashes with an error in free():

% diff -y a b
free(): corrupted unsorted chunks
zsh: IOT instruction  src/diff -y ~/a ~/b

I haven't looked into the source to find out the problem, but I did compile a debug build and run it under Valgrind. It detected some memory corruption - here is the report:

==9602== Memcheck, a memory error detector
==9602== Copyright (C) 2002-2024, and GNU GPL'd, by Julian Seward et al.
==9602== Using Valgrind-3.24.0 and LibVEX; rerun with -h for copyright info
==9602== Command: src/diff -y /home/nick/a /home/nick/b
==9602== 
==9602== Invalid write of size 8
==9602==    at 0x40EC8A: find_and_hash_each_line (io.c:1017)
==9602==    by 0x40FBAA: read_files (io.c:1366)
==9602==    by 0x40596C: diff_2_files (analyze.c:463)
==9602==    by 0x409B1F: compare_prepped_files (diff.c:1371)
==9602==    by 0x40ADBF: compare_files (diff.c:1633)
==9602==    by 0x408834: main (diff.c:881)
==9602==  Address 0x4b12f80 is 0 bytes after a block of size 656 alloc'd
==9602==    at 0x4850C7C: realloc (vg_replace_malloc.c:1801)
==9602==    by 0x41A8A6: rpl_realloc (stdlib.h:2066)
==9602==    by 0x41CE27: xrealloc (xmalloc.c:66)
==9602==    by 0x41D196: xpalloc (xmalloc.c:271)
==9602==    by 0x40EC4A: find_and_hash_each_line (io.c:1013)
==9602==    by 0x40FBAA: read_files (io.c:1366)
==9602==    by 0x40596C: diff_2_files (analyze.c:463)
==9602==    by 0x409B1F: compare_prepped_files (diff.c:1371)
==9602==    by 0x40ADBF: compare_files (diff.c:1633)
==9602==    by 0x408834: main (diff.c:881)
==9602== 
--9602-- VALGRIND INTERNAL ERROR: Valgrind received a signal 11 (SIGSEGV) - exiting
--9602-- si_code=1;  Faulting address: 0x9622BA0;  sp: 0x1002cf6e20

valgrind: the 'impossible' happened:
   Killed by fatal signal

host stacktrace:
==9602==    at 0x5804AE1F: ??? (in /usr/libexec/valgrind/memcheck-amd64-linux)
==9602==    by 0x58004E0C: ??? (in /usr/libexec/valgrind/memcheck-amd64-linux)
==9602==    by 0x58005203: ??? (in /usr/libexec/valgrind/memcheck-amd64-linux)
==9602==    by 0x58097E37: ??? (in /usr/libexec/valgrind/memcheck-amd64-linux)
==9602==    by 0x580E1E1A: ??? (in /usr/libexec/valgrind/memcheck-amd64-linux)

sched status:
  running_tid=1

Thread 1: status = VgTs_Runnable (lwpid 9602)
==9602==    at 0x4850A5F: calloc (vg_replace_malloc.c:1675)
==9602==    by 0x4160B0: icalloc (ialloc.h:91)
==9602==    by 0x41D239: xicalloc (xmalloc.c:304)
==9602==    by 0x41D1E7: xizalloc (xmalloc.c:289)
==9602==    by 0x405E39: diff_2_files (analyze.c:529)
==9602==    by 0x409B1F: compare_prepped_files (diff.c:1371)
==9602==    by 0x40ADBF: compare_files (diff.c:1633)
==9602==    by 0x408834: main (diff.c:881)
client stack range: [0x1FFEFFD000 0x1FFF000FFF] client SP: 0x1FFEFFEDA0
valgrind stack range: [0x1002BF7000 0x1002CF6FFF] top usage: 7272 of 1048576

Nick

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Nick Smallbone <nick <at> smallbone.se>
Cc: 76613 <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#76613: diff -y crashes with apparent memory
 corruption
Date: Thu, 27 Feb 2025 11:13:06 -0800

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.

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)]

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Collin Funk <collin.funk1 <at> gmail.com>
Cc: 76613-done <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: Fri, 28 Feb 2025 23:16:07 -0800

On 2025-02-27 20:35, Collin Funk wrote:
> I've attached a patch that satisfies sanitizers

Thanks, that looks good, and I installed that one-line change along with 
a NEWS file notice and a test case. And thanks to Nick for reporting 
this. Closing the bug report.

Previous Next

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