Re: [PATCH v2 2/2] sequencer: fix quoting in write_author_script

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Jul 31, 2018 at 7:15 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
> Single quotes should be escaped as \' not \\'. Note that this only
> affects authors that contain a single quote and then only external
> scripts that read the author script and users whose git is upgraded from
> the shell version of rebase -i while rebase was stopped. This is because
> the parsing in read_env_script() expected the broken version and for
> some reason sq_dequote() called by read_author_ident() seems to handle
> the broken quoting correctly.

Is the:

    ...for some reason sq_dequote() called by read_author_ident()
    seems to handle the broken quoting correctly.

bit outdated? We know now from patch 2/4 of my series[1] that
read_author_ident() wasn't handling it correctly at all. It was merely
ignoring the return value from sq_dequote() and using whatever broken
value came back from it.

[1]: https://public-inbox.org/git/20180731073331.40007-3-sunshine@sunshineco.com/

> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -664,14 +664,25 @@ static int write_author_script(const char *message)
>  static int read_env_script(struct argv_array *env)
>  {
>         if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
>                 return -1;

This is not a problem introduced by this patch, but since
strbuf_read_file() doesn't guarantee that memory hasn't been allocated
when it returns an error, this is leaking.

> +       /*
> +        * write_author_script() used to fail to terminate the GIT_AUTHOR_DATE
> +        * line with a "'" and also escaped "'" incorrectly as "'\\\\''" rather
> +        * than "'\\''". We check for the terminating "'" on the last line to
> +        * see how "'" has been escaped in case git was upgraded while rebase
> +        * was stopped.
> +        */
> +       sq_bug = script.len && script.buf[script.len - 2] != '\'';

I think you need to be checking 'script.len > 1', not just
'script.len', otherwise you might access memory outside the allocated
buffer.

This is a very "delicate" check, assuming that a hand-edited file
won't end with, say, an extra newline. I wonder if this level of
backward-compatibility is overkill for such an unlikely case.

>         for (p = script.buf; *p; p++)
> -               if (skip_prefix(p, "'\\\\''", (const char **)&p2))
> +               if (sq_bug && skip_prefix(p, "'\\\\''", &p2))
> +                       strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
> +               else if (skip_prefix(p, "'\\''", &p2))
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> @@ -75,6 +75,22 @@ test_expect_success 'rebase --keep-empty' '
> +test_expect_success 'rebase -i writes correct author-script' '
> +       test_when_finished "test_might_fail git rebase --abort" &&
> +       git checkout -b author-with-sq master &&
> +       GIT_AUTHOR_NAME="Auth O$SQ R" git commit --allow-empty -m with-sq &&
> +       set_fake_editor &&
> +       FAKE_LINES="edit 1" git rebase -ki HEAD^ &&

Hmph, -k doesn't seem to be documented in git-rebase.txt. Is it needed here?