Rev 4708: (jam) Bug #490228, in file:///home/pqm/archives/thelove/bzr/2.0/

[Canonical.com Patch Queue Manager]

At file:///home/pqm/archives/thelove/bzr/2.0/

------------------------------------------------------------
revno: 4708 [merge]
revision-id: pqm at pqm.ubuntu.com-20091214192908-ipr80rfv0kd5h5o6
parent: pqm at pqm.ubuntu.com-20091202024309-7eblqn2luitz3tzs
parent: john at arbash-meinel.com-20091214155442-tm43um6g8xofx4rs
committer: Canonical.com Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: 2.0
timestamp: Mon 2009-12-14 19:29:08 +0000
message:
  (jam) Bug #490228,
  	rare segfault from accessing one past the end of an array.
modified:
  NEWS                           NEWS-20050323055033-4e00b5db738777ff
  bzrlib/diff-delta.c            diffdelta.c-20090226042143-l9wzxynyuxnb5hus-1
=== modified file 'NEWS'
--- a/NEWS	2009-12-02 01:30:35 +0000
+++ b/NEWS	2009-12-14 15:52:24 +0000
@@ -27,6 +27,11 @@
 * Content filters are now applied correctly after pull, merge and switch.
   (Ian Clatworthy, #385879)
 
+* Fix a potential segfault in the groupcompress hash map handling code.
+  When inserting new entries, if the final hash bucket was empty, we could
+  end up trying to access if ``(last_entry+1)->ptr == NULL``.
+  (John Arbash Meinel, #490228)
+
 * Improve "Binary files differ" hunk handling.  (Aaron Bentley, #436325)
 
 Improvements

=== modified file 'bzrlib/diff-delta.c'
--- a/bzrlib/diff-delta.c	2009-08-03 16:54:36 +0000
+++ b/bzrlib/diff-delta.c	2009-12-14 15:54:42 +0000
@@ -688,7 +688,7 @@
     const unsigned char *data, *buffer, *top;
     unsigned char cmd;
     struct delta_index *new_index;
-    struct index_entry *entry, *entries, *old_entry;
+    struct index_entry *entry, *entries;
 
     if (!src->buf || !src->size)
         return NULL;
@@ -789,6 +789,7 @@
     entry = entries;
     num_inserted = 0;
     for (; num_entries > 0; --num_entries, ++entry) {
+        struct index_entry *next_bucket_entry, *cur_entry, *bucket_first_entry;
         hash_offset = (entry->val & old_index->hash_mask);
         /* The basic structure is a hash => packed_entries that fit in that
          * hash bucket. Things are structured such that the hash-pointers are
@@ -797,15 +798,19 @@
          * forward. If there are no NULL targets, then we know because
          * entry->ptr will not be NULL.
          */
-        old_entry = old_index->hash[hash_offset + 1];
-        old_entry--;
-        while (old_entry->ptr == NULL
-               && old_entry >= old_index->hash[hash_offset]) {
-            old_entry--;
+        // The start of the next bucket, this may point past the end of the
+        // entry table if hash_offset is the last bucket.
+        next_bucket_entry = old_index->hash[hash_offset + 1];
+        // First entry in this bucket
+        bucket_first_entry = old_index->hash[hash_offset];
+        cur_entry = next_bucket_entry - 1;
+        while (cur_entry->ptr == NULL && cur_entry >= bucket_first_entry) {
+            cur_entry--;
         }
-        old_entry++;
-        if (old_entry->ptr != NULL
-            || old_entry >= old_index->hash[hash_offset + 1]) {
+        // cur_entry now either points at the first NULL, or it points to
+        // next_bucket_entry if there were no blank spots.
+        cur_entry++;
+        if (cur_entry >= next_bucket_entry || cur_entry->ptr != NULL) {
             /* There is no room for this entry, we have to resize */
             // char buff[128];
             // get_text(buff, entry->ptr);
@@ -822,7 +827,7 @@
             break;
         }
         num_inserted++;
-        *old_entry = *entry;
+        *cur_entry = *entry;
         /* For entries which we *do* manage to insert into old_index, we don't
          * want them double copied into the final output.
          */