GNU bug report logs - #77315
31.0.50; Crash in Finsert_file_contents, file size changed

Previous Next

To reply to this bug, email your comments to 77315 AT debbugs.gnu.org.

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

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

From: Pip Cet <pipcet <at> protonmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 31.0.50; Crash in Finsert_file_contents, file size changed
Date: Thu, 27 Mar 2025 15:34:31 +0000

I experienced a crash on the feature/igc branch (unfortunately, with
local modifications) in a long-running session.  I still have the (very
large) coredump file so I can do some limited post-mortem debugging of
the session, but I think I've identified the bug and we can just fix it
instead.

Note that line numbers will probably be off in the backtrace because of
the local modifications.

The crash happened when a buffer showing config.log in an Emacs
directory auto-reverted while I was running ./configure in that
directory in another shell buffer.

After looking at this more closely, I think it's a bug in
Finsert_file_contents, which currently assumes that a call to
emacs_fd_fstat establishes st_size as an upper limit of how many
characters read from the beginning of the file can match the current
buffer.

Most likely, the race window is so small that this happens only very
rarely on the master branch, but feature/igc introduces the occasional
unexpected delay so is more vulnerable to such bugs (see also
bug#74590, which also is most likely explained by a delay introduced by
MPS exposing a previously-latent bug).  However, I believe this isn't
likely to be an MPS-specific bug.

The crash happened in this line/call in fileio.c:

	  if (overlap > 0)
	    same_at_end += overlap;
	  same_at_end_charpos = BYTE_TO_CHAR (same_at_end); <<<<<< HERE

	  /* Arrange to read only the nonmatching middle part of the file.  */
	  beg_offset += same_at_start - BEGV_BYTE;
	  end_offset -= ZV_BYTE - same_at_end;

