Re: [PATCH v4 1/2] refs: introduce reftable backend

From: Jeff King <peff@peff.net>
Date: Feb 7, 2024, 22:31 UTC
Message-ID: <20240207223120.GA537741@coredump.intra.peff.net>
In-Reply-To: <5de60d46bdccbfbf0a923abc2f45eda07f30c110.1707288261.git.ps@pks.im>

On Wed, Feb 07, 2024 at 08:20:31AM +0100, Patrick Steinhardt wrote:

Show 24 quoted lines

> +static int write_copy_table(struct reftable_writer *writer, void *cb_data)
> +{
> [...]
> +	/*
> +	 * Create the reflog entry for the newly created branch.
> +	 */
> +	ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
> +	memset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));
> +	fill_reftable_log_record(&logs[logs_nr]);
> +	logs[logs_nr].refname = (char *)arg->newname;
> +	logs[logs_nr].update_index = creation_ts;
> +	logs[logs_nr].value.update.message =
> +		xstrndup(arg->logmsg, arg->refs->write_options.block_size / 2);
> +	logs[logs_nr].value.update.new_hash = old_ref.value.val1;
> +	logs_nr++;
> +
> +	/*
> +	 * In addition to writing the reflog entry for the new branch, we also
> +	 * copy over all log entries from the old reflog. Last but not least,
> +	 * when renaming we also have to delete all the old reflog entries.
> +	 */
> +	ret = reftable_merged_table_seek_log(mt, &it, arg->oldname);
> +	if (ret < 0)
> +		return ret;

Should this last line be "goto done" as is used elsewhere in the function? Otherwise we are at least leaking the "logs" array (and possibly some of the other cleanup is important, too).

Show 8 quoted lines

> +	while (1) {
> +		ret = reftable_iterator_next_log(&it, &old_log);
> +		if (ret < 0)
> +			goto done;
> +		if (ret > 0 || strcmp(old_log.refname, arg->oldname)) {
> +			ret = 0;
> +			break;
> +		}

This "ret = 0" doesn't have any effect. We break out of the loop, and then...

Show 5 quoted lines

> +	}
> +
> +	ret = reftable_writer_add_logs(writer, logs, logs_nr);
> +	if (ret < 0)
> +		goto done;

...the first thing we do is write over it. I dunno if it's worth keeping as a maintenance precaution, though (if the code after the loop changed to omit that assignment, then setting "ret" would become important).

Both were noticed by Coverity (along with several other false positives).

-Peff