| Message ID | 32fead57de989335b17d16f63c1cd144460495a1.1726489647.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reftable: handle allocation errors | expand |
Patrick Steinhardt <ps@pks.im> writes: > -int new_indexed_table_ref_iter(struct indexed_table_ref_iter **dest, > +int indexed_table_ref_iter_new(struct indexed_table_ref_iter **dest, > struct reftable_reader *r, uint8_t *oid, > int oid_len, uint64_t *offsets, int offset_len) > { > struct indexed_table_ref_iter empty = INDEXED_TABLE_REF_ITER_INIT; > - struct indexed_table_ref_iter *itr = reftable_calloc(1, sizeof(*itr)); > + struct indexed_table_ref_iter *itr; > int err = 0; > > + itr = reftable_calloc(1, sizeof(*itr)); > + if (!itr) { > + err = REFTABLE_OUT_OF_MEMORY_ERROR; > + goto out; > + } > + > *itr = empty; > itr->r = r; > strbuf_add(&itr->oid, oid, oid_len); > @@ -197,11 +203,15 @@ int new_indexed_table_ref_iter(struct indexed_table_ref_iter **dest, > itr->offset_len = offset_len; > > err = indexed_table_ref_iter_next_block(itr); > - if (err < 0) { > + if (err < 0) > + goto out; > + > + *dest = itr; > + err = 0; > + > +out: > + if (err < 0) > reftable_free(itr); > - } else { > - *dest = itr; > - } > return err; > } Unless the service the helper function offers is to upgrade an existing resource (e.g., realloc() taking a pointer and give an enlarged piece of memory), it may be a safer calling convention to promise that *dest is cleared to NULL when the function fails, instead of promising that *dest is left intact. The caller, when it needs to evantually release the resource acquired here, has to remember what the returned value (i.e., err) was, in order to decide if it needs to call the release helper on *dest it obtained from us. The only caller seems to initialize *dest to NULL itself, so it does not matter in the current code, though.
On Sat, Sep 21, 2024 at 11:26:09PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > -int new_indexed_table_ref_iter(struct indexed_table_ref_iter **dest, > > +int indexed_table_ref_iter_new(struct indexed_table_ref_iter **dest, > > struct reftable_reader *r, uint8_t *oid, > > int oid_len, uint64_t *offsets, int offset_len) > > { > > struct indexed_table_ref_iter empty = INDEXED_TABLE_REF_ITER_INIT; > > - struct indexed_table_ref_iter *itr = reftable_calloc(1, sizeof(*itr)); > > + struct indexed_table_ref_iter *itr; > > int err = 0; > > > > + itr = reftable_calloc(1, sizeof(*itr)); > > + if (!itr) { > > + err = REFTABLE_OUT_OF_MEMORY_ERROR; > > + goto out; > > + } > > + > > *itr = empty; > > itr->r = r; > > strbuf_add(&itr->oid, oid, oid_len); > > @@ -197,11 +203,15 @@ int new_indexed_table_ref_iter(struct indexed_table_ref_iter **dest, > > itr->offset_len = offset_len; > > > > err = indexed_table_ref_iter_next_block(itr); > > - if (err < 0) { > > + if (err < 0) > > + goto out; > > + > > + *dest = itr; > > + err = 0; > > + > > +out: > > + if (err < 0) > > reftable_free(itr); > > - } else { > > - *dest = itr; > > - } > > return err; > > } > > Unless the service the helper function offers is to upgrade an > existing resource (e.g., realloc() taking a pointer and give an > enlarged piece of memory), it may be a safer calling convention to > promise that *dest is cleared to NULL when the function fails, > instead of promising that *dest is left intact. The caller, when it > needs to evantually release the resource acquired here, has to > remember what the returned value (i.e., err) was, in order to decide > if it needs to call the release helper on *dest it obtained from us. > > The only caller seems to initialize *dest to NULL itself, so it does > not matter in the current code, though. I don't really see it as much of a problem here, mostly because this function is internal to the reftable library anyway. Also, callers essentially have to NULL-initialize the variable anyway once there are multiple error paths and if they want to free it, because otherwise they could end up freeing a uninitialized pointer. On the other hand it doesn't hurt much to assign `NULL` on the error path either, so I'll just do that. Patrick
diff --git a/reftable/iter.c b/reftable/iter.c index 416a9f6996b..41bdfbb13f9 100644 --- a/reftable/iter.c +++ b/reftable/iter.c @@ -181,14 +181,20 @@ static int indexed_table_ref_iter_next(void *p, struct reftable_record *rec) } } -int new_indexed_table_ref_iter(struct indexed_table_ref_iter **dest, +int indexed_table_ref_iter_new(struct indexed_table_ref_iter **dest, struct reftable_reader *r, uint8_t *oid, int oid_len, uint64_t *offsets, int offset_len) { struct indexed_table_ref_iter empty = INDEXED_TABLE_REF_ITER_INIT; - struct indexed_table_ref_iter *itr = reftable_calloc(1, sizeof(*itr)); + struct indexed_table_ref_iter *itr; int err = 0; + itr = reftable_calloc(1, sizeof(*itr)); + if (!itr) { + err = REFTABLE_OUT_OF_MEMORY_ERROR; + goto out; + } + *itr = empty; itr->r = r; strbuf_add(&itr->oid, oid, oid_len); @@ -197,11 +203,15 @@ int new_indexed_table_ref_iter(struct indexed_table_ref_iter **dest, itr->offset_len = offset_len; err = indexed_table_ref_iter_next_block(itr); - if (err < 0) { + if (err < 0) + goto out; + + *dest = itr; + err = 0; + +out: + if (err < 0) reftable_free(itr); - } else { - *dest = itr; - } return err; } diff --git a/reftable/iter.h b/reftable/iter.h index befc4597df1..b3225bc7add 100644 --- a/reftable/iter.h +++ b/reftable/iter.h @@ -82,7 +82,7 @@ void iterator_from_indexed_table_ref_iter(struct reftable_iterator *it, struct indexed_table_ref_iter *itr); /* Takes ownership of `offsets` */ -int new_indexed_table_ref_iter(struct indexed_table_ref_iter **dest, +int indexed_table_ref_iter_new(struct indexed_table_ref_iter **dest, struct reftable_reader *r, uint8_t *oid, int oid_len, uint64_t *offsets, int offset_len); diff --git a/reftable/reader.c b/reftable/reader.c index f696e992dfc..0179e4e73dd 100644 --- a/reftable/reader.c +++ b/reftable/reader.c @@ -715,7 +715,7 @@ static int reftable_reader_refs_for_indexed(struct reftable_reader *r, goto done; } - err = new_indexed_table_ref_iter(&itr, r, oid, hash_size(r->hash_id), + err = indexed_table_ref_iter_new(&itr, r, oid, hash_size(r->hash_id), got.u.obj.offsets, got.u.obj.offset_len); if (err < 0) @@ -740,6 +740,11 @@ static int reftable_reader_refs_for_unindexed(struct reftable_reader *r, int err; REFTABLE_ALLOC_ARRAY(ti, 1); + if (!ti) { + err = REFTABLE_OUT_OF_MEMORY_ERROR; + goto out; + } + table_iter_init(ti, r); err = table_iter_seek_start(ti, BLOCK_TYPE_REF, 0); if (err < 0)
Handle allocation failures in `new_indexed_table_ref_iter()`. While at it, rename the function to match our coding style. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- reftable/iter.c | 22 ++++++++++++++++------ reftable/iter.h | 2 +- reftable/reader.c | 7 ++++++- 3 files changed, 23 insertions(+), 8 deletions(-)