On Mon, Aug 05, 2024 at 01:53:22AM +1000, Mark Jamsek wrote:
> Updated diff checks the buffer for the terminating newline--not the
> line returned from getline(3). This fixes an OOB read in the previous
> diff.

ok by me too (style: could avoid the reundant == 1 in boolean context)

> I still want to write a test for this case though.

Great!

> diffstat f997e889eb0517a24a28f2f4000d6915683fef86 b5ac15ce9816489710c3698d89d6a7ef21b4d2fe
>  M  lib/diff3.c  |  12+  1-
> 
> 1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff f997e889eb0517a24a28f2f4000d6915683fef86 b5ac15ce9816489710c3698d89d6a7ef21b4d2fe
> commit - f997e889eb0517a24a28f2f4000d6915683fef86
> commit + b5ac15ce9816489710c3698d89d6a7ef21b4d2fe
> blob - c1ca2fdf44d461cda6adb950263ed6a9775971ac
> blob + cc7cc1ee97127908122a804cd7aa3e75540389df
> --- lib/diff3.c
> +++ lib/diff3.c
> @@ -155,6 +155,8 @@ struct diff3_state {
>  
>  	char *buf;
>  
> +	int no_eofnl;		/* set if the merged file has no eof newline */
> +
>  	BUF *diffbuf;
>  };
>  
> @@ -417,6 +419,9 @@ out:
>  			err = got_error_from_errno("fclose");
>  	}
>  	if (err == NULL && diffb) {
> +		if (d3s->no_eofnl == 1 &&
> +		    diffb->cb_buf[diffb->cb_len - 1] == '\n')
> +			--diffb->cb_len;
>  		if (buf_write_fd(diffb, outfd) < 0)
>  			err = got_error_from_errno("buf_write_fd");
>  		*overlapcnt = d3s->overlapcnt;
> @@ -1063,7 +1068,13 @@ edscript(int n, struct diff3_state *d3s)
>  		}
>  
>  		if (!d3s->overlap[n]) {
> -			err = diff_output(d3s->diffbuf, ".\n");
> +			size_t len;
> +
> +			len = buf_len(d3s->diffbuf);
> +			d3s->no_eofnl = len > 0 &&
> +			    buf_getc(d3s->diffbuf, len - 1) != '\n';
> +			err = diff_output(d3s->diffbuf, "%s.\n",
> +			    d3s->no_eofnl == 1 ? "\n" : "");
>  			if (err)
>  				goto done;
>  		} else {
> 
> 
> -- 
> Mark Jamsek <https://bsdbox.org>
> GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68
> 
>