| Message ID | 20181104004957.52913-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | multi-pack-index: make code -Wunused-parameter clean | expand |
On Sat, Nov 03, 2018 at 05:49:57PM -0700, Carlo Marcelo Arenas Belón wrote: > 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 <peff@peff.net> --- midx.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/midx.c b/midx.c index 4fac0cd08a..ecd583666a 100644 --- a/midx.c +++ b/midx.c @@ -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;
On 11/3/2018 10:27 PM, Jeff King wrote: > On Sat, Nov 03, 2018 at 05:49:57PM -0700, Carlo Marcelo Arenas Belón wrote: > >> 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 <peff@peff.net> Thanks, both, for catching this. I prefer the approach that adds defenses. Reviewed-by: Derrick Stolee <dstolee@microsoft.com> > --- > midx.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/midx.c b/midx.c > index 4fac0cd08a..ecd583666a 100644 > --- a/midx.c > +++ b/midx.c > @@ -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;
diff --git a/midx.c b/midx.c index 4fac0cd08a..a2c17e3108 100644 --- a/midx.c +++ b/midx.c @@ -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:
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 <carenas@gmail.com> --- midx.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)