patch to fileio.c

patch to fileio.c

[Fabrice Popineau]

Hi,

You should consider adding the following patch. As I reported earlier, the 
fd file descriptor may be
closed twice in insert-file-contents. It happens every time a file is 
visited and this same file is modified
outside emacs. In this case, you can ask to visit the file again (or emacs 
will ask you if you try to save it)

"File foobar.txt changed on disk. Reread from disk? (yes or no)"

The fd file descriptor in this case is closed  by emacs_close() at line 3654
and by close_file_unwind() registered at line 3233. When emacs_close(fd) is 
reached, the unwind_protect
registered function should be removed. Unfortunately, in the mean time other 
stuff has
been put in the unwind_protect stack, so you can't just decrement the 
pointer. It can be done
when the handled: label is reached. Hence the flag in my patch.

There was a previous patch may be related to the same problem, but not 
fixing it:
2008-09-14  Kenichi Handa  <handa@m17n.org>

 * fileio.c (Finsert_file_contents): Delete incorrect decrement of
 specpdl_ptr.

Best regards,

Fabrice

--- \Mirror\emacs\src\fileio.c  2008-10-16 21:30:49.000000000 +0200
+++ fileio.c    2008-10-26 10:39:11.000000000 +0100
@@ -3141,6 +3141,7 @@
   int read_quit = 0;
   Lisp_Object old_Vdeactivate_mark = Vdeactivate_mark;
   int we_locked_file = 0;
+  int should_remove_unwind_protect = 0;

   if (current_buffer->base_buffer && ! NILP (visit))
     error ("Cannot do file visiting in an indirect buffer");
@@ -3650,8 +3651,10 @@
          if (coding.carryover_bytes > 0)
            bcopy (coding.carryover, read_buf, unprocessed);
        }
+
       UNGCPRO;
       emacs_close (fd);
+      should_remove_unwind_protect = 1;

       /* At this point, HOW_MUCH should equal TOTAL, or should be <= 0
         if we couldn't read the file.  */
@@ -4033,6 +4036,9 @@
 #endif

  handled:
