[PATCH] column: fix parsing of the '--nl' option

[PATCH] column: fix parsing of the '--nl' option

[SZEDER Gábor]

'git column's '--nl' option can be used to specify a "string to be
printed at the end of each line" (quoting the man page), but this
option and its mandatory argument has been parsed as OPT_INTEGER since
the introduction of the command in 7e29b8254f (Add column layout
skeleton and git-column, 2012-04-21).  Consequently, any non-number
argument is rejected by parse-options, and any number other than 0
leads to segfault:

  $ printf "%s\n" one two |git column --mode=plain --nl=foo
  error: option `nl' expects a numerical value
  $ printf "%s\n" one two |git column --mode=plain --nl=42
  Segmentation fault (core dumped)
  $ printf "%s\n" one two |git column --mode=plain --nl=0
  one
  two

Parse this option as OPT_STRING.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 Documentation/git-column.txt |  2 +-
 builtin/column.c             |  2 +-
 t/t9002-column.sh            | 18 ++++++++++++++++++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-column.txt b/Documentation/git-column.txt
index f58e9c43e6..6cea9ab463 100644
--- a/Documentation/git-column.txt
+++ b/Documentation/git-column.txt
@@ -39,7 +39,7 @@ OPTIONS
 --indent=<string>::
 	String to be printed at the beginning of each line.
 
---nl=<N>::
+--nl=<string>::
 	String to be printed at the end of each line,
 	including newline character.
 
diff --git a/builtin/column.c b/builtin/column.c
index 40d4b3bee2..158fdf53d9 100644
--- a/builtin/column.c
+++ b/builtin/column.c
@@ -29,7 +29,7 @@ int cmd_column(int argc, const char **argv, const char *prefix)
 		OPT_INTEGER(0, "raw-mode", &colopts, N_("layout to use")),
 		OPT_INTEGER(0, "width", &copts.width, N_("maximum width")),
 		OPT_STRING(0, "indent", &copts.indent, N_("string"), N_("padding space on left border")),
-		OPT_INTEGER(0, "nl", &copts.nl, N_("padding space on right border")),
+		OPT_STRING(0, "nl", &copts.nl, N_("string"), N_("padding space on right border")),
 		OPT_INTEGER(0, "padding", &copts.padding, N_("padding space between columns")),
 		OPT_END()
 	};
diff --git a/t/t9002-column.sh b/t/t9002-column.sh
index 89983527b6..6d3dbde3fe 100755
--- a/t/t9002-column.sh
+++ b/t/t9002-column.sh
@@ -42,6 +42,24 @@ EOF
 	test_cmp expected actual
 '
 
+test_expect_success '--nl' '
+	cat >expected <<\EOF &&
+oneZ
+twoZ
+threeZ
+fourZ
+fiveZ
+sixZ
+sevenZ
+eightZ
+nineZ
+tenZ
+elevenZ
+EOF
+	git column --nl="Z$LF" --mode=plain <lista >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success '80 columns' '
 	cat >expected <<\EOF &&
 one    two    three  four   five   six    seven  eight  nine   ten    eleven
-- 
2.33.0.453.gc5e41af357

Re: [PATCH] column: fix parsing of the '--nl' option

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3218 bytes --]

Hi Gábor,

On Wed, 18 Aug 2021, SZEDER Gábor wrote:

> 'git column's '--nl' option can be used to specify a "string to be
> printed at the end of each line" (quoting the man page), but this
> option and its mandatory argument has been parsed as OPT_INTEGER since
> the introduction of the command in 7e29b8254f (Add column layout
> skeleton and git-column, 2012-04-21).  Consequently, any non-number
> argument is rejected by parse-options, and any number other than 0
> leads to segfault:
>
>   $ printf "%s\n" one two |git column --mode=plain --nl=foo
>   error: option `nl' expects a numerical value
>   $ printf "%s\n" one two |git column --mode=plain --nl=42
>   Segmentation fault (core dumped)
>   $ printf "%s\n" one two |git column --mode=plain --nl=0
>   one
>   two
>
> Parse this option as OPT_STRING.

Whoa. Nice catch. I have just one suggestion about the patch, below.

> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  Documentation/git-column.txt |  2 +-
>  builtin/column.c             |  2 +-
>  t/t9002-column.sh            | 18 ++++++++++++++++++
>  3 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-column.txt b/Documentation/git-column.txt
> index f58e9c43e6..6cea9ab463 100644
> --- a/Documentation/git-column.txt
> +++ b/Documentation/git-column.txt
> @@ -39,7 +39,7 @@ OPTIONS
>  --indent=<string>::
>  	String to be printed at the beginning of each line.
>
> ---nl=<N>::
> +--nl=<string>::
>  	String to be printed at the end of each line,
>  	including newline character.
>
> diff --git a/builtin/column.c b/builtin/column.c
> index 40d4b3bee2..158fdf53d9 100644
> --- a/builtin/column.c
> +++ b/builtin/column.c
> @@ -29,7 +29,7 @@ int cmd_column(int argc, const char **argv, const char *prefix)
>  		OPT_INTEGER(0, "raw-mode", &colopts, N_("layout to use")),
>  		OPT_INTEGER(0, "width", &copts.width, N_("maximum width")),
>  		OPT_STRING(0, "indent", &copts.indent, N_("string"), N_("padding space on left border")),
> -		OPT_INTEGER(0, "nl", &copts.nl, N_("padding space on right border")),
> +		OPT_STRING(0, "nl", &copts.nl, N_("string"), N_("padding space on right border")),
>  		OPT_INTEGER(0, "padding", &copts.padding, N_("padding space between columns")),
>  		OPT_END()
>  	};
> diff --git a/t/t9002-column.sh b/t/t9002-column.sh
> index 89983527b6..6d3dbde3fe 100755
> --- a/t/t9002-column.sh
> +++ b/t/t9002-column.sh
> @@ -42,6 +42,24 @@ EOF
>  	test_cmp expected actual
>  '
>
> +test_expect_success '--nl' '
> +	cat >expected <<\EOF &&
> +oneZ
> +twoZ
> +threeZ
> +fourZ
> +fiveZ
> +sixZ
> +sevenZ
> +eightZ
> +nineZ
> +tenZ
> +elevenZ
> +EOF

Or maybe we can do this instead?

	sed "s/\$/Z" <lista >expected &&

Much shorter, and less error-prone (in case any `--run=[...]` option would
change what is in `lista` in the future).

Ciao,
Dscho

> +	git column --nl="Z$LF" --mode=plain <lista >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_expect_success '80 columns' '
>  	cat >expected <<\EOF &&
>  one    two    three  four   five   six    seven  eight  nine   ten    eleven
> --
> 2.33.0.453.gc5e41af357
>
>

Re: [PATCH] column: fix parsing of the '--nl' option

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> 'git column's '--nl' option can be used to specify a "string to be
> printed at the end of each line" (quoting the man page), but this
> option and its mandatory argument has been parsed as OPT_INTEGER since
> the introduction of the command in 7e29b8254f (Add column layout
> skeleton and git-column, 2012-04-21).  Consequently, any non-number
> argument is rejected by parse-options, and any number other than 0
> leads to segfault:
>
>   $ printf "%s\n" one two |git column --mode=plain --nl=foo
>   error: option `nl' expects a numerical value
>   $ printf "%s\n" one two |git column --mode=plain --nl=42
>   Segmentation fault (core dumped)
>   $ printf "%s\n" one two |git column --mode=plain --nl=0
>   one
>   two

... and another thing to notice is that number 0 would have meant
"use LF" due to columns.c::print_columns()

	nopts.nl = opts && opts->nl ? opts->nl : "\n";

which is the same as the default, so it is not likely that people
have (mistakenly) used to trigger NUL terminated records, or
anything fancy like that.

> Parse this option as OPT_STRING.

So a possible "regression" by this fix could be that those who took
advantage of the fact that --nl=0 is an absolute no-op would
suddenly start seeing their output terminated with a digit "0".  I
would have to say that it is not all that likely ;-)

I agree with Dscho's comment on the test script addition, but other
than that this looks good to me.

> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  Documentation/git-column.txt |  2 +-
>  builtin/column.c             |  2 +-
>  t/t9002-column.sh            | 18 ++++++++++++++++++
>  3 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-column.txt b/Documentation/git-column.txt
> index f58e9c43e6..6cea9ab463 100644
> --- a/Documentation/git-column.txt
> +++ b/Documentation/git-column.txt
> @@ -39,7 +39,7 @@ OPTIONS
>  --indent=<string>::
>  	String to be printed at the beginning of each line.
>  
> ---nl=<N>::
> +--nl=<string>::
>  	String to be printed at the end of each line,
>  	including newline character.
>  
> diff --git a/builtin/column.c b/builtin/column.c
> index 40d4b3bee2..158fdf53d9 100644
> --- a/builtin/column.c
> +++ b/builtin/column.c
> @@ -29,7 +29,7 @@ int cmd_column(int argc, const char **argv, const char *prefix)
>  		OPT_INTEGER(0, "raw-mode", &colopts, N_("layout to use")),
>  		OPT_INTEGER(0, "width", &copts.width, N_("maximum width")),
>  		OPT_STRING(0, "indent", &copts.indent, N_("string"), N_("padding space on left border")),
> -		OPT_INTEGER(0, "nl", &copts.nl, N_("padding space on right border")),
> +		OPT_STRING(0, "nl", &copts.nl, N_("string"), N_("padding space on right border")),
>  		OPT_INTEGER(0, "padding", &copts.padding, N_("padding space between columns")),
>  		OPT_END()
>  	};
> diff --git a/t/t9002-column.sh b/t/t9002-column.sh
> index 89983527b6..6d3dbde3fe 100755
> --- a/t/t9002-column.sh
> +++ b/t/t9002-column.sh
> @@ -42,6 +42,24 @@ EOF
>  	test_cmp expected actual
>  '
>  
> +test_expect_success '--nl' '
> +	cat >expected <<\EOF &&
> +oneZ
> +twoZ
> +threeZ
> +fourZ
> +fiveZ
> +sixZ
> +sevenZ
> +eightZ
> +nineZ
> +tenZ
> +elevenZ
> +EOF
> +	git column --nl="Z$LF" --mode=plain <lista >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_expect_success '80 columns' '
>  	cat >expected <<\EOF &&
>  one    two    three  four   five   six    seven  eight  nine   ten    eleven

Re: [PATCH] column: fix parsing of the '--nl' option

[SZEDER Gábor]

On Wed, Aug 18, 2021 at 01:53:14PM -0700, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> > 'git column's '--nl' option can be used to specify a "string to be
> > printed at the end of each line" (quoting the man page), but this
> > option and its mandatory argument has been parsed as OPT_INTEGER since
> > the introduction of the command in 7e29b8254f (Add column layout
> > skeleton and git-column, 2012-04-21).  Consequently, any non-number
> > argument is rejected by parse-options, and any number other than 0
> > leads to segfault:
> >
> >   $ printf "%s\n" one two |git column --mode=plain --nl=foo
> >   error: option `nl' expects a numerical value
> >   $ printf "%s\n" one two |git column --mode=plain --nl=42
> >   Segmentation fault (core dumped)
> >   $ printf "%s\n" one two |git column --mode=plain --nl=0
> >   one
> >   two
> 
> ... and another thing to notice is that number 0 would have meant
> "use LF" due to columns.c::print_columns()
> 
> 	nopts.nl = opts && opts->nl ? opts->nl : "\n";
> 
> which is the same as the default, so it is not likely that people
> have (mistakenly) used to trigger NUL terminated records, or
> anything fancy like that.
> 
> > Parse this option as OPT_STRING.
> 
> So a possible "regression" by this fix could be that those who took
> advantage of the fact that --nl=0 is an absolute no-op would
> suddenly start seeing their output terminated with a digit "0".  I
> would have to say that it is not all that likely ;-)

