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

[Ludovic Courtès]

Hello,

Neil Jerram <address@hidden> 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  <address@hidden>

        * 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.")