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

[Phillip Wood <phillip.wood@talktalk.net>]

On 31/07/18 22:39, Eric Sunshine wrote:
> 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.

I think I'll get rid of the check and instead use a version number 
written to .git/rebase-merge/interactive to indicate if we need to fix 
the quoting (if there's no number then it needs fixing). We can 
increment the version number in the future if we ever need to implement 
other fallbacks to handle the case where git got upgraded while rebase 
was stopped. I'll send a patch tomorrow

Best Wishes

Phillip

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