Hello,

Hey Jerry - thank you for investigating this!

This looks OK to me. What do other vi'ers think?

Kind regards,

Job

On Wed, Jul 30, 2025 at 07:43:26PM +0200, jerry wrote:
> Hello,
> 
> while using vi(1) with expandtab I noticed a situation that caused it to
> segfault that I managed to fix.
> The bug happens also in nvi from ports, and in all the other nvi that I
> testes.
> 
> The procedure that causes the bug:
> * open vi with set expandtab
> * yy -> yank something
> * :di b -> shows that the default buffer have the text copied
> * :!ls >/dev/null run an external command without filtering lines, all
>   commands are the same
> * :di b -> already shows something strange, outputting:
>   ********** default buffer (character mode)
>   note that when the buffers are empty :di b shows:
>   No cut buffers to display.
> * p -> trying to paste segfault
> 
> A bug that does not segfault occurs also with the bang command, but when
> filtering lines:
> * open vi with set expandtab
> * yy -> yank something and then move to an other line
> * :di b -> shows that the default buffer have the text copied
> * :.!ls >/dev/null run an external command filtering any range of lines,
>   both one address or two is the same, noticing the content of the line
>   you are on
> * :di b -> shows that the line(s) being put into the default buffer
>   is(are) the line(s) that is(are) now under the cursor, not the original
>   deleted ones
> 
> Running the bang without filtering files leaves the sp->gp->dcb_store as
> a dangling pointer.
> The stacktrace of when the bug happens is:
> * ex_bang in ex/ex_bang.c line 176
> * ex_retab in ex/ex_shift.c line 59
> * shift in ex/ex_shift.c line 82
> * cut in common/cut.c line 137
> * cut_line in common/cut.c line 242
> * db_get in common/line.c line 88
> When cut_line returns an error, by going to the goto cut_line_err, it
> skps the line 160 that repoints the default buffer on each pass:
>     sp->gp->dcbp = cbp; /* Repoint the default buffer on each pass. */
> thus leaving dcbp in an invalid state.
> 
> The changes that I made:
> * added sp->gp->dcbp = NULL in the goto cut_line_err.
>   With just this change, the bug is still present but at least there is
>   not the segfault since the default buffer will be pointing to NULL, so
>   it will be as if anything has ever being copied, :di b will show:
>   No cut buffers to display.
> * then I added a check at the start of cut that return error if any of
>   the line numbers of the marks are out-of-band, since otherwise it will
>   fail below in cut_line
> * going up the stackframe, in shift I made so that the cut command is
>   run just when real shifts are happening, not when doing expandtab 
> * then in ex_bang I made so expandtab is applied just when at least an
>   address was specified since otherwise there is nothing to expand
> * then at last I noticed that in common/line.c OOBLNO was used in some
>   places but not in others, so I made it consistent by using it in all
>   occurances
> 
> Bye,
> jerry
> 
> diff --git common/cut.c common/cut.c
> index 7ed134693a5..e77a8ed22a9 100644
> --- common/cut.c
> +++ common/cut.c
> @@ -69,6 +69,10 @@ cut(SCR *sp, CHAR_T *namep, MARK *fm, MARK *tm, int flags)
>  	recno_t lno;
>  	int append, copy_one, copy_def;
>  
> +	/* Check if the line numbers are out-of-band */
> +	if (fm->lno == OOBLNO || tm->lno == OOBLNO)
> +		return (1);
> +
>  	/*
>  	 * If the user specified a buffer, put it there.  (This may require
>  	 * a copy into the numeric buffers.  We do the copy so that we don't
> @@ -175,6 +179,7 @@ cut_line_err:
>  	text_lfree(&cbp->textq);
>  	cbp->len = 0;
>  	cbp->flags = 0;
> +	sp->gp->dcbp = NULL;
>  	return (1);
>  }
>  
> diff --git common/line.c common/line.c
> index 7dfbdbf2756..84f04b27c0f 100644
> --- common/line.c
> +++ common/line.c
> @@ -49,7 +49,7 @@ db_eget(SCR *sp, recno_t lno, char **pp, size_t *lenp, int *isemptyp)
>  	 * line in an empty file, find the last line of the file; db_last
>  	 * fails loudly.
>  	 */
> -	if ((lno == 0 || lno == 1) && db_last(sp, &l1))
> +	if ((lno == OOBLNO || lno == 1) && db_last(sp, &l1))
>  		return (1);
>  
>  	/* If the file isn't empty, fail loudly. */
> @@ -84,7 +84,7 @@ db_get(SCR *sp, recno_t lno, u_int32_t flags, char **pp, size_t *lenp)
>  	 * have to have an OOB condition for the look-aside into the input
>  	 * buffer anyway.
>  	 */
> -	if (lno == 0)
> +	if (lno == OOBLNO)
>  		goto err1;
>  
>  	/* Check for no underlying file. */
> diff --git ex/ex_bang.c ex/ex_bang.c
> index cdb7bf2ec57..4ef93c1c678 100644
> --- ex/ex_bang.c
> +++ ex/ex_bang.c
> @@ -171,8 +171,8 @@ ex_bang(SCR *sp, EXCMD *cmdp)
>  	if (!F_ISSET(sp, SC_VI) && !F_ISSET(sp, SC_EX_SILENT))
>  		(void)ex_puts(sp, "!\n");
>  
> -	/* Apply expandtab to the new text */
> -	if (O_ISSET(sp, O_EXPANDTAB))
> +	/* If addresses were specified, apply expandtab to the new text */
> +	if (cmdp->addrcnt != 0 && O_ISSET(sp, O_EXPANDTAB))
>  		ex_retab(sp, cmdp);
>  
>  	/*
> diff --git ex/ex_shift.c ex/ex_shift.c
> index e7f45c1916c..8f6edad0f2d 100644
> --- ex/ex_shift.c
> +++ ex/ex_shift.c
> @@ -78,9 +78,13 @@ shift(SCR *sp, EXCMD *cmdp, enum which rl)
>  		return (0);
>  	}
>  
> -	/* Copy the lines being shifted into the unnamed buffer. */
> -	if (cut(sp, NULL, &cmdp->addr1, &cmdp->addr2, CUT_LINEMODE))
> -		return (1);
> +    /*
> +     * When not doing re-expand tabs, copy the lines being shifted into
> +     * the unnamed buffer.
> +     */
> +	if (rl != RETAB &&
> +        cut(sp, NULL, &cmdp->addr1, &cmdp->addr2, CUT_LINEMODE))
> +        return (1);
>  
>  	/*
>  	 * The historic version of vi permitted the user to string any number
>