+#if 1
+  if (should_remove_unwind_protect) specpdl_ptr--;
+#endif

   if (!NILP (visit))
     {
 

Re: patch to fileio.c

[Chong Yidong]

"Fabrice Popineau" <fabrice.popineau@supelec.fr> writes:

> You should consider adding the following patch. As I reported earlier,
> the fd file descriptor may be closed twice in insert-file-contents. It
> happens every time a file is visited and this same file is modified
> outside emacs. In this case, you can ask to visit the file again (or
> emacs will ask you if you try to save it)
>
> "File foobar.txt changed on disk. Reread from disk? (yes or no)"
>
> The fd file descriptor in this case is closed by emacs_close() at line
> 3654 and by close_file_unwind() registered at line 3233. When
> emacs_close(fd) is reached, the unwind_protect registered function
> should be removed. Unfortunately, in the mean time other stuff has
> been put in the unwind_protect stack, so you can't just decrement the
> pointer. It can be done when the handled: label is reached. Hence the
> flag in my patch.

Thanks for the patch.  Your analysis sounds correct.

However, I would like to test it first.  Do you have a recipe for
demonstrating incorrect specpdl_ptr handling (e.g. a crash)?

Re: patch to fileio.c

[Fabrice Popineau]

[ I'm posting my answer here too.]

> Thanks for the patch.  Your analysis sounds correct.
>
> However, I would like to test it first.  Do you have a recipe for
> demonstrating incorrect specpdl_ptr handling (e.g. a crash)?

I compiled emacs with msvc, and it crashed all the time under the same
circumstance.
Visit a file, modify it outside emacs, and try to re-visit it. Emacs will
detect that it has been
modified and will ask about reloading it. The fd is closed twice because of
the unwind_protect.
With msvcrt.dll, the MS C library, this is a crash. I'm not sure that glibc
is that picky. Possibly
you won't see the problem. However, to make sure, and to test the patch, I
added printf()
statements at both places where fd can be closed, to trace what happened.

Greetings,

Fabrice

Re: patch to fileio.c

[Chong Yidong]

"Fabrice Popineau" <fabrice.popineau@free.fr> writes:

> [ I'm posting my answer here too.]
>
>> Thanks for the patch.  Your analysis sounds correct.
>>
>> However, I would like to test it first.  Do you have a recipe for
>> demonstrating incorrect specpdl_ptr handling (e.g. a crash)?
>
> I compiled emacs with msvc, and it crashed all the time under the same
> circumstance.  Visit a file, modify it outside emacs, and try to
> re-visit it. Emacs will detect that it has been modified and will ask
> about reloading it. The fd is closed twice because of the
> unwind_protect.  With msvcrt.dll, the MS C library, this is a
> crash. I'm not sure that glibc is that picky. Possibly you won't see
> the problem. However, to make sure, and to test the patch, I added
> printf() statements at both places where fd can be closed, to trace
> what happened.

Does anyone else using Windows see this problem?

Re: patch to fileio.c

[Eli Zaretskii]

> From: Chong Yidong <cyd@stupidchicken.com>
> Date: Thu, 13 Nov 2008 10:43:42 -0500
> 
> "Fabrice Popineau" <fabrice.popineau@free.fr> writes:
> 
> > [ I'm posting my answer here too.]
> >
> >> Thanks for the patch.  Your analysis sounds correct.
> >>
> >> However, I would like to test it first.  Do you have a recipe for
> >> demonstrating incorrect specpdl_ptr handling (e.g. a crash)?
> >
> > I compiled emacs with msvc, and it crashed all the time under the same
> > circumstance.  Visit a file, modify it outside emacs, and try to
> > re-visit it. Emacs will detect that it has been modified and will ask
> > about reloading it. The fd is closed twice because of the
> > unwind_protect.  With msvcrt.dll, the MS C library, this is a
> > crash. I'm not sure that glibc is that picky. Possibly you won't see
> > the problem. However, to make sure, and to test the patch, I added
> > printf() statements at both places where fd can be closed, to trace
> > what happened.
> 
> Does anyone else using Windows see this problem?

I don't see the crashes, but if I run Emacs under GDB and put a
breakpoint in w32.c:sys_close, I definitely see it being called twice
for the same file descriptor on which Emacs opened the file in
insert-file-contents.

Re: patch to fileio.c

[Chong Yidong]

"Fabrice Popineau" <fabrice.popineau@supelec.fr> writes:

> You should consider adding the following patch. As I reported earlier,
> the fd file descriptor may be closed twice in insert-file-contents. It
> happens every time a file is visited and this same file is modified
> outside emacs. In this case, you can ask to visit the file again (or
> emacs will ask you if you try to save it)
>
> "File foobar.txt changed on disk. Reread from disk? (yes or no)"
>
> The fd file descriptor in this case is closed by emacs_close() at line
> 3654 and by close_file_unwind() registered at line 3233. When
> emacs_close(fd) is reached, the unwind_protect registered function
> should be removed. Unfortunately, in the mean time other stuff has
> been put in the unwind_protect stack, so you can't just decrement the
> pointer. It can be done when the handled: label is reached. Hence the
> flag in my patch.

I've checked in your patch.  Thanks.

Re: patch to fileio.c

[Chong Yidong]

Chong Yidong <cyd@stupidchicken.com> writes:

>> The fd file descriptor in this case is closed by emacs_close() at line
>> 3654 and by close_file_unwind() registered at line 3233. When
>> emacs_close(fd) is reached, the unwind_protect registered function
>> should be removed. Unfortunately, in the mean time other stuff has
>> been put in the unwind_protect stack, so you can't just decrement the
>> pointer. It can be done when the handled: label is reached. Hence the
>> flag in my patch.
>
> I've checked in your patch.  Thanks.

BTW, could someone check whether this has any impact on the tar file
crash in Windows?  (Hope springs eternal :-)

Re: patch to fileio.c

[Juanma Barranquero]

On Fri, Nov 14, 2008 at 22:16, Chong Yidong <cyd@stupidchicken.com> wrote:

> BTW, could someone check whether this has any impact on the tar file
> crash in Windows?  (Hope springs eternal :-)

Sorry.

             Juanma


Breakpoint 1, w32_abort () at w32fns.c:7279
7279      button = MessageBox (NULL,
(gdb) bt
#0  w32_abort () at w32fns.c:7279
#1  0x011d0441 in r_re_alloc (ptr=0x312b408, size=2191) at ralloc.c:1028
#2  0x01070e4b in enlarge_buffer_text (b=0x312b400, delta=2170) at buffer.c:5065
#3  0x01177eee in make_gap_larger (nbytes_added=2170) at insdel.c:526
#4  0x0117908f in insert_from_string_1 (string=48214403, pos=0,
pos_byte=0, nchars=190, nbytes=190, inherit=0, before_markers=0)
    at insdel.c:1107
#5  0x0117934b in insert_from_string (string=48214403, pos=0,
pos_byte=0, length=190, length_byte=190, inherit=0)
    at insdel.c:1048
#6  0x0116b91f in general_insert_function (insert_func=0x117979d <insert>,
    insert_from_string_func=0x11792dc <insert_from_string>, inherit=0,
nargs=2, args=0x82ee60) at editfns.c:2184
#7  0x0116ba94 in Finsert (nargs=2, args=0x82ee60) at editfns.c:2228
#8  0x01146575 in Fbyte_code (bytestr=57438499, vector=53536772,
maxdepth=<value optimized out>) at bytecode.c:1265
#9  0x0101c466 in funcall_lambda (fun=54011108, nargs=0,
arg_vector=0x82efa4) at eval.c:3231
#10 0x01019bac in Ffuncall (nargs=1, args=0x82efa0) at eval.c:3090
#11 0x01146e24 in Fbyte_code (bytestr=57478915, vector=53535748,
maxdepth=<value optimized out>) at bytecode.c:678
#12 0x0101c466 in funcall_lambda (fun=52967844, nargs=0,
arg_vector=0x82f0e4) at eval.c:3231
#13 0x01019bac in Ffuncall (nargs=1, args=0x82f0e0) at eval.c:3090
#14 0x01146e24 in Fbyte_code (bytestr=19203587, vector=19203628,
maxdepth=<value optimized out>) at bytecode.c:678
#15 0x0101c466 in funcall_lambda (fun=19203532, nargs=2,
arg_vector=0x82f214) at eval.c:3231
#16 0x01019bac in Ffuncall (nargs=3, args=0x82f210) at eval.c:3090
#17 0x01146e24 in Fbyte_code (bytestr=19202763, vector=19202996,
maxdepth=<value optimized out>) at bytecode.c:678
#18 0x0101c466 in funcall_lambda (fun=19202716, nargs=0,
arg_vector=0x82f2e0) at eval.c:3231
#19 0x0101c6ec in apply_lambda (fun=19202716, args=47925249,
eval_flag=1) at eval.c:3155
#20 0x0101b99d in Feval (form=19198109) at eval.c:2417
#21 0x0101cce3 in internal_lisp_condition_case (var=48413441,
bodyform=19198109, handlers=19198117) at eval.c:1456
#22 0x011476b5 in Fbyte_code (bytestr=19197915, vector=19198020,
maxdepth=<value optimized out>) at bytecode.c:868
#23 0x0101c466 in funcall_lambda (fun=19197868, nargs=1,
arg_vector=0x82f574) at eval.c:3231
#24 0x01019bac in Ffuncall (nargs=2, args=0x82f570) at eval.c:3090
#25 0x01146e24 in Fbyte_code (bytestr=19196987, vector=19197236,
maxdepth=<value optimized out>) at bytecode.c:678
#26 0x0101c466 in funcall_lambda (fun=19196908, nargs=2,
arg_vector=0x82f6b4) at eval.c:3231
#27 0x01019bac in Ffuncall (nargs=3, args=0x82f6b0) at eval.c:3090
#28 0x01146e24 in Fbyte_code (bytestr=19194627, vector=19194804,
maxdepth=<value optimized out>) at bytecode.c:678
#29 0x0101c466 in funcall_lambda (fun=19194556, nargs=6,
arg_vector=0x82f7e4) at eval.c:3231
#30 0x01019bac in Ffuncall (nargs=7, args=0x82f7e0) at eval.c:3090
#31 0x01146e24 in Fbyte_code (bytestr=19193387, vector=19193748,
maxdepth=<value optimized out>) at bytecode.c:678
#32 0x0101c466 in funcall_lambda (fun=19193316, nargs=4,
arg_vector=0x82f924) at eval.c:3231
#33 0x01019bac in Ffuncall (nargs=5, args=0x82f920) at eval.c:3090
#34 0x01146e24 in Fbyte_code (bytestr=19186819, vector=19186876,
maxdepth=<value optimized out>) at bytecode.c:678
#35 0x0101c466 in funcall_lambda (fun=19186764, nargs=2,
arg_vector=0x82fa64) at eval.c:3231
#36 0x01019bac in Ffuncall (nargs=3, args=0x82fa60) at eval.c:3090
#37 0x0101b5af in Fapply (nargs=2, args=0x82fac0) at eval.c:2532
#38 0x0101b7a8 in apply1 (fn=56944145, arg=2191) at eval.c:2793
#39 0x01148ae0 in Fcall_interactively (function=56944145,
record_flag=47925249, keys=47958788) at callint.c:389
#40 0x01019fb6 in Ffuncall (nargs=4, args=0x82fc78) at eval.c:3050
#41 0x0101a238 in call3 (fn=48116833, arg1=56944145, arg2=47925249,
arg3=47925249) at eval.c:2870
#42 0x01096318 in command_loop_1 () at keyboard.c:1880
#43 0x010192f6 in internal_condition_case (bfun=0x1095d3f
<command_loop_1>, handlers=47989001, hfun=0x108cf77 <cmd_error>)
    at eval.c:1511
#44 0x0108c40b in command_loop_2 () at keyboard.c:1338
#45 0x010193a0 in internal_catch (tag=47985073, func=0x108c3e8
<command_loop_2>, arg=47925249) at eval.c:1247
#46 0x0108cdc2 in command_loop () at keyboard.c:1317
#47 0x0108d110 in recursive_edit_1 () at keyboard.c:942
#48 0x0108d27b in Frecursive_edit () at keyboard.c:1004
#49 0x01002fb1 in main (argc=2, argv=0xa92750) at emacs.c:1777

Lisp Backtrace:
"tar-summarize-buffer" (0x82efa4)
"tar-mode" (0x82f0e4)
"set-auto-mode-0" (0x82f214)
"set-auto-mode" (0x82f2e0)
"normal-mode" (0x82f574)
"after-find-file" (0x82f6b4)
"find-file-noselect-1" (0x82f7e4)
"find-file-noselect" (0x82f924)
"find-file" (0x82fa64)
"call-interactively" (0x82fc7c)
(gdb)