Skip to contentSkip to conversation

multi-pack-index: make code -Wunused-parameter clean

PATCH In review Carlo Marcelo Arenas Belón 3 participants 3 messages Nov 4, 2018 – Nov 5, 2018

lore
Share & tools
lore

Carlo Marcelo Arenas Belón

author
#0 lore raw

PATCH 1/1 multi-pack-index: make code -Wunused-parameter clean

introduced in 662148c435 ("midx: write object offsets", 2018-07-12) but included on all previous versions as well.

midx.c:713:54: warning: unused parameter 'nr_objects' [-Wunused-parameter]

likely an oversight as the information needed to iterate over is embedded in nr_large_offset

Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>
---
 midx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
midx.c +2 -2
index 4fac0cd08a..a2c17e3108 100644
@@ -710,7 +710,7 @@ static size_t write_midx_object_offsets(struct hashfile *f, int large_offset_nee
 }
 
 static size_t write_midx_large_offsets(struct hashfile *f, uint32_t nr_large_offset,
-				       struct pack_midx_entry *objects, uint32_t nr_objects)
+				       struct pack_midx_entry *objects)
 {
 	struct pack_midx_entry *list = objects;
 	size_t written = 0;

@@ -880,7 +880,7 @@ int write_midx_file(const char *object_dir)
 				break;
 
 			case MIDX_CHUNKID_LARGEOFFSETS:
-				written += write_midx_large_offsets(f, num_large_offsets, entries, nr_entries);
+				written += write_midx_large_offsets(f, num_large_offsets, entries);
 				break;
 
 			default:
Signature · 1 line
2.19.1.816.gcd69ec8cd
#1 lore raw
Quoted 7 lines · Carlo Marcelo Arenas Belón

introduced in 662148c435 ("midx: write object offsets", 2018-07-12) but included on all previous versions as well.

midx.c:713:54: warning: unused parameter 'nr_objects' [-Wunused-parameter]

likely an oversight as the information needed to iterate over is embedded in nr_large_offset

I've been preparing a series to make the whole code base compile with -Wunused-parameter, and I handled this case a bit differently.

-- >8 --
Subject: [PATCH] midx: double-check large object write loop

The write_midx_large_offsets() function takes an array of object entries, the number of entries in the array (nr_objects), and the number of entries with large offsets (nr_large_offset). But we never actually use nr_objects; instead we keep walking down the array and counting down nr_large_offset until we've seen all of the large entries.

This is correct, but we can be a bit more defensive. If there were ever a mismatch between nr_large_offset and the actual set of large-offset objects, we'd walk off the end of the array.

Since we know the size of the array, we can use nr_objects to make sure we don't walk too far.

Signed-off-by Jeff King <[email protected]>
---
 midx.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
midx.c +9 -3
index 4fac0cd08a..ecd583666a 100644
@@ -712,12 +712,18 @@ static size_t write_midx_object_offsets(struct hashfile *f, int large_offset_nee
 static size_t write_midx_large_offsets(struct hashfile *f, uint32_t nr_large_offset,
 				       struct pack_midx_entry *objects, uint32_t nr_objects)
 {
-	struct pack_midx_entry *list = objects;
+	struct pack_midx_entry *list = objects, *end = objects + nr_objects;
 	size_t written = 0;
 
 	while (nr_large_offset) {
-		struct pack_midx_entry *obj = list++;
-		uint64_t offset = obj->offset;
+		struct pack_midx_entry *obj;
+		uint64_t offset;
+
+		if (list >= end)
+			BUG("too many large-offset objects");
+
+		obj = list++;
+		offset = obj->offset;
 
 		if (!(offset >> 31))
 			continue;
Signature · 1 line
2.19.1.1352.g60f3b1a4c2
Reviewed-by
#2 lore raw
Quoted 21 lines · Jeff King
Quoted 7 lines · Carlo Marcelo Arenas Belón

introduced in 662148c435 ("midx: write object offsets", 2018-07-12) but included on all previous versions as well.

midx.c:713:54: warning: unused parameter 'nr_objects' [-Wunused-parameter]

likely an oversight as the information needed to iterate over is embedded in nr_large_offset

I've been preparing a series to make the whole code base compile with -Wunused-parameter, and I handled this case a bit differently.

-- >8 --
Subject: [PATCH] midx: double-check large object write loop

The write_midx_large_offsets() function takes an array of object entries, the number of entries in the array (nr_objects), and the number of entries with large offsets (nr_large_offset). But we never actually use nr_objects; instead we keep walking down the array and counting down nr_large_offset until we've seen all of the large entries.

Quoted 8 lines

This is correct, but we can be a bit more defensive. If there were ever a mismatch between nr_large_offset and the actual set of large-offset objects, we'd walk off the end of the array.

Since we know the size of the array, we can use nr_objects to make sure we don't walk too far.

Signed-off-by Jeff King <[email protected]>

Thanks, both, for catching this. I prefer the approach that adds defenses.

Reviewed-by Derrick Stolee <[email protected]>
Quoted 30 lines
---
  midx.c | 12 +++++++++---
  1 file changed, 9 insertions(+), 3 deletions(-)
midx.c +9 -3
index 4fac0cd08a..ecd583666a 100644
@@ -712,12 +712,18 @@ static size_t write_midx_object_offsets(struct hashfile *f, int large_offset_nee
  static size_t write_midx_large_offsets(struct hashfile *f, uint32_t nr_large_offset,
  				       struct pack_midx_entry *objects, uint32_t nr_objects)
  {
-	struct pack_midx_entry *list = objects;
+	struct pack_midx_entry *list = objects, *end = objects + nr_objects;
  	size_t written = 0;
 
  	while (nr_large_offset) {
-		struct pack_midx_entry *obj = list++;
-		uint64_t offset = obj->offset;
+		struct pack_midx_entry *obj;
+		uint64_t offset;
+
+		if (list >= end)
+			BUG("too many large-offset objects");
+
+		obj = list++;
+		offset = obj->offset;
 
  		if (!(offset >> 31))
  			continue;