Re: [PATCH] Ensure __BYTE_ORDER is always set

From

Jeff King <peff@peff.net>

Date

Jan 30, 2014, 21:50 UTC

Message-ID

<20140130215002.GB1130@sigill.intra.peff.net>

In-Reply-To

<20140130204538.GA1130@sigill.intra.peff.net>

On Thu, Jan 30, 2014 at 03:45:38PM -0500, Jeff King wrote:

> Either way, we should perhaps be more careful in the bitmap code, too,
> that the values we get are sensible. It's better to die("your bitmap is
> broken") than to read off the end of the array. I can't seem to trigger
> the same failure mode, though. On my x86 system, turning off the
> endian-swap (i.e., the opposite of what should happen) makes t5310 fail,
> but it is because we end up trying to set the bit very far into a
> dynamic bitfield, and die allocating memory.

  1. When we expand the ewah, it has the same number of bits we claimed
     in the on-disk header.

  2. The ewah header matches the number of objects in the packfile.

diff --git a/ewah/ewah_bitmap.c b/ewah/ewah_bitmap.c
index 9ced2da..a8f77cf 100644
--- a/ewah/ewah_bitmap.c
+++ b/ewah/ewah_bitmap.c
@@ -343,6 +343,18 @@ int ewah_iterator_next(eword_t *next, struct ewah_iterator *it)
 	if (it->pointer >= it->buffer_size)
 		return 0;
 
+	/*
+	 * If we return more bits than the ewah advertised, then either
+	 * our data bits or the bit_size field was corrupted, and we
+	 * risk a caller overwriting their own buffer (if they used
+	 * bit_size to size their buffer in the first place).
+	 *
+	 * We don't have a good way of returning an error here, so let's
+	 * just die.
+	 */
+	if (!it->words_remaining--)
+		die("ewah bitmap contains more bits than it claims");
+
 	if (it->compressed < it->rl) {
 		it->compressed++;
 		*next = it->b ? (eword_t)(~0) : 0;
@@ -371,6 +383,8 @@ void ewah_iterator_init(struct ewah_iterator *it, struct ewah_bitmap *parent)
 	it->buffer_size = parent->buffer_size;
 	it->pointer = 0;
 
+	it->words_remaining = (parent->bit_size + 63) / 64;
+
 	it->lw = 0;
 	it->rl = 0;
 	it->compressed = 0;
diff --git a/ewah/ewok.h b/ewah/ewok.h
index 43adeb5..a3f49de 100644
--- a/ewah/ewok.h
+++ b/ewah/ewok.h
@@ -144,6 +144,7 @@ struct ewah_iterator {
 	size_t buffer_size;
 
 	size_t pointer;
+	size_t words_remaining;
 	eword_t compressed, literals;
 	eword_t rl, lw;
 	int b;
diff --git a/pack-bitmap.c b/pack-bitmap.c
index ae0b57b..a31e529 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -118,6 +118,7 @@ static struct ewah_bitmap *lookup_stored_bitmap(struct stored_bitmap *st)
  */
 static struct ewah_bitmap *read_bitmap_1(struct bitmap_index *index)
 {
+	size_t expected_bits;
 	struct ewah_bitmap *b = ewah_pool_new();
 
 	int bitmap_size = ewah_read_mmap(b,
@@ -130,6 +131,31 @@ static struct ewah_bitmap *read_bitmap_1(struct bitmap_index *index)
 		return NULL;
 	}
 
+	/*
+	 * It's OK for us to have too fewer bits than objects, as the EWAH
+	 * writer may have simply left off an ending that is all-zeroes.
+	 *
+	 * However it's not OK for us to have too many bits, as that would
+	 * entail touching objects that we don't have. We are careful
+	 * enough to avoid doing so in later code, but in the case of
+	 * nonsensical values, we would want to avoid even allocating
+	 * memory to hold the expanded bitmap.
+	 *
+	 * There is one exception: we may "go over" to round up to the next
+	 * 64-bit ewah word, since the storage comes in chunks of that size.
+	 */
+	expected_bits = index->pack->num_objects;
+	if (expected_bits & 63) {
+		expected_bits &= ~63;
+		expected_bits += 64;
+	}
+	if (b->bit_size > expected_bits) {
+		error("unexpected number of bits in bitmap: %"PRIuMAX" > %"PRIuMAX,
+		      (uintmax_t)b->bit_size, (uintmax_t)expected_bits);
+		ewah_pool_free(b);
+		return NULL;
+	}
+
 	index->map_pos += bitmap_size;
 	return b;
 }