Package: emacs;
Reported by: Pip Cet <pipcet <at> protonmail.com>
Date: Thu, 27 Mar 2025 15:36:02 UTC
Severity: normal
Found in version 31.0.50
View this message in rfc822 format
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: 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
>
>
>
>
>
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.