Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

KSH hang in |spawnvex(3ast)| #34

Closed
DavidMorano opened this issue Apr 9, 2017 · 5 comments
Closed

KSH hang in |spawnvex(3ast)| #34

DavidMorano opened this issue Apr 9, 2017 · 5 comments
Labels

Comments

@DavidMorano
Copy link

Just a reminder that if you see a hang from KSH (or another AST program) while it is in the "spawnvex(3ast)| subroutine it is most likely caused by LIBAST getting compiled with the use of
|vfork(2)|. When using VFORK, a program should pretty much just |exec(2)| after |vfork(2)|'ing.
This is just safe UNIX practice. But |spawnvex(3ast)| plays with the program signal mast (or some
such) after it has |vfork(2)|'ed but before it |exec(2)|'s. This can occasionally caused a hang on
some UNIX systems. The fix is to undefine some defines as follows in the |spawnvex(3ast)|
subroutine somewhere after the headers are included, with something like:

#undef _lib_vfork
#undef _real_vfork

Enjoy.

@krader1961 krader1961 added the bug label Jan 2, 2018
@krader1961
Copy link
Contributor

The need for such #undef directives implies that the code needs to be refactored. But I would prefer to just remove the use of the posix_spawn() API. That API is so fugly that even a Microsoft software engineer would probably hate it.

@krader1961
Copy link
Contributor

BTW, the intended use case for posix_spawn() is spawning "helper processes", not starting new process groups. Notably it doesn't provide the equivalent of setsid() and tcsetpgrp(). Which makes it a poor choice for shells like ksh. It's really meant to provide a cheap way for potentially large programs to run external commands without incurring the cost of duplicating its virtual memory structures (e.g., the process page tables).

While it is more efficient than fork() when the virtual process size is large that will not normally be an issue for a ksh process. And we still have to support fork() because posix_spawn() isn't available on all systems we want to support or is broken (e.g., on Cygwin). Also, note that on Linux it's actually implemented as a function not a syscall. On Linux we should be using clone(). And on other systems (e.g., BSD) we should probably be using vfork().

See https://bugzilla.redhat.com/show_bug.cgi?id=1295563 for another example of how posix_spawn() causes problems.

There was a long discussion about using posix_spawn() in the Go language: golang/go#5838. It looks like ultimately they chose to use the Linux clone() function.

@siteshwar
Copy link
Contributor

@DavidMorano This issue was more of a candidate for wiki, but thanks for reporting it.

@krader1961 @DavidMorano Since we have removed support for vfork() through 6af5449, Can we close this issue ?

@DavidMorano
Copy link
Author

@siteshwar

Since we have removed support for vfork() through 6af5449, Can we close this issue ?

YES - close.

As long as VFORK is gone, I am happy. This can certainly be closed now.
VFORK caused different problems on different OSes that it was selected to be used for in the KSH build process. It was always an error-prone interface. I just wanted it gone (for safety reasons as well as because of the various hangs or other problems it caused on some OSes). Good riddance.

@krader1961
Copy link
Contributor

Closing as I've removed all mentions and uses of vfork from the code.

citrus-it pushed a commit to citrus-it/ast that referenced this issue Apr 15, 2021
Ksh was trying to use the 'pw' variable as a valid pointer even
when it was NULL. This is fixed by doing the error check for
'pw' before doing anything else in 'job_kill'.

This bugfix is from Red Hat:
https://git.centos.org/rpms/ksh/blob/44e0a643a93492b1f6beebbf6ffcfd453d9ab8f2/f/SOURCES/ksh-20130214-fixkill.patch

Fixes att#34
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants