Stefan Sperling <stsp@stsp.name> wrote:
> On Fri, Aug 29, 2025 at 11:15:17AM +0200, Stefan Sperling wrote:
> > If the patch file lacks a trailing newline just before end-of-file
> > then 'got patch' will exit with an "unexpected privsep message" error.
> > 
> > Regression test and fix below.
> > 
> > ok?
> 
> Imrpoved version which fixes a function name in an error I overlooked
> ("malloc" -> "strdup") and reorders the checks in recv_patch() for
> cosmetic reasons.

mixed feelings, sorry.  I'm not sure about many of these changes from
the linelen to strlen(): before I was trying to also support the case of
NUL bytes in the middle of a line.

Now, I know it's not an important thing, patch is for text stuff, but
from time to time some binary thing could slip, or maybe it's still text
but in other encoding (hello utf16), so I think it's worth to at least
give it a try at not breaking that easily on NUL bytes.

unfortunately at the moment I lack the time to propose a better diff,
hopefully i'll be able to take a closer look tonight.


Thanks,
Omar Polo

> M  lib/patch.c                              |  11+  7-
> M  libexec/got-read-patch/got-read-patch.c  |   2+  2-
> M  regress/cmdline/patch.sh                 |  50+  0-
> 
> 3 files changed, 63 insertions(+), 9 deletions(-)
> 
> commit - a906b277aa501d5897960b8e9f4466450d91dcac
> commit + 2a1120ba5e4469c33e2fb13129ed303f45cefe40
> blob - edb26e01b7ce7ae6f0582ad4b4cb1a56c500e3d4
> blob + 1c80144c8b6b71c24e7500863b0dc3c3f28ff965
> --- lib/patch.c
> +++ lib/patch.c
> @@ -164,13 +164,13 @@ pushline(struct got_patch_hunk *h, const char *line, s
>  		h->cap = newcap;
>  	}
>  
> -	if ((t = malloc(len - 1)) == NULL)
> -		return got_error_from_errno("malloc");
> -	memcpy(t, line + 1, len - 1);	/* skip the line type */
> +	t = strdup(line + 1); /* skip the line type */
> +	if (t == NULL)
> +		return got_error_from_errno("strdup");
>  
>  	h->lines[h->len].mode = *line;
>  	h->lines[h->len].line = t;
> -	h->lines[h->len].len = len - 2;	/* line type and trailing NUL */
> +	h->lines[h->len].len = strlen(t);
>  	h->len++;
>  	return NULL;
>  }
> @@ -299,12 +299,16 @@ recv_patch(struct imsgbuf *ibuf, int *done, struct got
>  				err = got_error(GOT_ERR_PRIVSEP_MSG);
>  				goto done;
>  			}
> -			t = imsg.data;
>  			/* at least one char */
> -			if (datalen < 2 || t[datalen-1] != '\0') {
> +			if (datalen < 2) {
>  				err = got_error(GOT_ERR_PRIVSEP_MSG);
>  				goto done;
>  			}
> +			t = imsg.data;
> +			if (memchr(t, '\0', datalen) == NULL) {
> +				err = got_error(GOT_ERR_PRIVSEP_MSG);
> +				goto done;
> +			}
>  			if (*t != ' ' && *t != '-' && *t != '+' &&
>  			    *t != '\\') {
>  				err = got_error(GOT_ERR_PRIVSEP_MSG);
> @@ -312,7 +316,7 @@ recv_patch(struct imsgbuf *ibuf, int *done, struct got
>  			}
>  
>  			if (*t != '\\')
> -				err = pushline(h, t, datalen);
> +				err = pushline(h, t, strlen(t));
>  			else if (lastmode == '-')
>  				h->old_nonl = 1;
>  			else if (lastmode == '+')
> blob - 14537c5cce57d708e10787a44cfd196c34bb6263
> blob + bbb4fdeb0e8ad7491f1ffe1cec7450ea6bb923c2
> --- libexec/got-read-patch/got-read-patch.c
> +++ libexec/got-read-patch/got-read-patch.c
> @@ -467,7 +467,7 @@ send_line(const char *line, size_t len)
>  	}
>  
>  	iov[iovcnt].iov_base = (void *)line;
> -	iov[iovcnt].iov_len = len;
> +	iov[iovcnt].iov_len = len + 1; /* send NUL terminator */
>  	iovcnt++;
>  
>  	if (imsg_composev(&ibuf, GOT_IMSG_PATCH_LINE, 0, 0, -1,
> @@ -585,7 +585,7 @@ parse_hunk(FILE *fp, int *done)
>  			goto done;
>  		}
>  
> -		err = send_line(line, linelen);
> +		err = send_line(line, strlen(line));
>  		if (err)
>  			goto done;
>  
> blob - e5504f42e6a08f89598536b78ff4c5ed56bfb461
> blob + f41077edecd7463beacb0b7995d2081ff27c3bff
> --- regress/cmdline/patch.sh
> +++ regress/cmdline/patch.sh
> @@ -2147,6 +2147,55 @@ test_patch_commit_keywords() {
>  	test_done $testroot $ret
>  }
>  
> +test_patch_no_newline_at_end_of_patch() {
> +	local testroot=`test_init patch_no_newline_at_end_of_patch`
> +
> +	got checkout $testroot/repo $testroot/wt > /dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		test_done $testroot $ret
> +		return 1
> +	fi
> +
> +	cat <<EOF > $testroot/wt/patch
> +--- alpha
> ++++ alpha
> +@@ -1 +1 @@
> +-alpha
> ++alpha is my favourite character
> +EOF
> +
> +	head -n 4 $testroot/wt/patch > $testroot/wt/patch.noeol
> +	tail -n 1 $testroot/wt/patch | tr -d '\n' >> $testroot/wt/patch.noeol
> +	
> +	(cd $testroot/wt && got patch < patch.noeol) > $testroot/stdout
> +	if [ $ret -ne 0 ]; then
> +		test_done "$testroot" $ret
> +		return 1
> +	fi
> +
> +	echo 'M  alpha' > $testroot/stdout.expected
> +
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done $testroot $ret
> +		return 1
> +	fi
> +
> +	echo 'alpha is my favourite character' > $testroot/wt/alpha.expected
> +	cmp -s $testroot/wt/alpha.expected $testroot/wt/alpha
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/wt/alpha.expected $testroot/wt/alpha
> +		test_done "$testroot" $ret
> +		return 1
> +	fi
> +
> +	test_done $testroot $ret
> +}
> +
>  test_parseargs "$@"
>  run_test test_patch_basic
>  run_test test_patch_dont_apply
> @@ -2179,3 +2228,4 @@ run_test test_patch_newfile_xbit_git_diff
>  run_test test_patch_umask
>  run_test test_patch_remove_binary_file
>  run_test test_patch_commit_keywords
> +run_test test_patch_no_newline_at_end_of_patch