From: Phillip Wood Date: Tue, 31 Jul 2018 10:02:25 GMT Subject: Re: [PATCH v2 4/4] sequencer: don't die() on bogus user-edited timestamp Message-ID: <71924f65-1890-0460-55a1-987354807c7d@talktalk.net> In-Reply-To: <20180731073331.40007-5-sunshine@sunshineco.com> On 31/07/18 08:33, Eric Sunshine wrote: > read_author_ident() is careful to handle errors "gently" when parsing > "rebase-merge/author-script" by printing a suitable warning and > returning NULL; it never die()'s. One possible reason that parsing might > fail is that "rebase-merge/author-script" has been hand-edited in such a > way which corrupts it or the information it contains. > > However, read_author_ident() invokes fmt_ident() which is not so careful > about failing "gently". It will die() if it encounters a malformed > timestamp. Since read_author_ident() doesn't want to die() and since > it's dealing with possibly hand-edited data, take care to avoid passing > a bogus timestamp to fmt_ident(). > > A more "correctly engineered" fix would be to add a "gentle" version of > fmt_ident(), however, such a change it outside the scope of the bug-fix > series. If fmt_ident() ever does grow a "gentle" cousin, then the manual > timestamp check added here can be retired. > > Signed-off-by: Eric Sunshine > --- > sequencer.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/sequencer.c b/sequencer.c > index 15a66a334c..9b6cdb6ff8 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -742,6 +742,15 @@ static const char *read_author_ident(struct strbuf *buf) > return NULL; > } > > + /* validate date since fmt_ident() will die() on bad value */ > + if (parse_date(val[2], &out)){ > + warning(_("invalid date format '%s' in '%s'"), > + val[2], rebase_path_author_script()); > + strbuf_release(&out); > + return NULL; > + } I think if you're going to do this then the caller needs to be changed to treat NULL as an error > + strbuf_reset(&out); > strbuf_addstr(&out, fmt_ident(val[0], val[1], val[2], 0)); > strbuf_swap(buf, &out); > strbuf_release(&out); >