On Sat, Feb 18, 2023 at 06:47:21PM +0100, Omar Polo wrote:
> conf_parse_line may read/write out of bounds when a line is indented
> by spaces.  This because the loop to trim leading spaces advances the
> pointer without adjusting the length of the line.
> 
> Since the `line' pointer is part of a bigger buffer, when we try to
> "skip trailing whitespace, if any" we're actually writing NUL bytes in
> the middle of the next line, mangling it.  (OK, so "out of bounds"
> isn't 100% correct, we're not reading/writing arbitrary stuff in the
> heap, as long as that part is not triggered on the last line...)
> 
> There's also a new regress for this.  The write out of bounds for the
> "name" line would mangle the next line, making us skip the author info
> from the gitconfig file (as reported by falsifian on IRC.)
> 
> (this doesn't affect got directly because gitconfig.c is only included
> in the got-read-gitconfig libexec helper ;-)

Nice catch. Ok. One question below:

> diff /home/op/w/gotd
> commit - cd0aa8caa26478b2cb3c60e889894de02e0eb921
> path + /home/op/w/gotd
> blob - 4b5de6124b792fc8fcfae5babf81e657a387f457
> file + lib/gitconfig.c
> --- lib/gitconfig.c
> +++ lib/gitconfig.c
> @@ -243,8 +243,10 @@ conf_parse_line(char **section, struct got_gitconfig *
>  			return got_error_from_errno("strndup");
>  		return NULL;
>  	}
> -	while (isspace((unsigned char)*line))
> +	while (sz > 0 && isspace((unsigned char)*line)) {
>  		line++;
> +		sz--;
> +	}
>  
>  	/* Deal with assignments.  */
>  	for (i = 0; i < sz; i++)
> blob - 488cdaf04a738a2e6a11df99d0d5df6b7a604c83
> file + regress/cmdline/commit.sh
> --- regress/cmdline/commit.sh
> +++ regress/cmdline/commit.sh
> @@ -976,7 +976,40 @@ test_commit_gitconfig_author() {
>  	ret=$?
>  	if [ $ret -ne 0 ]; then
>  		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
>  	fi
> +
> +	# retry with spaces in the git config
> +	ed -s "$testroot/repo/.git/config" <<EOF
> +,s/	/    /g
> +wq
> +EOF
> +	echo "modified again" > $testroot/wt/alpha
> +
> +	# unset in a subshell to avoid affecting our environment
> +	(unset GOT_IGNORE_GITCONFIG && cd "$testroot/wt" && \
> +		got commit -m 'test gitconfig author again' >/dev/null)
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	(cd "$testroot/repo" && got log -l1 | grep ^from: > $testroot/stdout)
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	echo "from: Flank Luck <flan_luck@openbsd.org>" \

Who is Flank?

> +		> $testroot/stdout.expectd
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +	fi
>  	test_done "$testroot" "$ret"
>  }
>  
> 
>