Lots of code for a conceptually simple change: gave the internal set

iteration protocol a destructor (finiSetIteration()), to plug assorted
leaks of keys, values and even BTree nodes.

src/BTrees/BTreeModuleTemplate.c

@@ -179,45 +179,71 @@ staticforward PyExtensionClass BTreeType;
  * 1. Declare a new iterator and a "merge" int:
  *        SetIteration si = {0,0,0};
  *        int merge = 0;
- *    XXX Using "{0,0,0}" or "{0,0}" appear most common, but I don't
- *    XXX think it makes any difference; looks like "{0}" would work too;
- *    XXX looks like not initializing it at all would also work.
+ *    Using "{0,0,0}" or "{0,0}" appear most common.  Only one {0} is
+ *    necssary.  At least one must be given so that finiSetIteration() works
+ *    correctly even if you don't get around to calling initSetIteration().
  * 2. Initialize it via
  *        initSetIteration(&si, PyObject *s, int weight, &merge)
- *    There's an error if that returns an int < 0.  Note that si.set must
- *        be Py_XDECREF'ed in this case.
- *    If it's successful, si.hasValue is set to true iff s has values (as
- *    well as keys).
+ *    It's an error if that returns an int < 0.  In case of error on the
+ *    init call, calling finiSetIteration(&si) is optional.  But if the
+ *    init call succeeds, you must eventually call finiSetIteration(),
+ *    and whether or not subsequent calls to si.next() fail.
  * 3. Get the first element:
  *        if (si.next(&si) < 0) { there was an error }
  *    If the set isn't empty, this sets si.position to an int >= 0,
- *    si.key to the element's key (of type KEY_TYPE), and si.value to
+ *    si.key to the element's key (of type KEY_TYPE), and maybe si.value to
  *    the element's value (of type VALUE_TYPE).  si.value is defined
- *    iff merge was set to true by the initSetIteration() call.  If
- *    there was an error, the caller is responsible for Py_XDECREF'ing
- *    si.set.
+ *    iff merge was set to true by the initSetIteration() call, which is
+ *    equivalent to whether si.usesValue is true.
  * 4. Process all the elements:
  *        while (si.position >= 0) {
  *            do something with si.key and/or si.value;
- *            if (si.next(&si) < 0) {
- *                there was an error;
- *                Py_XDECREF(si.set);
- *                do whatever else is appropriate for the caller;
+ *            if (si.next(&si) < 0) { there was an error; }
  *        }
- *        }
- * 5. Decref the SetIteration's set:
- *        Py_XDECREF(si.set);
+ * 5. Finalize the SetIterator:
+ *        finiSetIteration(&si);
+ *    This is mandatory!  si may contain references to iterator objects,
+ *    keys and values, and they must be cleaned up else they'll leak.  If
+ *    this were C++ we'd hide that in the destructor, but in C you have to
+ *    do it by hand.
  */
 typedef struct SetIteration_s
 {
   PyObject *set;    /* the set, bucket, BTree, ..., being iterated */
   int position;     /* initialized to 0; set to -1 by next() when done */
+  int usesValue;    /* true iff 'set' has values & we iterate them */
   int hasValue;     /* true iff 'set' has values (as well as keys) */
   KEY_TYPE key;     /* next() sets to next key */
   VALUE_TYPE value; /* next() may set to next value */
   int (*next)(struct SetIteration_s*);  /* function to get next key+value */
 } SetIteration;
 
+/* Finish the set iteration protocol.  This MUST be called by everyone
+ * who starts a set iteration, unless the initial call to initSetIteration
+ * failed; in that case, and only that case, calling finiSetIteration is
+ * optional.
+ */
+static void
+finiSetIteration(SetIteration *i)
+{
+    assert(i != NULL);
+    if (i->set == NULL)
+        return;
+    Py_DECREF(i->set);
+    i->set = NULL;      /* so it doesn't hurt to call this again */
+
+    if (i->position > 0) {
+        /* next() was called at least once, but didn't finish iterating
+         * (else position would be negative).  So the cached key and
+         * value need to be cleaned up.
+         */
+        DECREF_KEY(i->key);
+        if (i->usesValue)
+            DECREF_VALUE(i->value);
+    }
+    i->position = -1;   /* stop any stray next calls from doing harm */
+}
+
 static PyObject *
 IndexError(int i)
 {
@@ -384,7 +410,7 @@ static char BTree_module_documentation[] =
 "\n"
 MASTER_ID
 BTREEITEMSTEMPLATE_C
-"$Id: BTreeModuleTemplate.c,v 1.29 2002/06/02 07:46:43 tim_one Exp $\n"
+"$Id: BTreeModuleTemplate.c,v 1.30 2002/06/08 04:41:44 tim_one Exp $\n"
 BTREETEMPLATE_C
 BUCKETTEMPLATE_C
 KEYMACROS_H

src/BTrees/BTreeTemplate.c

@@ -12,7 +12,7 @@
   
  ****************************************************************************/
 
-#define BTREETEMPLATE_C "$Id: BTreeTemplate.c,v 1.36 2002/06/06 19:30:21 jeremy Exp $\n"
+#define BTREETEMPLATE_C "$Id: BTreeTemplate.c,v 1.37 2002/06/08 04:41:44 tim_one Exp $\n"
 
 /*
 ** _BTree_get
@@ -1065,7 +1065,7 @@ BTree_byValue(BTree *self, PyObject *args)
   VALUE_TYPE min;
   VALUE_TYPE v;
   int copied=1;
-  SetIteration it={0,0};
+  SetIteration it = {0, 0, 1};
 
   PER_USE_OR_RETURN(self, NULL);
 
@@ -1114,6 +1114,7 @@ BTree_byValue(BTree *self, PyObject *args)
   UNLESS (item) goto err;
   Py_DECREF(item);
 
+  finiSetIteration(&it);
   PER_ALLOW_DEACTIVATION(self);
   PER_ACCESSED(self);
   return r;
@@ -1122,7 +1123,7 @@ BTree_byValue(BTree *self, PyObject *args)
   PER_ALLOW_DEACTIVATION(self);
   PER_ACCESSED(self);
   Py_XDECREF(r);
-  Py_XDECREF(it.set);
+  finiSetIteration(&it);
   Py_XDECREF(item);
   return NULL;
 }

src/BTrees/MergeTemplate.c

@@ -12,7 +12,7 @@
 
  ****************************************************************************/
 
-#define MERGETEMPLATE_C "$Id: MergeTemplate.c,v 1.13 2002/06/03 17:21:55 tim_one Exp $\n"
+#define MERGETEMPLATE_C "$Id: MergeTemplate.c,v 1.14 2002/06/08 04:41:44 tim_one Exp $\n"
 
 /****************************************************************************
  Set operations
@@ -252,9 +252,9 @@ bucket_merge(Bucket *s1, Bucket *s2, Bucket *s3)
       if (i3.next(&i3) < 0) goto err;
     }
 
-  Py_DECREF(i1.set);
-  Py_DECREF(i2.set);
-  Py_DECREF(i3.set);
+  finiSetIteration(&i1);
+  finiSetIteration(&i2);
+  finiSetIteration(&i3);
 
   if (s1->next)
     {
@@ -267,9 +267,9 @@ bucket_merge(Bucket *s1, Bucket *s2, Bucket *s3)
   return s;
 
  err:
-  Py_XDECREF(i1.set);
-  Py_XDECREF(i2.set);
-  Py_XDECREF(i3.set);
+  finiSetIteration(&i1);
+  finiSetIteration(&i2);
+  finiSetIteration(&i3);
   Py_XDECREF(r);
   return NULL;
 }

src/BTrees/SetOpTemplate.c

@@ -16,7 +16,7 @@
  Set operations
  ****************************************************************************/
 
-#define SETOPTEMPLATE_C "$Id: SetOpTemplate.c,v 1.23 2002/06/08 02:37:58 tim_one Exp $\n"
+#define SETOPTEMPLATE_C "$Id: SetOpTemplate.c,v 1.24 2002/06/08 04:41:44 tim_one Exp $\n"
 
 #ifdef INTSET_H
 static int
@@ -49,7 +49,14 @@ nextIntSet(SetIteration *i)
 static int
 nextKeyAsSet(SetIteration *i)
 {
-  i->position = i->position == 0 ? 1 : -1;
+    if (i->position >= 0) {
+        if (i->position) {
+            DECREF_KEY(i->key);
+            i->position = -1;
+        }
+        else
+            i->position = 1;
+    }
     return 0;
 }
 #endif
@@ -67,90 +74,100 @@ nextKeyAsSet(SetIteration *i)
  *
  * Return
  *      0 on success; -1 and an exception set if error.
- *      merge is set to 1 if s has values and w >= 0, else merge is left
+ *      *merge is set to 1 if s has values and w >= 0, else merge is left
  *          alone.
+ *      i.usesValue is also set to 1 if s has values and w >= 0, else
+ *          .usesValue is set to 0.
  *      i.set gets a new reference to s, or to some other object used to
- *          iterate over s.  The caller must Py_XDECREF i.set when it's
- *          done with i, and regardless of whether the call to
- *          initSetIteration succeeds or fails (a failing call may or not
- *          set i.set to NULL).
+ *          iterate over s.
  *      i.position is set to 0.
  *      i.hasValue is set to true if s has values, and to false otherwise.
- *          Note that this is done independent of w's value.
+ *          Note that this is done independent of w's value, and when w < 0
+ *          may differ from i.usesValue.
  *      i.next is set to an appropriate iteration function.
  *      i.key and i.value are left alone.
+ *
+ * Internal
+ *      i.position < 0 means iteration terminated.
+ *      i.position = 0 means iteration hasn't yet begun (next() hasn't
+ *          been called yet).
+ *      In all other cases, i.key, and possibly i.value, own references.
+ *          These must be cleaned up, either by next() routines, or by
+ *          finiSetIteration.
+ *      next() routines must ensure the above.  They should return without
+ *          doing anything when i.position < 0.
+ *      It's the responsibility of {init, fini}setIteration to clean up
+ *          the reference in i.set, and to ensure that no stale references
+ *          live in i.key or i.value if iteration terminates abnormally.
+ *          A SetIteration struct has been cleaned up iff i.set is NULL.
  */
 static int
 initSetIteration(SetIteration *i, PyObject *s, int w, int *merge)
 {
-  i->position = 0;
+  i->set = NULL;
+  i->position = -1;     /* set to 0 only on normal return */
+  i->hasValue = 0;      /* assume it's a set */
+  i->usesValue = 0;     /* assume it's a set or that values aren't iterated */
 
   if (ExtensionClassSubclassInstance_Check(s, &BucketType))
     {
       i->set = s;
       Py_INCREF(s);
+      i->hasValue = 1;
 
       if (w >= 0)
         {
-          *merge = 1;
+          i->usesValue = 1;
           i->next = nextBucket;
         }
       else
         i->next = nextSet;
-
-      i->hasValue = 1;
     }
   else if (ExtensionClassSubclassInstance_Check(s, &SetType))
     {
       i->set = s;
       Py_INCREF(s);
-
       i->next = nextSet;
-      i->hasValue = 0;
     }
   else if (ExtensionClassSubclassInstance_Check(s, &BTreeType))
     {
       i->set = BTree_rangeSearch(BTREE(s), NULL, 'i');
       UNLESS(i->set) return -1;
+      i->hasValue = 1;
 
       if (w >= 0)
         {
-          *merge = 1;
+          i->usesValue = 1;
           i->next = nextBTreeItems;
         }
       else
         i->next = nextTreeSetItems;
-      i->hasValue = 1;
     }
   else if (ExtensionClassSubclassInstance_Check(s, &TreeSetType))
     {
       i->set = BTree_rangeSearch(BTREE(s), NULL, 'k');
       UNLESS(i->set) return -1;
-
       i->next = nextTreeSetItems;
-      i->hasValue = 0;
     }
 #ifdef INTSET_H
   else if (s->ob_type == (PyTypeObject*)intSetType)
     {
       i->set = s;
       Py_INCREF(s);
-
       i->next = nextIntSet;
-      i->hasValue = 0;
     }
 #endif
 #ifdef KEY_CHECK
   else if (KEY_CHECK(s))
     {
       int copied = 1;
+      COPY_KEY_FROM_ARG(i->key, s, copied);
+      UNLESS (copied) return -1;
 
+      INCREF_KEY(i->key);
       i->set = s;
       Py_INCREF(s);
-      i->next=nextKeyAsSet;
-      i->hasValue = 0;
-      COPY_KEY_FROM_ARG(i->key, s, copied);
-      UNLESS (copied) return -1;
+      i->next = nextKeyAsSet;
     }
 #endif
   else
@@ -159,6 +176,9 @@ initSetIteration(SetIteration *i, PyObject *s, int w, int *merge)
       return -1;
     }
 
