[Zodb-checkins] CVS: ZODB3/BTrees - BTreeItemsTemplate.c:1.17.12.1 BTreeTemplate.c:1.71.12.2 BucketTemplate.c:1.47.12.2

[Tim Peters]

Update of /cvs-repository/ZODB3/BTrees
In directory cvs.zope.org:/tmp/cvs-serv4208/BTrees

Modified Files:
      Tag: ZODB3-3_1-branch
	BTreeItemsTemplate.c BTreeTemplate.c BucketTemplate.c 
Log Message:
Backporting various BTree and Bucket fixes from the Zope2 and Zope3
heads:

_bucket_set():  This could leave a mapping bucket in a variety of insane
states when the value passed in was of the wrong type (for example,
doing
    b[obj] = 3.7
when b is an OIBTree).  This manifested as a refcount leak in the test
suite, but could have been much worse (most likely in real life is that
a seemingly arbitrary existing key would "go missing").

_BTree_set():  We leaked a reference to the first key of the second
bucket when deleting the first child of a BTree node with more than one
child.  This caused >600 int objects to leak in the OI and OO flavors
of testRemoveSucceeds.

BTree_deleteNextBucket():  This failed to decref the temp result from
BTree_lastBucket().  In unusual cases, this could cause a chain of
buckets to leak (the DegenerateBTree tests appeared to be the only
ones that provoked this, and there it leaked 285 IISet buckets).  Other
uses of BTree_lastBucket() appear to be refcount-correct.

nextBTreeItems():  This was copying the value (from the next (key,
value) pair) from the bucket into the set-iteration struct twice.  I
don't believe this had any visible effect, it was simply pointless and
wasted a little time (discovered by eyeball).


=== ZODB3/BTrees/BTreeItemsTemplate.c 1.17 => 1.17.12.1 ===
--- ZODB3/BTrees/BTreeItemsTemplate.c:1.17	Sat Jun 22 13:22:54 2002
+++ ZODB3/BTrees/BTreeItemsTemplate.c	Fri Apr 18 14:08:36 2003
@@ -480,9 +480,6 @@
 
           COPY_VALUE(i->value,
                      currentbucket->values[ITEMS(i->set)->currentoffset]);
-          COPY_VALUE(i->value,
-                   BUCKET(ITEMS(i->set)->currentbucket)
-                   ->values[ITEMS(i->set)->currentoffset]);
           INCREF_VALUE(i->value);
 
           i->position ++;


=== ZODB3/BTrees/BTreeTemplate.c 1.71.12.1 => 1.71.12.2 ===
--- ZODB3/BTrees/BTreeTemplate.c:1.71.12.1	Fri Oct  4 13:35:31 2002
+++ ZODB3/BTrees/BTreeTemplate.c	Fri Apr 18 14:08:36 2003
@@ -493,12 +493,14 @@
   UNLESS (b=BTree_lastBucket(self)) goto err;
   if (Bucket_deleteNextBucket(b) < 0) goto err;
 
+  Py_DECREF(b);
   PER_ALLOW_DEACTIVATION(self);
   PER_ACCESSED(self);
 
   return 0;
 
  err:
+  Py_XDECREF(b);
   PER_ALLOW_DEACTIVATION(self);
   return -1;
 }
@@ -755,9 +757,22 @@
 
     /* Remove the child from self->data. */
     Py_DECREF(d->child);
+#ifdef KEY_TYPE_IS_PYOBJECT
     if (min) {
         DECREF_KEY(d->key);
     }
+    else if (self->len > 1) {
+	/* We're deleting the first child of a BTree with more than one
+	 * child.  The key at d+1 is about to be shifted into slot 0,
+	 * and hence never to be referenced again (the key in slot 0 is
+	 * trash).
+	 */
+	DECREF_KEY((d+1)->key);
+    }
+    /* Else min==0 and len==1:  we're emptying the BTree entirely, and
+     * there is no key in need of decrefing.
+     */
+#endif
     --self->len;
     if (min < self->len)
         memmove(d, d+1, (self->len - min) * sizeof(BTreeItem));


=== ZODB3/BTrees/BucketTemplate.c 1.47.12.1 => 1.47.12.2 ===
--- ZODB3/BTrees/BucketTemplate.c:1.47.12.1	Fri Oct  4 13:35:31 2002
+++ ZODB3/BTrees/BucketTemplate.c	Fri Apr 18 14:08:36 2003
@@ -292,17 +292,36 @@
 {
     int i, cmp;
     KEY_TYPE key;
+
+    /* Subtle:  there may or may not be a value.  If there is, we need to
+     * check its type early, so that in case of error we can get out before
+     * mutating the bucket.  But because value isn't used on all paths, if
+     * we don't initialize value then gcc gives a nuisance complaint that
+     * value may be used initialized (it can't be, but gcc doesn't know
+     * that).  So we initialize it.  However, VALUE_TYPE can be various types,
+     * including int, PyObject*, and char[6], so it's a puzzle to spell
+     * initialization.  It so happens that {0} is a valid initializer for all
+     * these types.
+     */
+    VALUE_TYPE value = {0};	/* squash nuisance warning */
     int result = -1;    /* until proven innocent */
     int copied = 1;
 
     COPY_KEY_FROM_ARG(key, keyarg, copied);
     UNLESS(copied) return -1;
 
+    /* Copy the value early (if needed), so that in case of error a
+     * pile of bucket mutations don't need to be undone.
+     */
+    if (v && !noval) {
+    	COPY_VALUE_FROM_ARG(value, v, copied);
+    	UNLESS(copied) return -1;
+    }
+
     PER_USE_OR_RETURN(self, -1);
 
     BUCKET_SEARCH(i, cmp, self, key, goto Done);
     if (cmp == 0) {
-        VALUE_TYPE value;
         /* The key exists, at index i. */
 
         if (v) {
@@ -316,8 +335,6 @@
             }
 
             /* The key exists at index i, and we need to replace the value. */
-            COPY_VALUE_FROM_ARG(value, v, copied);
-            UNLESS(copied) goto Done;
 #ifdef VALUE_SAME
             /* short-circuit if no change */
             if (VALUE_SAME(self->values[i], value)) {
@@ -390,8 +407,7 @@
     INCREF_KEY(self->keys[i]);
 
     if (! noval) {
-        COPY_VALUE_FROM_ARG(self->values[i], v, copied);
-        UNLESS(copied) return -1;
+        COPY_VALUE(self->values[i], value);
         INCREF_VALUE(self->values[i]);
     }