PATCH: fix for memory corruption and eventual crash in print.c

PATCH: fix for memory corruption and eventual crash in print.c

[Ami Fischman]

[-- Attachment #1.1: Type: text/plain, Size: 1835 bytes --]

src/print.c:print_object() has this code:

   1570       if (NILP (Vprint_circle) && NILP (Vprint_gensym))
[...]
   1581           being_printed[print_depth] = obj;
[...]
   1611   print_depth++;
   1612
   1613   /* See similar code in print_preprocess.  */
   1614   if (print_depth > PRINT_CIRCLE)
   1615     error ("Apparently circular structure being printed");

Note that being_printed[print_depth] is assigned to /before/ print_depth is
checked for exceeding PRINT_CIRCLE (the declared size of being_printed).
Here's a snippet of elisp that exhibits the bug:

(let ((print-circle nil)
      (i 0))
  (require 'cl)
  (setq x '(a b))
  (while (< i 200)
    (incf i)
    (setq x `(,x)))
  (prin1-to-string x))

This errors with "Apparently circular structure being printed".  So far so
good.  Now evaling:
(prin1-to-string "hello")
errors with "Lisp nesting exceeds `max-lisp-eval-depth`" even though it
should be an easy thing to print!

Groveling with gdb shows that Vprin1_to_string_buffer gets overwritten
during the deep prin1 because of the bug above and instead of pointing at
the " prin1" buffer it has a value that pp's as a long chain of "[[[[[["'s
and errors out before completing the pretty-print.

Moving the guard check on print_depth above the assignment (and changing >
to >= because it's now above the print_depth++) makes the first elisp
snippet innocuous - eval'ing it still errors out about the apparently
circular structure, but subsequent prin1-to-string's work just fine.

Patch attached.

FWIW, I discovered the bug because using emacs-jabber was making my emacs
sessions unstable (at some point random standard elisp functions would start
failing).  It turned out that one of its variables (jabber-connections) has
a deeply-enough nested component that this bug is triggered if it is
prin1'd.

Cheers,
-a

[-- Attachment #1.2: Type: text/html, Size: 4204 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: prin1.patch --]
[-- Type: text/x-patch; name=prin1.patch, Size: 817 bytes --]

diff --git a/src/print.c b/src/print.c
index 8fac266..b9d2e12 100644
--- a/src/print.c
+++ b/src/print.c
@@ -1560,6 +1560,10 @@ print_object (obj, printcharfun, escapeflag)
 
   QUIT;
 
+  /* See similar code in print_preprocess.  */
+  if (print_depth >= PRINT_CIRCLE)
+    error ("Apparently circular structure being printed");
+
   /* Detect circularities and truncate them.  */
   if (STRINGP (obj) || CONSP (obj) || VECTORP (obj)
       || COMPILEDP (obj) || CHAR_TABLE_P (obj) || SUB_CHAR_TABLE_P (obj)
@@ -1610,9 +1614,6 @@ print_object (obj, printcharfun, escapeflag)
 
   print_depth++;
 
-  /* See similar code in print_preprocess.  */
-  if (print_depth > PRINT_CIRCLE)
-    error ("Apparently circular structure being printed");
 #ifdef MAX_PRINT_CHARS
   if (max_print && print_chars > max_print)
     {

Re: PATCH: fix for memory corruption and eventual crash in print.c

[YAMAMOTO Mitsuharu]

>>>>> On Sat, 5 Jul 2008 21:11:14 -0700, "Ami Fischman" <ami@fischman.org> said:

> src/print.c:print_object() has this code:

>    1570       if (NILP (Vprint_circle) && NILP (Vprint_gensym))
> [...]
>    1581           being_printed[print_depth] = obj;
> [...]
>    1611   print_depth++;
>    1612
>    1613   /* See similar code in print_preprocess.  */
>    1614   if (print_depth > PRINT_CIRCLE)
>    1615     error ("Apparently circular structure being printed");

> Note that being_printed[print_depth] is assigned to /before/ print_depth is
> checked for exceeding PRINT_CIRCLE (the declared size of being_printed).

This reminds me of a similar buffer overrun that had existed in
`print_preprocess' also with respect to `being_printed'.

  http://lists.gnu.org/archive/html/emacs-devel/2004-07/msg00146.html

I think a fix for `print_object' should go to the EMACS_22_BASE branch
as well as the trunk so it may not be missed.  Another candidate for
the inclusion to the EMACS_22_BASE branch would be the PNG background
color bit-depth fix below:

2008-05-09  Chong Yidong  <cyd@stupidchicken.com>

	* image.c (png_load): Use correct bit-depth for setting background
	color.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: PATCH: fix for memory corruption and eventual crash in print.c

[Chong Yidong]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

> I think a fix for `print_object' should go to the EMACS_22_BASE branch
> as well as the trunk so it may not be missed.  Another candidate for
> the inclusion to the EMACS_22_BASE branch would be the PNG background
> color bit-depth fix below:
>
> 2008-05-09  Chong Yidong  <cyd@stupidchicken.com>
>
> 	* image.c (png_load): Use correct bit-depth for setting background
> 	color.

Yes.  Could someone please check it into the branch?  Thanks.

Re: PATCH: fix for memory corruption and eventual crash in print.c

[YAMAMOTO Mitsuharu]

>>>>> On Sun, 06 Jul 2008 09:51:59 -0400, Chong Yidong <cyd@stupidchicken.com> said:

> YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:
>> I think a fix for `print_object' should go to the EMACS_22_BASE branch
>> as well as the trunk so it may not be missed.  Another candidate for
>> the inclusion to the EMACS_22_BASE branch would be the PNG background
>> color bit-depth fix below:
>> 
>> 2008-05-09  Chong Yidong  <cyd@stupidchicken.com>
>> 
>> * image.c (png_load): Use correct bit-depth for setting background
>> color.

> Yes.  Could someone please check it into the branch?  Thanks.

Done for `png_load'.  For the `print_object' patch offered by the OP,
I'd like to leave it to experts on this area.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: PATCH: fix for memory corruption and eventual crash in print.c

[Chong Yidong]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

>> Yes.  Could someone please check it into the branch?  Thanks.
>
> Done for `png_load'.  For the `print_object' patch offered by the OP,
> I'd like to leave it to experts on this area.

Thanks.

Re: PATCH: fix for memory corruption and eventual crash in print.c

[Chong Yidong]

"Ami Fischman" <ami@fischman.org> writes:

> src/print.c:print_object() has this code:
>
>    1570       if (NILP (Vprint_circle) && NILP (Vprint_gensym))
> [...]
>    1581           being_printed[print_depth] = obj;
> [...]
>    1611   print_depth++;
>    1612
>    1613   /* See similar code in print_preprocess.  */
>    1614   if (print_depth > PRINT_CIRCLE)
>    1615     error ("Apparently circular structure being printed");
>
> Note that being_printed[print_depth] is assigned to /before/ print_depth is
> checked for exceeding PRINT_CIRCLE (the declared size of being_printed). 
>
> Patch attached.

Thanks, I've checked in the fix (to both the trunk and the branch).