| Message ID | 20220902175902.22346-1-oystwa@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f20b9c36d034fe37715d18ef67183cf5544227a7 |
| Headers | show |
| Series | rev-parse --parseopt: detect missing opt-spec | expand |
Damnit, forgot the v3. Hope that's okay. Øsse
Øystein Walle <oystwa@gmail.com> writes: > Damnit, forgot the v3. Hope that's okay. > > Øsse Sure, and thanks.
On Fri, Sep 02, 2022 at 07:59:02PM +0200, Øystein Walle wrote: > After 2d893dff4c (rev-parse --parseopt: allow [*=?!] in argument hints, > 2015-07-14) Oh, no, that's not the first bad commit! The segfault started earlier, with commit 9bab5b6061 (rev-parse --parseopt: option argument name hints, 2014-03-22). Before that it printed the following for the spec input used in the new test: $ ./git rev-parse --parseopt -- <spec set -- -- I'm not sure what the desired behavior should have been back then. Anyway, since 2d893dff4c the documentation is clear that 'opt-spec' "May not contain any of the `<flags>` characters", so now erroring out is the right thing to do. > updated the parser, a line in parseopts's input can start > with one of the flag characters and be erroneously parsed as a opt-spec > where the short name of the option is the flag character itself and the > long name is after the end of the string. This makes Git want to > allocate SIZE_MAX bytes of memory at this line: > > o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2); > > Since s and sb.buf are equal the second argument is -2 (except unsigned) > and xmemdupz allocates len + 1 bytes, ie. -1 meaning SIZE_MAX. > > Avoid this by checking whether a flag character was found in the zeroth > position. > > Reported-by: Ingy dot Net <ingy@ingy.net> > Reviewed-by: SZEDER Gábor <szeder.dev@gmail.com> > Signed-off-by: Øystein Walle <oystwa@gmail.com> > --- > > Hi guys, thanks for the review. I incorporated a reference to the old > commit into the message (and took the liberty of adding --parseopt to > the subject like it had). I tried to verify that it was in fact this > commit, since the code prior to this one had the exact same xmemdupz() > call. I wasn't able to build that commit, Yeah, that's why I didn't check it in the morning... NO_OPENSSL=1 NO_PERL_MAKEMAKER=1 sometimes helps building older versions on modern setups, and, FWIW, Ubuntu 16.04 can build both today's Git and v1.6.0 (my goto version when I want to check historical behavior) even with OPENSSL. > builtin/rev-parse.c | 3 +++ > t/t1502-rev-parse-parseopt.sh | 7 +++++++ > 2 files changed, 10 insertions(+) > > diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c > index b259d8990a..85c271acd7 100644 > --- a/builtin/rev-parse.c > +++ b/builtin/rev-parse.c > @@ -479,6 +479,9 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) > if (!s) > s = help; > > + if (s == sb.buf) > + die(_("missing opt-spec before option flags")); > + > if (s - sb.buf == 1) /* short option only */ > o->short_name = *sb.buf; > else if (sb.buf[1] != ',') /* long option only */ > diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh > index 284fe18e72..de1d48f3ba 100755 > --- a/t/t1502-rev-parse-parseopt.sh > +++ b/t/t1502-rev-parse-parseopt.sh > @@ -306,6 +306,13 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:" > test_cmp expect actual > ' > > +test_expect_success 'test --parseopt invalid opt-spec' ' > + test_write_lines x -- "=, x" >spec && > + echo "fatal: missing opt-spec before option flags" >expect && > + test_must_fail git rev-parse --parseopt -- >out <spec 2>err && > + test_cmp expect err > +' > + > test_expect_success 'test --parseopt help output: multi-line blurb after empty line' ' > sed -e "s/^|//" >spec <<-\EOF && > |cmd [--some-option] > -- > 2.34.1 >
SZEDER Gábor <szeder.dev@gmail.com> writes: > NO_OPENSSL=1 NO_PERL_MAKEMAKER=1 sometimes helps building older > versions on modern setups, and, FWIW, Ubuntu 16.04 can build both > today's Git and v1.6.0 (my goto version when I want to check > historical behavior) even with OPENSSL. Thanks for a good piece of advice.
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index b259d8990a..85c271acd7 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -479,6 +479,9 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) if (!s) s = help; + if (s == sb.buf) + die(_("missing opt-spec before option flags")); + if (s - sb.buf == 1) /* short option only */ o->short_name = *sb.buf; else if (sb.buf[1] != ',') /* long option only */ diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh index 284fe18e72..de1d48f3ba 100755 --- a/t/t1502-rev-parse-parseopt.sh +++ b/t/t1502-rev-parse-parseopt.sh @@ -306,6 +306,13 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:" test_cmp expect actual ' +test_expect_success 'test --parseopt invalid opt-spec' ' + test_write_lines x -- "=, x" >spec && + echo "fatal: missing opt-spec before option flags" >expect && + test_must_fail git rev-parse --parseopt -- >out <spec 2>err && + test_cmp expect err +' + test_expect_success 'test --parseopt help output: multi-line blurb after empty line' ' sed -e "s/^|//" >spec <<-\EOF && |cmd [--some-option]
After 2d893dff4c (rev-parse --parseopt: allow [*=?!] in argument hints, 2015-07-14) updated the parser, a line in parseopts's input can start with one of the flag characters and be erroneously parsed as a opt-spec where the short name of the option is the flag character itself and the long name is after the end of the string. This makes Git want to allocate SIZE_MAX bytes of memory at this line: o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2); Since s and sb.buf are equal the second argument is -2 (except unsigned) and xmemdupz allocates len + 1 bytes, ie. -1 meaning SIZE_MAX. Avoid this by checking whether a flag character was found in the zeroth position. Reported-by: Ingy dot Net <ingy@ingy.net> Reviewed-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Øystein Walle <oystwa@gmail.com> --- Hi guys, thanks for the review. I incorporated a reference to the old commit into the message (and took the liberty of adding --parseopt to the subject like it had). I tried to verify that it was in fact this commit, since the code prior to this one had the exact same xmemdupz() call. I wasn't able to build that commit, and reverting it also wasn't straightforward. But I'm fairly confident it's the case since the old call had an if similar to the one added here. I completely agree with the changes to the test; it makes little sense to mix stdout and stderr here. Jeff, I agree that perhaps a larger rewrite would be better. I personally can get easily confused by "sporadic" ifs like this one in the middle of a piece of code. At least in this case the message within die() neatly explains what's going on. Øsse builtin/rev-parse.c | 3 +++ t/t1502-rev-parse-parseopt.sh | 7 +++++++ 2 files changed, 10 insertions(+)