Yeah, I doubt that it's worth worrying about.

> I agree with Dscho's comment on the test script addition, but other
> than that this looks good to me.

Eleven other tests look just like the one I added.  I really don't
think that doing it in some other way would gain us anything, but it
would be inconsistent with the rest.

> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> > ---
> >  Documentation/git-column.txt |  2 +-
> >  builtin/column.c             |  2 +-
> >  t/t9002-column.sh            | 18 ++++++++++++++++++
> >  3 files changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/git-column.txt b/Documentation/git-column.txt
> > index f58e9c43e6..6cea9ab463 100644
> > --- a/Documentation/git-column.txt
> > +++ b/Documentation/git-column.txt
> > @@ -39,7 +39,7 @@ OPTIONS
> >  --indent=<string>::
> >  	String to be printed at the beginning of each line.
> >  
> > ---nl=<N>::
> > +--nl=<string>::
> >  	String to be printed at the end of each line,
> >  	including newline character.
> >  
> > diff --git a/builtin/column.c b/builtin/column.c
> > index 40d4b3bee2..158fdf53d9 100644
> > --- a/builtin/column.c
> > +++ b/builtin/column.c
> > @@ -29,7 +29,7 @@ int cmd_column(int argc, const char **argv, const char *prefix)
> >  		OPT_INTEGER(0, "raw-mode", &colopts, N_("layout to use")),
> >  		OPT_INTEGER(0, "width", &copts.width, N_("maximum width")),
> >  		OPT_STRING(0, "indent", &copts.indent, N_("string"), N_("padding space on left border")),
> > -		OPT_INTEGER(0, "nl", &copts.nl, N_("padding space on right border")),
> > +		OPT_STRING(0, "nl", &copts.nl, N_("string"), N_("padding space on right border")),
> >  		OPT_INTEGER(0, "padding", &copts.padding, N_("padding space between columns")),
> >  		OPT_END()
> >  	};
> > diff --git a/t/t9002-column.sh b/t/t9002-column.sh
> > index 89983527b6..6d3dbde3fe 100755
> > --- a/t/t9002-column.sh
> > +++ b/t/t9002-column.sh
> > @@ -42,6 +42,24 @@ EOF
> >  	test_cmp expected actual
> >  '
> >  
> > +test_expect_success '--nl' '
> > +	cat >expected <<\EOF &&
> > +oneZ
> > +twoZ
> > +threeZ
> > +fourZ
> > +fiveZ
> > +sixZ
> > +sevenZ
> > +eightZ
> > +nineZ
> > +tenZ
> > +elevenZ
> > +EOF
> > +	git column --nl="Z$LF" --mode=plain <lista >actual &&
> > +	test_cmp expected actual
> > +'
> > +
> >  test_expect_success '80 columns' '
> >  	cat >expected <<\EOF &&
> >  one    two    three  four   five   six    seven  eight  nine   ten    eleven

Re: [PATCH] column: fix parsing of the '--nl' option

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> Eleven other tests look just like the one I added.  I really don't
> think that doing it in some other way would gain us anything, but it
> would be inconsistent with the rest.

Fair enough.