Bazaar

Merge lp:~jameinel/bzr/2.0-490228-gc-segfault into lp:bzr/2.0

  1. 2.0-490228-gc-segfault
  2. Merge into 2.0
Proposed by John A Meinel
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~jameinel/bzr/2.0-490228-gc-segfault
Merge into: lp:bzr/2.0
Diff against target: 73 lines (+20/-10)
2 files modified
NEWS (+5/-0)
bzrlib/diff-delta.c (+15/-10)
To merge this branch: bzr merge lp:~jameinel/bzr/2.0-490228-gc-segfault
Reviewer Review Type Date Requested Status
Vincent Ladeuil Approve
Review via email: mp+16140@code.launchpad.net
To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

See the mp here:
https://code.edge.launchpad.net/~jameinel/bzr/2.0-490228-gc-segfault/+merge/16139

I meant to submit this for 2.0 inclusion, and I just forgot to mark it as such.

Revision history for this message
Vincent Ladeuil (vila) wrote :

Unless you can demonstrate that you don't include regression here, that clearly a controversial bug
to land.

My feeling here is that we should rely on your judgment (as I mentioned in the related thread
on the bazaar list).

So, since you're still RM for 2.0.3 and you feel comfortable landing this, go ahead !

review: Approve
Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> Review: Approve
> Unless you can demonstrate that you don't include regression here, that clearly a controversial bug
> to land.
>
> My feeling here is that we should rely on your judgment (as I mentioned in the related thread
> on the bazaar list).

I'm quite confident it isn't going to introduce a regression. I did a
fairly careful analysis of the code and structures (since I didn't have
test cases to rely on :).

A just-as-correct-but-more-controversial fix would have been to remove
the '->ptr == NULL' check. Specifically,

if (cur_entry == next_bucket_entry) {

The data structures *should* work such that if there isn't a slot to
hold the new pointer, cur_entry == next_bucket_entry after the while
loop. I was being conservative by leaving the rest of the structures alone.

>
> So, since you're still RM for 2.0.3 and you feel comfortable landing this, go ahead !

If you look at the bug report, you'll see the analysis I did. I'll land
it in 2.0.*

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAksmcr0ACgkQJdeBCYSNAANVuQCgmS+waehHRvbZ08thk1LKs+Qc
mXMAoMlH2cwwCtIqk/baLPiHDErzJchT
=5nzA
-----END PGP SIGNATURE-----

Revision history for this message
Vincent Ladeuil (vila) wrote :

>>>>> "jam" == John A Meinel <email address hidden> writes:

<snip/>

    jam> If you look at the bug report, you'll see the analysis I
    jam> did.

I did look and the analysis as well as the commit message
are... great. You did a really good job there for sure.

       Vincent

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-12-02 01:30:35 +0000
3+++ NEWS 2009-12-14 16:02:17 +0000
4@@ -27,6 +27,11 @@
5 * Content filters are now applied correctly after pull, merge and switch.
6 (Ian Clatworthy, #385879)
7
8+* Fix a potential segfault in the groupcompress hash map handling code.
9+ When inserting new entries, if the final hash bucket was empty, we could
10+ end up trying to access if ``(last_entry+1)->ptr == NULL``.
11+ (John Arbash Meinel, #490228)
12+
13 * Improve "Binary files differ" hunk handling. (Aaron Bentley, #436325)
14
15 Improvements
16
17=== modified file 'bzrlib/diff-delta.c'
18--- bzrlib/diff-delta.c 2009-08-03 16:54:36 +0000
19+++ bzrlib/diff-delta.c 2009-12-14 16:02:17 +0000
20@@ -688,7 +688,7 @@
21 const unsigned char *data, *buffer, *top;
22 unsigned char cmd;
23 struct delta_index *new_index;
24- struct index_entry *entry, *entries, *old_entry;
25+ struct index_entry *entry, *entries;
26
27 if (!src->buf || !src->size)
28 return NULL;
29@@ -789,6 +789,7 @@
30 entry = entries;
31 num_inserted = 0;
32 for (; num_entries > 0; --num_entries, ++entry) {
33+ struct index_entry *next_bucket_entry, *cur_entry, *bucket_first_entry;
34 hash_offset = (entry->val & old_index->hash_mask);
35 /* The basic structure is a hash => packed_entries that fit in that
36 * hash bucket. Things are structured such that the hash-pointers are
37@@ -797,15 +798,19 @@
38 * forward. If there are no NULL targets, then we know because
39 * entry->ptr will not be NULL.
40 */
41- old_entry = old_index->hash[hash_offset + 1];
42- old_entry--;
43- while (old_entry->ptr == NULL
44- && old_entry >= old_index->hash[hash_offset]) {
45- old_entry--;
46+ // The start of the next bucket, this may point past the end of the
47+ // entry table if hash_offset is the last bucket.
48+ next_bucket_entry = old_index->hash[hash_offset + 1];
49+ // First entry in this bucket
50+ bucket_first_entry = old_index->hash[hash_offset];
51+ cur_entry = next_bucket_entry - 1;
52+ while (cur_entry->ptr == NULL && cur_entry >= bucket_first_entry) {
53+ cur_entry--;
54 }
55- old_entry++;
56- if (old_entry->ptr != NULL
57- || old_entry >= old_index->hash[hash_offset + 1]) {
58+ // cur_entry now either points at the first NULL, or it points to
59+ // next_bucket_entry if there were no blank spots.
60+ cur_entry++;
61+ if (cur_entry >= next_bucket_entry || cur_entry->ptr != NULL) {
62 /* There is no room for this entry, we have to resize */
63 // char buff[128];
64 // get_text(buff, entry->ptr);
65@@ -822,7 +827,7 @@
66 break;
67 }
68 num_inserted++;
69- *old_entry = *entry;
70+ *cur_entry = *entry;
71 /* For entries which we *do* manage to insert into old_index, we don't
72 * want them double copied into the final output.
73 */

Subscribers

People subscribed via source and target branches