Re: [PATCH] sequencer: detect author name errors in read_author_script()

Jeff King [off-list ref] writes:

On Sun, Oct 02, 2022 at 11:39:16PM -0700, Michael V. Scovetta wrote:

quoted

In commit 2a7d63a2, sequencer.c:912 looks like:
912  if (name_i == -2)
913      error(_("missing 'GIT_AUTHOR_NAME'"));
914  if (email_i == -2)
915      error(_("missing 'GIT_AUTHOR_EMAIL'"));
916  if (date_i == -2)
917      error(_("missing 'GIT_AUTHOR_DATE'"));
918  if (date_i < 0 || email_i < 0 || date_i < 0 || err)    <-- date_i
is referenced here twice
919      goto finish;

I'm fairly sure that line 918 should be:
918  if (name_i < 0 || email_i < 0 || date_i < 0 || err)

Agreed, but +cc Phillip as the original author.

quoted

I haven't validated this, but I suspect that if
`rebase-merge/author-script` contained two GIT_AUTHOR_NAME fields,
then name_i would be set to -1 (by the error function), but control
wouldn't flow to finish, but instead to line 920 ( *name =
kv.items[name_i].util; ) where it would attempt to access memory
slightly outside of items' memory space.

Correct. It also happens if GIT_AUTHOR_NAME is missing.

quoted

I haven't been able to actually trigger the bug, but strongly suspect
I'm just not familiar enough with how rebasing works under the covers.

It's a little tricky, because we avoid writing and reading the
author-script file unless necessary. An easy way to need it is to break
with a conflict (which writes it), and then resume with "git rebase
--continue" (which reads it back while committing).

Here's a patch to fix it. Thanks for your report!

And thanks for your fix ;-)