| Message ID | cover.1727774935.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | reftable: handle allocation errors | expand |
Patrick Steinhardt <ps@pks.im> writes: > this is the fourth version of this patch series that converts the > reftable library to start handling allocation errors. This is done such > that the reftable library can truly behave like a library and let its > callers handle such conditions. > > Changes compared to v3: > > - Fix some additional sites where we use strdup(3P)/free(3P) by > accident. > > - Convert preexisting callsites of free(3P) to use reftable_free(). > > - Introduce a REFTABLE_FREE_AND_NULL() macro and use it. > > - Ban standard allocators in reftable code "banned.h"-style. With this patch, there is only one hit from $ git grep '[^_]free(' reftable/ and no hits from $ git grep '[^_]FREE_AND_NULL(' reftable/ which is just as expected. Nicely done. Shouldn't we add FREE_AND_NULL() to the banned list as well in the last step? Thanks.
Am 01.10.24 um 19:52 schrieb Junio C Hamano: > Shouldn't we add FREE_AND_NULL() to the banned list as well in the > last step? And perhaps the wrapper.h functions like xmalloc()? At least as long as git-compat-util.h is included by reftable/system.h. Can be done later, of course, no need to reroll just for that. René
René Scharfe <l.s.r@web.de> writes: > Am 01.10.24 um 19:52 schrieb Junio C Hamano: >> Shouldn't we add FREE_AND_NULL() to the banned list as well in the >> last step? > > And perhaps the wrapper.h functions like xmalloc()? At least as long as > git-compat-util.h is included by reftable/system.h. Can be done later, > of course, no need to reroll just for that. Yeah, and I agree FREE_AND_NULL() falls into the same bucket as xmalloc() and friends, so no need to reroll just for that, either. Thanks.
On Tue, Oct 01, 2024 at 12:25:02PM -0700, Junio C Hamano wrote: > René Scharfe <l.s.r@web.de> writes: > > > Am 01.10.24 um 19:52 schrieb Junio C Hamano: > >> Shouldn't we add FREE_AND_NULL() to the banned list as well in the > >> last step? > > > > And perhaps the wrapper.h functions like xmalloc()? At least as long as > > git-compat-util.h is included by reftable/system.h. Can be done later, > > of course, no need to reroll just for that. > > Yeah, and I agree FREE_AND_NULL() falls into the same bucket as xmalloc() > and friends, so no need to reroll just for that, either. `FREE_AND_NULL()` would already be detected because it expands to code that contains `free()`. Same for `REALLOC_ARRAY()` and the likes. I'm overall a bit more hesitant to also start banning all the Git specific wrappers, mostly because there are so many of them. My goal is to rather make them inaccessible in the "reftable/" code in the first place so that we don't have to play whack-a-mole. My plan here is to split out the POSIX-providing bits from "git-compat-util.h" into a new header "compat/posix.h" that we can include in "reftable/system.h". But it'll take a while to get there :) Thanks for your inputs! Patrick
Patrick Steinhardt <ps@pks.im> writes: > `FREE_AND_NULL()` would already be detected because it expands to code > that contains `free()`. Same for `REALLOC_ARRAY()` and the likes. Ah, that is very true.
Hi, this is the fourth version of this patch series that converts the reftable library to start handling allocation errors. This is done such that the reftable library can truly behave like a library and let its callers handle such conditions. Changes compared to v3: - Fix some additional sites where we use strdup(3P)/free(3P) by accident. - Convert preexisting callsites of free(3P) to use reftable_free(). - Introduce a REFTABLE_FREE_AND_NULL() macro and use it. - Ban standard allocators in reftable code "banned.h"-style. Thanks! Patrick Patrick Steinhardt (25): reftable/error: introduce out-of-memory error code reftable/basics: merge "publicbasics" into "basics" reftable: introduce `reftable_strdup()` reftable/basics: handle allocation failures in `reftable_calloc()` reftable/basics: handle allocation failures in `parse_names()` reftable/record: handle allocation failures on copy reftable/record: handle allocation failures when decoding records reftable/writer: handle allocation failures in `writer_index_hash()` reftable/writer: handle allocation failures in `reftable_new_writer()` reftable/merged: handle allocation failures in `merged_table_init_iter()` reftable/reader: handle allocation failures for unindexed reader reftable/reader: handle allocation failures in `reader_init_iter()` reftable/stack: handle allocation failures on reload reftable/stack: handle allocation failures in `reftable_new_stack()` reftable/stack: handle allocation failures in `stack_compact_range()` reftable/stack: handle allocation failures in auto compaction reftable/iter: handle allocation failures when creating indexed table iter reftable/blocksource: handle allocation failures reftable/block: handle allocation failures reftable/pq: handle allocation failures when adding entries reftable/tree: handle allocation failures reftable: handle trivial allocation failures reftable: fix calls to free(3P) reftable: introduce `REFTABLE_FREE_AND_NULL()` reftable/basics: ban standard allocator functions Makefile | 1 - refs/reftable-backend.c | 39 ++++-- reftable/basics.c | 93 +++++++++++++- reftable/basics.h | 23 +++- reftable/block.c | 29 ++++- reftable/block.h | 4 +- reftable/blocksource.c | 25 +++- reftable/error.c | 2 + reftable/iter.c | 22 +++- reftable/iter.h | 2 +- reftable/merged.c | 84 ++++++++----- reftable/merged.h | 6 +- reftable/pq.c | 9 +- reftable/pq.h | 2 +- reftable/publicbasics.c | 66 ---------- reftable/reader.c | 70 ++++++++--- reftable/reader.h | 6 +- reftable/record.c | 174 +++++++++++++++++++------- reftable/record.h | 6 +- reftable/reftable-basics.h | 18 +++ reftable/reftable-error.h | 3 + reftable/reftable-malloc.h | 18 --- reftable/reftable-merged.h | 8 +- reftable/reftable-reader.h | 8 +- reftable/reftable-stack.h | 8 +- reftable/reftable-writer.h | 12 +- reftable/stack.c | 187 +++++++++++++++++++++------- reftable/tree.c | 42 +++++-- reftable/tree.h | 21 +++- reftable/writer.c | 154 +++++++++++++++-------- t/helper/test-reftable.c | 10 +- t/unit-tests/lib-reftable.c | 8 +- t/unit-tests/t-reftable-basics.c | 11 +- t/unit-tests/t-reftable-block.c | 24 ++-- t/unit-tests/t-reftable-merged.c | 16 ++- t/unit-tests/t-reftable-readwrite.c | 65 ++++++---- t/unit-tests/t-reftable-stack.c | 6 +- t/unit-tests/t-reftable-tree.c | 10 +- 38 files changed, 885 insertions(+), 407 deletions(-) delete mode 100644 reftable/publicbasics.c create mode 100644 reftable/reftable-basics.h delete mode 100644 reftable/reftable-malloc.h Range-diff against v3: 1: 8c99ecc325 = 1: 94eaef3ae5 reftable/error: introduce out-of-memory error code 2: 4dcdf1d48e = 2: fe55051cb7 reftable/basics: merge "publicbasics" into "basics" 3: 21fa9b15d9 = 3: b1a10d41d3 reftable: introduce `reftable_strdup()` 4: f6ad92ffd0 = 4: fd9141e985 reftable/basics: handle allocation failures in `reftable_calloc()` 5: 922783708d ! 5: bdfddbebce reftable/basics: handle allocation failures in `parse_names()` @@ reftable/basics.c: void parse_names(char *buf, int size, char ***namesp) + if (!names) + goto err; + -+ names[names_len] = strdup(p); ++ names[names_len] = reftable_strdup(p); + if (!names[names_len++]) + goto err; } 6: d4676e8a6a = 6: 756a32c285 reftable/record: handle allocation failures on copy 7: 874e6a1c20 = 7: ca64971e7b reftable/record: handle allocation failures when decoding records 8: 42bc57fd00 = 8: fc2f113cba reftable/writer: handle allocation failures in `writer_index_hash()` 9: 9edd1d84cd ! 9: 0ed99e0bdf reftable/writer: handle allocation failures in `reftable_new_writer()` @@ reftable/writer.c: reftable_new_writer(ssize_t (*writer_func)(void *, const void strbuf_init(&wp->last_key, 0); REFTABLE_CALLOC_ARRAY(wp->block, opts.block_size); + if (!wp->block) { -+ free(wp); ++ reftable_free(wp); + return REFTABLE_OUT_OF_MEMORY_ERROR; + } wp->write = writer_func; 10: d4004a7f43 ! 10: 8dfbfd9286 reftable/merged: handle allocation failures in `merged_table_init_iter()` @@ reftable/merged.c: reftable_merged_table_min_update_index(struct reftable_merged + reftable_iterator_destroy(&subiters[i].iter); + reftable_record_release(&subiters[i].rec); + } -+ free(subiters); -+ free(mi); ++ reftable_free(subiters); ++ reftable_free(mi); + } + + return ret; 11: b2bd142021 = 11: 7b592a6c6b reftable/reader: handle allocation failures for unindexed reader 12: 9b5fd52862 = 12: ceed838265 reftable/reader: handle allocation failures in `reader_init_iter()` 13: 0f3e3d1585 = 13: 1e997a5766 reftable/stack: handle allocation failures on reload 14: c88645a251 = 14: 40d4d81378 reftable/stack: handle allocation failures in `reftable_new_stack()` 15: 92ad4fc934 = 15: 6aaae4baa6 reftable/stack: handle allocation failures in `stack_compact_range()` 16: 19acebd919 = 16: 103a59ef0a reftable/stack: handle allocation failures in auto compaction 17: 620658bffc = 17: 6799d299fe reftable/iter: handle allocation failures when creating indexed table iter 18: 48047dd4c9 ! 18: c7e54d71d7 reftable/blocksource: handle allocation failures @@ reftable/blocksource.c: int reftable_block_source_from_file(struct reftable_bloc + if (fd >= 0) + close(fd); + if (err < 0) -+ free(p); ++ reftable_free(p); return 0; } 19: 08685605ba = 19: 92d39b9021 reftable/block: handle allocation failures 20: a66937b7af = 20: 3416004e0d reftable/pq: handle allocation failures when adding entries 21: c291114553 = 21: a363930488 reftable/tree: handle allocation failures 22: 20f50c446a = 22: 28661500ff reftable: handle trivial allocation failures -: ---------- > 23: 228cc81263 reftable: fix calls to free(3P) -: ---------- > 24: 1c66f6ef8d reftable: introduce `REFTABLE_FREE_AND_NULL()` -: ---------- > 25: 764961e6f0 reftable/basics: ban standard allocator functions