-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nvdisc.c: Fix crash after an error or signal in discipline function #356
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch fixes the crashes experienced when a discipline function exited because of a signal or an error from a special builtin. The crashes were caused by ksh entering an inconsistent state after performing a longjmp away from the assign() and lookup() functions in nvdisc.c. Fixing the crash requires entering a new context, then setting a nonlocal goto with sigsetjmp(3). Any longjmps that happen while running the discipline function will go back to assign/lookup, allowing ksh to do a proper cleanup afterwards. Resolves: ksh93#346
Thanks, works for me. |
I'll try to take a look at the regression test later. But I think it's nonessential as this seems to be a straightforward fix. |
McDutchie
pushed a commit
that referenced
this pull request
Dec 5, 2021
…356) This patch fixes the crashes experienced when a discipline function exited because of a signal or an error from a special builtin. The crashes were caused by ksh entering an inconsistent state after performing a longjmp away from the assign() and lookup() functions in nvdisc.c. Fixing the crash requires entering a new context, then setting a nonlocal goto with sigsetjmp(3). Any longjmps that happen while running the discipline function will go back to assign/lookup, allowing ksh to do a proper cleanup afterwards. Resolves: #346
JohnoKing
added a commit
to JohnoKing/ksh
that referenced
this pull request
Feb 16, 2024
This was first reported in att#1472. Attempting to cancel a heredoc with ^C or ^D can cause ksh to crash with a segfault, or hangup and fill /tmp with files. Copy of the reproducer: $ cat << EOS > <Press Ctrl+C or Ctrl+D> src/cmd/ksh93/sh/main.c: - Reset the lexer state in an interactive shell if here-document creation was cancelled. This patch has been adapted from Solaris: https://github.com/oracle/solaris-userland/blob/e478b48/components/ksh93/patches/400-29444429.patch Due to the nature of this bug, I've skipped adding a regression test as it risks causing pty to hang up if run against an older release of ksh93 (cf. ksh93#356).
JohnoKing
added a commit
to JohnoKing/ksh
that referenced
this pull request
Feb 23, 2024
src/cmd/ksh93/tests/pty.sh: - Fix test for ksh93#722 by checking for 'Exit status 0' with 'u'. As far as I can tell this fixes the CI failure while still catching the crash on bugged commits. - Add a fixed version of the previously unused regression test from ksh93#356. Avoiding the CPU hungry lockup only requires spawning a child interactive shell for the crash to occur inside of. (This doesn't fix the underlying bug in pty, but merely works around it.)
McDutchie
pushed a commit
that referenced
this pull request
Mar 23, 2024
src/cmd/ksh93/tests/pty.sh: - Fix test for #722 by checking for 'Exit status 0' with 'u'. As far as I can tell this fixes the CI failure while still catching the crash on bugged commits. - Add a fixed version of the previously unused regression test from #356. Avoiding the CPU hungry lockup only requires spawning a child interactive shell for the crash to occur inside of. (This doesn't fix the underlying bug in pty, but merely works around it.)
McDutchie
pushed a commit
that referenced
this pull request
Mar 23, 2024
src/cmd/ksh93/tests/pty.sh: - Fix test for #722 by checking for 'Exit status 0' with 'u'. As far as I can tell this fixes the CI failure while still catching the crash on bugged commits. - Add a fixed version of the previously unused regression test from #356. Avoiding the CPU hungry lockup only requires spawning a child interactive shell for the crash to occur inside of. (This doesn't fix the underlying bug in pty, but merely works around it.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch fixes the crashes experienced when a discipline function (such as
.sh.tilde.set()
orPS2.get()
) exits because of a signal or an error from a special builtin. The crashes were caused by ksh entering an inconsistent state after performing alongjmp
away from theassign()
andlookup()
functions in nvdisc.c. Fixing the crash requires entering a new context, then setting a nonlocal goto withsigsetjmp
. Any longjmps that happen while running the discipline function will go back toassign
/lookup
, allowing ksh to do a proper cleanup afterwards.I haven't added the regression test for this bug to pty.sh. The test I made does work when ran with the bugfix applied to ksh, but it causes CPU-hungry hangups in unpatched versions of ksh. I couldn't create a good workaround for the hangups, but I'll leave the test here in case anyone manages to fix it:
Resolves: #346