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

> 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?
>