Re: [PATCH] Reference leak in `iprin1 ()'

[ludovic.courtes@laas.fr (Ludovic Courtès)]

Hello,

Neil Jerram <neil@ossau.uklinux.net> writes:

> Nice work, but it looks to me that PUSH_REF sets the value of the
> (pstate->top)th element _before_ incrementing pstate->top.  So
> shouldn't your fix do the decrement first and then set the slot to
> undefined?

Yes, you're perfectly right.

> (Note that your fix will still prevent most reference leaks, just not
> the outermost one.  So that's why it may appear to be working
> already.)

Exactly.

> Also, is the () -> (void) change strictly related?

In fact, it's strictly unrelated, but while we're at it (really `(void)'
was meant here, not `()')...

Below is an updated patch.  I modified `PUSH_REF ()' as well so that it
does PSTATE->TOP++ _after_ using the `PSTATE_STACK_SET ()' macro: this
is safer.  And, well, I couldn't resist the desire to "beautifully
backslashify" `ENTER_NESTED_DATA ()' as well.  :-)  I hope this is not
too much pollution for you.

Thanks,
Ludovic.


2005-11-14  Ludovic Courtès  <ludovic.courtes@laas.fr>

	* print.c (EXIT_NESTED_DATA): Before popping from the stack,
	reset the value at its top.  This fixes a reference leak.
	(PUSH_REF): Perform `pstate->top++' after calling
	`PSTATE_STACK_SET ()' in order to avoid undesired side effects.
	(scm_current_pstate): Replaced `()' by `(void)'.


--- orig/libguile/print.c
+++ mod/libguile/print.c
@@ -112,31 +112,40 @@
  * time complexity (O (depth * N)), The printer code can be
  * rewritten to be O(N).
  */
-#define PUSH_REF(pstate, obj) \
-do { \
-  PSTATE_STACK_SET (pstate, pstate->top++, obj); \
-  if (pstate->top == pstate->ceiling) \
-    grow_ref_stack (pstate); \
+#define PUSH_REF(pstate, obj)			\
+do						\
+{						\
+  PSTATE_STACK_SET (pstate, pstate->top, obj);	\
+  pstate->top++;				\
+  if (pstate->top == pstate->ceiling)		\
+    grow_ref_stack (pstate);			\
 } while(0)
 
-#define ENTER_NESTED_DATA(pstate, obj, label) \
-do { \
-  register unsigned long i; \
-  for (i = 0; i < pstate->top; ++i) \
-    if (scm_is_eq (PSTATE_STACK_REF (pstate, i), (obj))) \
-      goto label; \
-  if (pstate->fancyp) \
-    { \
-      if (pstate->top - pstate->list_offset >= pstate->level) \
-	{ \
-	  scm_putc ('#', port); \
-	  return; \
-	} \
-    } \
-  PUSH_REF(pstate, obj); \
+#define ENTER_NESTED_DATA(pstate, obj, label)			\
+do								\
+{								\
+  register unsigned long i;					\
+  for (i = 0; i < pstate->top; ++i)				\
+    if (scm_is_eq (PSTATE_STACK_REF (pstate, i), (obj)))	\
+      goto label;						\
+  if (pstate->fancyp)						\
+    {								\
+      if (pstate->top - pstate->list_offset >= pstate->level)	\
+	{							\
+	  scm_putc ('#', port);					\
+	  return;						\
+	}							\
+    }								\
+  PUSH_REF(pstate, obj);					\
 } while(0)
 
-#define EXIT_NESTED_DATA(pstate) { --pstate->top; }
+#define EXIT_NESTED_DATA(pstate)				\
+do								\
+{								\
+  --pstate->top;						\
+  PSTATE_STACK_SET (pstate, pstate->top, SCM_UNDEFINED);	\
+}								\
+while (0)
 
 SCM scm_print_state_vtable = SCM_BOOL_F;
 static SCM print_state_pool = SCM_EOL;
@@ -144,8 +153,8 @@
 
 #ifdef GUILE_DEBUG /* Used for debugging purposes */
 
-SCM_DEFINE (scm_current_pstate, "current-pstate", 0, 0, 0, 
-           (),
+SCM_DEFINE (scm_current_pstate, "current-pstate", 0, 0, 0,
+	    (void),
 	    "Return the current-pstate -- the car of the\n"
 	    "@code{print_state_pool}.  @code{current-pstate} is only\n"
 	    "included in @code{--enable-guile-debug} builds.")




_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel