| Message ID | f16175295f5c786fea2d56ebffc2b9a6beb07aa0.1715716605.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f25e1f2a4d48c6d8bfd659338d4415c7ef4df533 |
| Headers | show |
| Series | pack-bitmap: various pack-bitmap-write cleanups | expand |
On Tue, May 14, 2024 at 03:57:03PM -0400, Taylor Blau wrote: > Prepare to free() memory associated with bitmapped_commit structs by > zero'ing the 'write_as' field. > > In ideal cases, it is fine to do something like: > > for (i = 0; i < writer->selected_nr; i++) { > struct bitmapped_commit *bc = &writer->selected[i]; > if (bc->write_as != bc->bitmap) > ewah_free(bc->write_as); > ewah_free(bc->bitmap); > } > > but if not all of the 'write_as' fields were populated (e.g., because > the packing_data given does not form a reachability closure), then we > may attempt to free uninitialized memory. > > Guard against this by preemptively zero'ing this field just in case. > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > pack-bitmap-write.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c > index c0087dab12..420f17c2e0 100644 > --- a/pack-bitmap-write.c > +++ b/pack-bitmap-write.c > @@ -112,6 +112,7 @@ static inline void push_bitmapped_commit(struct bitmap_writer *writer, > > writer->selected[writer->selected_nr].commit = commit; > writer->selected[writer->selected_nr].bitmap = NULL; > + writer->selected[writer->selected_nr].write_as = NULL; > writer->selected[writer->selected_nr].flags = 0; Instead of having to ensure that all fields are initialized we could also set the whole structure to zero via `memset()`, which might be a bit more sustainable in the future. That alone doesn't really warrant a reroll though. Patrick
On Wed, May 15, 2024 at 11:05:10AM +0200, Patrick Steinhardt wrote: > > diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c > > index c0087dab12..420f17c2e0 100644 > > --- a/pack-bitmap-write.c > > +++ b/pack-bitmap-write.c > > @@ -112,6 +112,7 @@ static inline void push_bitmapped_commit(struct bitmap_writer *writer, > > > > writer->selected[writer->selected_nr].commit = commit; > > writer->selected[writer->selected_nr].bitmap = NULL; > > + writer->selected[writer->selected_nr].write_as = NULL; > > writer->selected[writer->selected_nr].flags = 0; > > Instead of having to ensure that all fields are initialized we could > also set the whole structure to zero via `memset()`, which might be a > bit more sustainable in the future. That alone doesn't really warrant a > reroll though. I had considered it, but decided against it as it seemed wasteful to memset(&writer->selected[writer->selected_nr], 0, sizeof(struct bitmapped_commit)) and then immediately un-zero some of its memory by assigning the commit field. Obviously not actually all that wasteful, as we're only talking about a couple of hundred CPU cycles ;-). Thanks, Taylor
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c index c0087dab12..420f17c2e0 100644 --- a/pack-bitmap-write.c +++ b/pack-bitmap-write.c @@ -112,6 +112,7 @@ static inline void push_bitmapped_commit(struct bitmap_writer *writer, writer->selected[writer->selected_nr].commit = commit; writer->selected[writer->selected_nr].bitmap = NULL; + writer->selected[writer->selected_nr].write_as = NULL; writer->selected[writer->selected_nr].flags = 0; writer->selected_nr++;
Prepare to free() memory associated with bitmapped_commit structs by zero'ing the 'write_as' field. In ideal cases, it is fine to do something like: for (i = 0; i < writer->selected_nr; i++) { struct bitmapped_commit *bc = &writer->selected[i]; if (bc->write_as != bc->bitmap) ewah_free(bc->write_as); ewah_free(bc->bitmap); } but if not all of the 'write_as' fields were populated (e.g., because the packing_data given does not form a reachability closure), then we may attempt to free uninitialized memory. Guard against this by preemptively zero'ing this field just in case. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- pack-bitmap-write.c | 1 + 1 file changed, 1 insertion(+)