On Wed, Feb 07, 2024 at 05:31:20PM -0500, Jeff King wrote:
> On Wed, Feb 07, 2024 at 08:20:31AM +0100, Patrick Steinhardt wrote:
>
> > +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).
Indeed we should.
> > + 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...
>
> > + }
> > +
> > + 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).
Yeah, I think this one we should keep. It's also a repeating pattern
that we have in many other places, so it helps lower the mental burden
when it looks similar to all the others.
> Both were noticed by Coverity (along with several other false
> positives).
Is the Coverity instance publicly accessible? If not, can you maybe
invite me so that I get a better feeling for them?
I'll send a patch to fix the missing `goto done` above, thanks!
Patrick
Attachment:
signature.asc
Description: PGP signature
