Subject: Re: [PATCH v3 09/22] reftable/writer: handle allocation failures in `reftable_new_writer()`

From: Patrick Steinhardt <[email-masked]>
Date: Mon, 30 Sep 2024 20:22:47 +0200

On Mon, Sep 30, 2024 at 07:40:48PM +0200, René Scharfe wrote:
> Am 30.09.24 um 10:08 schrieb Patrick Steinhardt:
> > diff --git a/reftable/writer.c b/reftable/writer.c
> > index ed61aaf59c..54ec822e1c 100644
> > --- a/reftable/writer.c
> > +++ b/reftable/writer.c
> > @@ -117,13 +117,17 @@ static void writer_reinit_block_writer(struct reftable_writer *w, uint8_t typ)
> >  	w->block_writer->restart_interval = w->opts.restart_interval;
> >  }
> >
> > -struct reftable_writer *
> > -reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
> > -		    int (*flush_func)(void *),
> > -		    void *writer_arg, const struct reftable_write_options *_opts)
> > +int reftable_writer_new(struct reftable_writer **out,
> > +			ssize_t (*writer_func)(void *, const void *, size_t),
> > +			int (*flush_func)(void *),
> > +			void *writer_arg, const struct reftable_write_options *_opts)
> >  {
> > -	struct reftable_writer *wp = reftable_calloc(1, sizeof(*wp));
> >  	struct reftable_write_options opts = {0};
> > +	struct reftable_writer *wp;
> > +
> > +	wp = reftable_calloc(1, sizeof(*wp));
> > +	if (!wp)
> > +		return REFTABLE_OUT_OF_MEMORY_ERROR;
> >
> >  	if (_opts)
> >  		opts = *_opts;
> > @@ -134,13 +138,19 @@ reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
> >  	strbuf_init(&wp->block_writer_data.last_key, 0);
> >  	strbuf_init(&wp->last_key, 0);
> >  	REFTABLE_CALLOC_ARRAY(wp->block, opts.block_size);
> > +	if (!wp->block) {
> > +		free(wp);
> 
> Better use reftable_free() to free it, since you use reftable_calloc()
> to allocate it above.
> 
> Perhaps ban free(3), strdup(3) etc. at the end of reftable/basics.h,
> banned.h style?

Ugh. I was relying too much on your review having mentioned all cases,
but I really should've double-checked the other patches, too. Mind you,
I really don't mean to blame you here, I blame myself.

In any case, banning these functions via "reftable/basics.h" certainly
seems like a good idea. It's just too easy to screw up by accident. Will
fix tomorrow.

Thanks!

Patrick