+  *merge |= i->usesValue;
+  i->position = 0;
+
   return 0;
 }
 
@@ -299,8 +319,8 @@ set_operation(PyObject *s1, PyObject *s2,
   if(c1 && copyRemaining(r, &i1, merge, w1) < 0) goto err;
   if(c2 && copyRemaining(r, &i2, merge, w2) < 0) goto err;
 
-  Py_DECREF(i1.set);
-  Py_DECREF(i2.set);
+  finiSetIteration(&i1);
+  finiSetIteration(&i2);
 
   return OBJECT(r);
 
@@ -310,8 +330,8 @@ invalid_set_operation:
 #endif
 
 err:
-  Py_XDECREF(i1.set);
-  Py_XDECREF(i2.set);
+  finiSetIteration(&i1);
+  finiSetIteration(&i2);
   Py_XDECREF(r);
   return NULL;
 }
@@ -434,7 +454,7 @@ multiunion_m(PyObject *ignored, PyObject *args)
     int n;                  /* length of input sequence */
     PyObject *set = NULL;   /* an element of the input sequence */
     Bucket *result;         /* result set */
-    SetIteration setiter = {0, 0, 0};
+    SetIteration setiter = {0};
     int i;
 
     UNLESS(PyArg_ParseTuple(args, "O", &seq))
@@ -497,8 +517,7 @@ multiunion_m(PyObject *ignored, PyObject *args)
                 /* We know the key is an int, so no need to incref it. */
                 if (setiter.next(&setiter) < 0) goto Error;
             }
-            Py_DECREF(setiter.set);
-            setiter.set = NULL;
+            finiSetIteration(&setiter);
         }
         Py_DECREF(set);
         set = NULL;
@@ -519,7 +538,7 @@ multiunion_m(PyObject *ignored, PyObject *args)
 Error:
     Py_DECREF(result);
     Py_XDECREF(set);
-    Py_XDECREF(setiter.set);
+    finiSetIteration(&setiter);
     return NULL;
 }