From 89c69b076d03d52083cba1c17ca56370ee613bc2 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Sun, 7 Mar 2021 00:27:33 +0000 Subject: [PATCH] Fix command history corruption on syntax error (re: e999f6b1) Analysis: When a syntax error occurs, the shell performs a longjmp(3) back to exfile() in main.c on line 417: 415| if(jmpval) 416| { 417| Sfio_t *top; 418| sh_iorestore((void*)shp,0,jmpval); 419| hist_flush(shp->gd->hist_ptr); 420| sfsync(shp->outpool); The first thing it does is restore the file descriptor state (sh_iorestore), then it flushes the history file (hist_flush), then it synchronises sfio's logical stream state with the physical stream state using (sfsync). However, the fix applied in e999f6b1 caused sh_iorestore() to sync all sfio streams unconditionally. So this was done before hist_flush(), which caused unpredictable behaviour, including temporary and/or permanent history corruption, as this also synched shp->outpool before hist_flush() had a chance to do its thing. The fix is to only call sfsync() in sh_iorestore() if we're actually about to call ftruncate(2), and not otherwise. Moral of the story: bug fixes should be as specific as possible to minimise the risk of side effects. src/cmd/ksh93/sh/io.c: sh_iorestore(): - Only call sfsync() if we're about to truncate a file. src/cmd/ksh93/tests/pty.sh: - Add test. Thanks to Marc Wilson for reporting the bug and to Johnothan King for finding the commit that introduced it. Resolves: https://github.com/ksh93/ksh/issues/209 Relevant: https://github.com/att/ast/issues/61 --- NEWS | 4 ++++ src/cmd/ksh93/sh/io.c | 8 +++----- src/cmd/ksh93/tests/pty.sh | 17 ++++++++++++++++- 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/NEWS b/NEWS index 3010c825d..9a9e9570b 100644 --- a/NEWS +++ b/NEWS @@ -12,6 +12,10 @@ Any uppercase BUG_* names are modernish shell bug IDs. - Fixed a bug introduced on 2020-08-19: Ctrl+D would break after an interactive shell received SIGWINCH. +- Fixed a bug introduced on 2020-05-21: on an interactive shell, command lines + containing a syntax error were not added to the command history file and + sometimes corrupted the command history. + 2021-03-05: - Unbalanced quotes and backticks now correctly produce a syntax error diff --git a/src/cmd/ksh93/sh/io.c b/src/cmd/ksh93/sh/io.c index 0f4be76b5..4b83a7e38 100644 --- a/src/cmd/ksh93/sh/io.c +++ b/src/cmd/ksh93/sh/io.c @@ -1712,11 +1712,6 @@ void sh_iorestore(Shell_t *shp, int last, int jmpval) register int origfd, savefd, fd; int flag = (last&IOSUBSHELL); last &= ~IOSUBSHELL; - /* - * There was an issue with truncating files (see 'ftruncate' below) that was caused by - * out-of-sync streams. So, to be safe, sync all streams before restoring file descriptors. - */ - sfsync(NULL); for (fd = shp->topfd - 1; fd >= last; fd--) { if(!flag && filemap[fd].subshell) @@ -1740,7 +1735,10 @@ void sh_iorestore(Shell_t *shp, int last, int jmpval) return; } if(filemap[fd].tname == Empty && shp->exitval==0) + { + sfsync(NIL(Sfio_t*)); ftruncate(origfd,lseek(origfd,0,SEEK_CUR)); + } else if(filemap[fd].tname) io_usename(filemap[fd].tname,(int*)0,origfd,shp->exitval?2:1); sh_close(origfd); diff --git a/src/cmd/ksh93/tests/pty.sh b/src/cmd/ksh93/tests/pty.sh index 733a269d5..69b30f3e3 100755 --- a/src/cmd/ksh93/tests/pty.sh +++ b/src/cmd/ksh93/tests/pty.sh @@ -107,7 +107,6 @@ if ! pty $bintrue < /dev/null then err_exit pty command hangs on $bintrue -- tests skipped exit 0 fi - # err_exit # tst $LINENO <<"!" L POSIX sh 026(C) @@ -712,5 +711,21 @@ r ^:test-3: ls vi_completion_A_file\r\n$ r ^vi_completion_A_file\r\n$ ! +# err_exit # +tst $LINENO <<"!" +L syntax error added to history file + +# https://github.com/ksh93/ksh/issues/209 + +d 10 +p :test-1: +w do something +r ^:test-1: do something\r\n$ +r : syntax error: `do' unexpected\r\n$ +w fc -lN1 +r ^:test-2: fc -lN1\r\n$ +r \tdo something\r\n$ +! + # ====== exit $((Errors<125?Errors:125))