At the time:

        same_at_start = 38706
        same_at_end = 1499444
        overlap = 25
        replace_handled = false
        giveup_match_end = false
        regular = true
        coding = {

        st = {
          st_size = 38681,
        }

        current_buffer->text->z_byte = 1499420
        end = Qnil
        current_buffer->text->gpt = 1399068
        current_buffer->text->beg + 38681 = "configure:10959: $? = 0\nconfigure:10996: result: none needed\n..."

        coding = {
          id = 1,
          result = CODING_RESULT_SUCCESS,
          decoder = 0x5555556c7f6c <decode_coding_raw_text>,
          encoder = 0x5555556c80c1 <encode_coding_raw_text>
        }

So the BYTE_TO_CHAR failed because same_at_end was out of range after
overlap was added to it.  Given the calculation of overlap as:

	  overlap = (same_at_start - BEGV_BYTE
		     - (same_at_end
			+ (! NILP (end) ? end_offset : st.st_size) - ZV_BYTE));

I believe the intention was for same_at_start - BEGV_BYTE never to
exceed st.st_size or end_offset (the two are the same here).  See below
for why I think that the overlap calculation should also subtract
beg_offset from this value.

same_at_start is set here:

	  int nread = emacs_fd_read (fd, read_buf, sizeof read_buf);
	  int bufpos = 0;
	  while (bufpos < nread && same_at_start < ZV_BYTE
		 && FETCH_BYTE (same_at_start) == read_buf[bufpos])
	    same_at_start++, bufpos++;

which is called some time after:

  if (emacs_fd_fstat (fd, &st) != 0)
    report_file_error ("Input file status", orig_filename);

so it seems entirely possible to me that the file grew by the time we
set same_at_start, leading to the backtrace.

The extra text written appears to have been

    "configure:10959: $? = 0\n"

My impression is that overlap should have been calculated as 0 (or a
negative number), not 25, because only one byte matches at the end of
the buffer (which ends in "s\n") and same_at_end was thus, rightfully,
1494419, one less than Z_BYTE = ZV_BYTE.

This code is very hard to understand; I believe that's because it
doesn't currently always do the right thing, particularly not if st_size
changes.  However, a preliminary suggestion for fixing this follows:

Let's add an additional condition to the loop counting bytes for
same_at_start, so it never exceeds end_offset - beg_offset + BEGV_BYTE,
and a symmetric condition in the loop counting bytes for same_at_end, so
it never becomes less than ZV_BYTE - (end_offset - beg_offset -
same_at_start + BEGV_BYTE).

This would fix both this bug and what seems to me to be a faulty
calculation, failing to detect an overlap, in the case where beg_offset
is != 0.

IOW, we would fix the overlap calculation and make it so no overlap can
ever happen, after which the overlap code can be removed.

In addition, this code:

      /* If the file matches the buffer completely,
	 there's no need to replace anything.  */
      if (same_at_start - BEGV_BYTE == end_offset - beg_offset)
	{
	  emacs_fd_close (fd);
	  clear_unwind_protect (fd_index);

	  /* Truncate the buffer to the size of the file.  */
	  del_range_1 (same_at_start, same_at_end, 0, 0);
	  goto handled;
	}

confuses me, because same_at_start and same_at_end are byte positions
and del_range_1 takes character positions.  I expect it's hit very
rarely except when same_at_start == same_at_end or same_at_start ==
BEGV_BYTE == BEGV, and del_range_1 is forgiving in these cases.

Backtrace:

#0  terminate_due_to_signal (sig=6, backtrace_limit=2147483647) at emacs.c:425
#1  0x000055555587f71b in die (msg=0x555555a597b8 "BUF_BEG_BYTE (b) <= bytepos && bytepos <= BUF_Z_BYTE (b)", file=0x555555a5974c "marker.c", line=330)
    at alloc.c:8064
#2  0x000055555581e011 in buf_bytepos_to_charpos (b=0x7ff8ef791a58, bytepos=1499444) at marker.c:330
#3  0x0000555555829246 in BYTE_TO_CHAR (bytepos=1499444) at /home/pip/emacs-20250320/src/buffer.h:1204
#4  0x0000555555833950 in Finsert_file_contents (filename=0x7ff8ef797af5, visit=0x38, beg=0x0, end=0x0, replace=0xc908) at fileio.c:4547

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> protonmail.com>, Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 77315 <at> debbugs.gnu.org
Subject: Re: bug#77315: 31.0.50;
 Crash in Finsert_file_contents, file size changed
Date: Thu, 27 Mar 2025 18:21:31 +0200

> Date: Thu, 27 Mar 2025 15:34:31 +0000
> From:  Pip Cet via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> I experienced a crash on the feature/igc branch (unfortunately, with
> local modifications) in a long-running session.  I still have the (very
> large) coredump file so I can do some limited post-mortem debugging of
> the session, but I think I've identified the bug and we can just fix it
> instead.
> 
> Note that line numbers will probably be off in the backtrace because of
> the local modifications.
> 
> The crash happened when a buffer showing config.log in an Emacs
> directory auto-reverted while I was running ./configure in that
> directory in another shell buffer.
> 
> After looking at this more closely, I think it's a bug in
> Finsert_file_contents, which currently assumes that a call to
> emacs_fd_fstat establishes st_size as an upper limit of how many
> characters read from the beginning of the file can match the current
> buffer.
> 
> Most likely, the race window is so small that this happens only very
> rarely on the master branch, but feature/igc introduces the occasional
> unexpected delay so is more vulnerable to such bugs (see also
> bug#74590, which also is most likely explained by a delay introduced by
> MPS exposing a previously-latent bug).  However, I believe this isn't
> likely to be an MPS-specific bug.

Adding Paul to the discussion.

AFAIR, insert-file-contents needs to be completely rewritten to handle
such situations.  If the file is being modified by another program
while we are trying to figure out the overlap (because REPLACE was
no-nil), then I'm not sure I understand how can REPLACE and overlap
work at all, since the data changes under our feet as we go.  So maybe
if we detect that this is happening, we should give up on being smart
and decide that there's no overlap at all.

> The crash happened in this line/call in fileio.c:
> 
> 	  if (overlap > 0)
> 	    same_at_end += overlap;
> 	  same_at_end_charpos = BYTE_TO_CHAR (same_at_end); <<<<<< HERE
> 
> 	  /* Arrange to read only the nonmatching middle part of the file.  */
> 	  beg_offset += same_at_start - BEGV_BYTE;
> 	  end_offset -= ZV_BYTE - same_at_end;
> 
> At the time:
> 
>         same_at_start = 38706
>         same_at_end = 1499444
>         overlap = 25
>         replace_handled = false
>         giveup_match_end = false
>         regular = true
>         coding = {
> 
>         st = {
>           st_size = 38681,
>         }
> 
>         current_buffer->text->z_byte = 1499420
>         end = Qnil
>         current_buffer->text->gpt = 1399068
>         current_buffer->text->beg + 38681 = "configure:10959: $? = 0\nconfigure:10996: result: none needed\n..."
> 
>         coding = {
>           id = 1,
>           result = CODING_RESULT_SUCCESS,
>           decoder = 0x5555556c7f6c <decode_coding_raw_text>,
>           encoder = 0x5555556c80c1 <encode_coding_raw_text>
>         }
> 
> So the BYTE_TO_CHAR failed because same_at_end was out of range after
> overlap was added to it.  Given the calculation of overlap as:
> 
> 	  overlap = (same_at_start - BEGV_BYTE
> 		     - (same_at_end
> 			+ (! NILP (end) ? end_offset : st.st_size) - ZV_BYTE));
> 
> I believe the intention was for same_at_start - BEGV_BYTE never to
> exceed st.st_size or end_offset (the two are the same here).  See below
> for why I think that the overlap calculation should also subtract
> beg_offset from this value.
> 
> same_at_start is set here:
> 
> 	  int nread = emacs_fd_read (fd, read_buf, sizeof read_buf);
> 	  int bufpos = 0;
> 	  while (bufpos < nread && same_at_start < ZV_BYTE
> 		 && FETCH_BYTE (same_at_start) == read_buf[bufpos])
> 	    same_at_start++, bufpos++;
> 
> which is called some time after:
> 
>   if (emacs_fd_fstat (fd, &st) != 0)
>     report_file_error ("Input file status", orig_filename);
> 
> so it seems entirely possible to me that the file grew by the time we
> set same_at_start, leading to the backtrace.
> 
> The extra text written appears to have been
> 
>     "configure:10959: $? = 0\n"
> 
> My impression is that overlap should have been calculated as 0 (or a
> negative number), not 25, because only one byte matches at the end of
> the buffer (which ends in "s\n") and same_at_end was thus, rightfully,
> 1494419, one less than Z_BYTE = ZV_BYTE.
> 
> This code is very hard to understand; I believe that's because it
> doesn't currently always do the right thing, particularly not if st_size
> changes.  However, a preliminary suggestion for fixing this follows:
> 
> Let's add an additional condition to the loop counting bytes for
> same_at_start, so it never exceeds end_offset - beg_offset + BEGV_BYTE,
> and a symmetric condition in the loop counting bytes for same_at_end, so
> it never becomes less than ZV_BYTE - (end_offset - beg_offset -
> same_at_start + BEGV_BYTE).
> 
> This would fix both this bug and what seems to me to be a faulty
> calculation, failing to detect an overlap, in the case where beg_offset
> is != 0.
> 
> IOW, we would fix the overlap calculation and make it so no overlap can
> ever happen, after which the overlap code can be removed.
> 
> In addition, this code:
> 
>       /* If the file matches the buffer completely,
> 	 there's no need to replace anything.  */
>       if (same_at_start - BEGV_BYTE == end_offset - beg_offset)
> 	{
> 	  emacs_fd_close (fd);
> 	  clear_unwind_protect (fd_index);
> 
> 	  /* Truncate the buffer to the size of the file.  */
> 	  del_range_1 (same_at_start, same_at_end, 0, 0);
> 	  goto handled;
> 	}
> 
> confuses me, because same_at_start and same_at_end are byte positions
> and del_range_1 takes character positions.  I expect it's hit very
> rarely except when same_at_start == same_at_end or same_at_start ==
> BEGV_BYTE == BEGV, and del_range_1 is forgiving in these cases.
> 
> Backtrace:
> 
> #0  terminate_due_to_signal (sig=6, backtrace_limit=2147483647) at emacs.c:425
> #1  0x000055555587f71b in die (msg=0x555555a597b8 "BUF_BEG_BYTE (b) <= bytepos && bytepos <= BUF_Z_BYTE (b)", file=0x555555a5974c "marker.c", line=330)
>     at alloc.c:8064
> #2  0x000055555581e011 in buf_bytepos_to_charpos (b=0x7ff8ef791a58, bytepos=1499444) at marker.c:330
> #3  0x0000555555829246 in BYTE_TO_CHAR (bytepos=1499444) at /home/pip/emacs-20250320/src/buffer.h:1204
> #4  0x0000555555833950 in Finsert_file_contents (filename=0x7ff8ef797af5, visit=0x38, beg=0x0, end=0x0, replace=0xc908) at fileio.c:4547
> 
> 
> 
> 
> 

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>, Pip Cet <pipcet <at> protonmail.com>
Cc: 77315 <at> debbugs.gnu.org
Subject: Re: bug#77315: 31.0.50; Crash in Finsert_file_contents, file size
 changed
Date: Thu, 27 Mar 2025 11:22:48 -0600

On 3/27/25 10:21, Eli Zaretskii wrote:
> AFAIR, insert-file-contents needs to be completely rewritten to handle
> such situations.

I drafted something a while ago along those lines but never got around 
to finish it. I'll try to bump the priority.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 77315 <at> debbugs.gnu.org, pipcet <at> protonmail.com
Subject: Re: bug#77315: 31.0.50; Crash in Finsert_file_contents, file size
 changed
Date: Thu, 27 Mar 2025 19:48:02 +0200

> Date: Thu, 27 Mar 2025 11:22:48 -0600
> Cc: 77315 <at> debbugs.gnu.org
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> 
> On 3/27/25 10:21, Eli Zaretskii wrote:
> > AFAIR, insert-file-contents needs to be completely rewritten to handle
> > such situations.
> 
> I drafted something a while ago along those lines but never got around 
> to finish it. I'll try to bump the priority.

Thanks in advance.

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

From: Pip Cet <pipcet <at> protonmail.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 77315 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#77315: 31.0.50;
 Crash in Finsert_file_contents, file size changed
Date: Thu, 27 Mar 2025 19:17:45 +0000

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

> On 3/27/25 10:21, Eli Zaretskii wrote:
>> AFAIR, insert-file-contents needs to be completely rewritten to handle
>> such situations.
>
> I drafted something a while ago along those lines but never got around
> to finish it. I'll try to bump the priority.

Thank you both!  I'll wait, then.  I'm perfectly happy reading "random"
data if a file is changed while being read, FWIW, but I see no reason to
crash, and it's also somewhat undesirable to throw an error in this
situation.

Pip

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: eggert <at> cs.ucla.edu
Cc: 77315 <at> debbugs.gnu.org, pipcet <at> protonmail.com
Subject: Re: bug#77315: 31.0.50;
 Crash in Finsert_file_contents, file size changed
Date: Sun, 13 Apr 2025 10:12:57 +0300

> Cc: 77315 <at> debbugs.gnu.org, pipcet <at> protonmail.com
> Date: Thu, 27 Mar 2025 19:48:02 +0200
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> > Date: Thu, 27 Mar 2025 11:22:48 -0600
> > Cc: 77315 <at> debbugs.gnu.org
> > From: Paul Eggert <eggert <at> cs.ucla.edu>
> > 
> > On 3/27/25 10:21, Eli Zaretskii wrote:
> > > AFAIR, insert-file-contents needs to be completely rewritten to handle
> > > such situations.
> > 
> > I drafted something a while ago along those lines but never got around 
> > to finish it. I'll try to bump the priority.
> 
> Thanks in advance.

Did you have a chance to make some progress in this matter?

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 77315 <at> debbugs.gnu.org, pipcet <at> protonmail.com
Subject: Re: bug#77315: 31.0.50; Crash in Finsert_file_contents, file size
 changed
Date: Mon, 14 Apr 2025 11:53:40 -0700

On 2025-04-13 00:12, Eli Zaretskii wrote:
>>> I drafted something a while ago along those lines but never got around
>>> to finish it. I'll try to bump the priority.
>> Thanks in advance.
> Did you have a chance to make some progress in this matter?

Not yet. Still on my list of things to do (but below filing taxes...).

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

From: Pip Cet <pipcet <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 77315 <at> debbugs.gnu.org, eggert <at> cs.ucla.edu
Subject: Re: bug#77315: 31.0.50;
 Crash in Finsert_file_contents, file size changed
Date: Sat, 07 Jun 2025 10:52:00 +0000

"Eli Zaretskii" <eliz <at> gnu.org> writes:

>> Cc: 77315 <at> debbugs.gnu.org, pipcet <at> protonmail.com
>> Date: Thu, 27 Mar 2025 19:48:02 +0200
>> From: Eli Zaretskii <eliz <at> gnu.org>
>>
>> > Date: Thu, 27 Mar 2025 11:22:48 -0600
>> > Cc: 77315 <at> debbugs.gnu.org
>> > From: Paul Eggert <eggert <at> cs.ucla.edu>
>> >
>> > On 3/27/25 10:21, Eli Zaretskii wrote:
>> > > AFAIR, insert-file-contents needs to be completely rewritten to handle
>> > > such situations.
>> >
>> > I drafted something a while ago along those lines but never got around
>> > to finish it. I'll try to bump the priority.
>>
>> Thanks in advance.
>
> Did you have a chance to make some progress in this matter?

I'm hitting this bug with some regularity (and working around it by
force-returning from Finsert_file_contents in gdb) on the MPS branch.
It's not a problem, but I am occasionally reminded it's still there :-)

Pip

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pip Cet <pipcet <at> protonmail.com>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 77315 <at> debbugs.gnu.org
Subject: Re: bug#77315: 31.0.50; Crash in Finsert_file_contents, file size
 changed
Date: Sat, 7 Jun 2025 10:24:42 -0700

On 2025-06-07 03:52, Pip Cet wrote:
> I'm hitting this bug with some regularity (and working around it by
> force-returning from Finsert_file_contents in gdb) on the MPS branch.
> It's not a problem, but I am occasionally reminded it's still there :-)

Yes, sorry, I've been crunched doing real work. I have one issue to 
attend to with GNU 'factor' and then this is my top free-software issue. 
But final exams are next week so it won't happen right away.

Previous Next

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