| Message ID | 20220902063958.2516-1-oystwa@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] rev-parse: Detect missing opt-spec | expand |
On Fri, Sep 2, 2022 at 2:40 AM Øystein Walle <oystwa@gmail.com> wrote: > If a line in parseopts's input starts with one of the flag characters it > is 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> > Signed-off-by: Øystein Walle <oystwa@gmail.com> > --- > > Thanks for the review, Eric (should I then add a Reviewed-by trailer?). > Fixed the casing, added the suggested trailer, and remove the > superfluous test_done which indeed was a leftover. Thanks for addressing my minor comments. Since I only scanned my eye over the commit message and patch text, but didn't actually dig into the code to verify if the fix was correct, a Reviewed-by: would be misleading, so let's not add that trailer.
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..75f576249c 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 >actual 2>&1 && + test_cmp expect actual +' + test_expect_success 'test --parseopt help output: multi-line blurb after empty line' ' sed -e "s/^|//" >spec <<-\EOF && |cmd [--some-option]
If a line in parseopts's input starts with one of the flag characters it is 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> Signed-off-by: Øystein Walle <oystwa@gmail.com> --- Thanks for the review, Eric (should I then add a Reviewed-by trailer?). Fixed the casing, added the suggested trailer, and remove the superfluous test_done which indeed was a leftover. builtin/rev-parse.c | 3 +++ t/t1502-rev-parse-parseopt.sh | 7 +++++++ 2 files changed, 10 insertions(+)