%L NLSPATH handling: just use locale name, skip language code #67
Comments
|
please pull jelmd@c1ec84f |
|
I don't understand why there isn't a detailed explanation of the "NLSPATH resolver" bug in the opening comment. Even if just to copy/paste the comment from the issue in your fork. Also, it would make it easier to decide to merge your fix if it included a regression unit test so that we can have some confidence the fix is correct and the problem won't be reintroduced. The description of the issue in your fork suggests there is a bug in the AST implementation according to the SUS standard. As I've said in a different issue I would prefer that we stop maintaining this code and switch to the NLS implementation provided by each distro since this is no longer a bleeding edge API that justifies a private implementation. Having said that I am not opposed to merging this change to the AST implementation. Merging the change would be easier to justify if there was a unit test or at least a description of how to reproduce the problem without this change in order to verify that the change does fix the problem. |
|
There isn't a "detailed explanation of the "NLSPATH resolver" bug in the opening comment" because this is IMHO not a real bug, but an optimization 'just use locale name, skip language code'. That's the summary and the details are as usual in the problem description. Wrt. bug: strictly taken, yes. But I guess, some ppl would call it possibly a feature - it does, what is required to do and a little bit more (for convenience). However, because the same behavior can be obtained, adding a similar %l path to NLSPATH, it can be optimized away, if mentioned in the release notes. I've not a full picture of ksh's localization implementation/features, just about what I've touched when trying to fix jelmd#1 . So as long as replacing it with the stuff the OS provides doesn't break anything, I do not really care. |
|
Closing since we're going to remove the dependency on the libast locale code as soon as possible because programs not using this code will behave differently than |
On systems where ksh needs to use the older and less secure FIFO method for process substitutions (which is currently all of them as the more modern and solid /dev/fd method is still broken, see att#67), process substitutions could leave background processes hanging in these two scenarios: 1. If the parent process exits without opening a pipe to the child process forked by the process substitution. The fifo_check() function in xec.c, which is periodically called to check if the parent process still exists while waiting for it to open the FIFO, verified the parent process's existence by checking if the PPID had reverted to 1, the traditional PID of init. However, POSIX specifies that the PPID can revert to any implementation- defined system process in that case. So this breaks on certain systems, causing unused process substitutions to hang around forever as they never detect that the parent disappeared. The fix is to save the current PID before forking and having the child check if the PPID has changed from that saved PID. 2. If command invoked from the main shell is passed a process substitution, but terminates without opening the pipe to the process substitution. In that case, the parent process never disappears in the first place, because the parent process is the main shell. So the same infinite wait occurs in unused process substitutions, even after correcting problem 1. The fix is to remember all FIFOs created for any number of process substitutions passed to a single command, and unlink any remaining FIFOs as they represent unused command substitutions. Unlinking them FIFOs causes sh_open() in the child to fail with ENOENT on the next periodic check, which can easily be handled. Fixing these problems causes the FIFO method to act identically to the /dev/fd method, which is good for compatibility. Even when att#67 is fixed this will still be important, as ksh also runs on systems that do not have /dev/fd (such as AIX, HP-UX, and QNX), so will fall back to using FIFOs. --- Fix problem 1 --- src/cmd/ksh93/sh/xec.c: - Add new static fifo_save_ppid variable. - sh_exec(): If a FIFO is defined, save the current PID in fifo_save_ppid for the forked child to use. - fifo_check(): Compare PPID against the saved value instead of 1. --- Fix problem 2 --- To keep things simple I'm abusing the name-value pair routines used for variables for this purpose. The overhead is negligible. A more elegant solution is possible but would involve adding more code. src/cmd/ksh93/include/defs.h: _SH_PRIVATE: - Define new sh.fifo_tree pointer to a new FIFO cleanup tree. src/cmd/ksh93/sh/args.c: sh_argprocsubs(): - After launching a process substitution in the background, add the FIFO to the cleanup list before freeing it. src/cmd/ksh93/sh/xec.c: - Add fifo_cleanup() that unlinks all FIFOs in the cleanup list and clears/closes the list. They should only still exist if the command never used them, however, just run 'unlink' and don't check for existence first as that would only add overhead. - sh_exec(): * Call fifo_cleanup() on finishing all simple commands (when setting $?) or when a special builtin fails. * When forking, clear/close the cleanup list; we do not want children doing duplicate cleanup, particularly as this can interfere when using multiple process substitutions in one command. * Process substitution handling: > Change FIFO check frequency from 500ms to 50ms. Note that each check sends a signal that interrupts open(2), causing sh_open() to reinvoke it. This causes sh_open() to fail with ENOENT on the next check when the FIFO no longer exists, so we do not need to add an additional check for existence to fifo_check(). Unused process substitutions now linger for a maximum of 50ms. > Do not issue an error message if errno == ENOENT. - sh_funct(): Process substitutions can be passed to functions as well, and we do not want commands within the function to clean up the FIFOs for the process substitutions passed to it from the outside. The problem is solved by simply saving fifo_tree in a local variable, setting it to null before running the function, and cleaning it up before restoring the parent one at the end. Since sh_funct() is called recursively for multiple-level function calls, this correctly gives each function a locally scoped fifo_tree. --- Tests --- src/cmd/ksh93/tests/io.sh: - Add tests covering the failing scenarios. Co-authored-by: Martijn Dekker <martijn@inlv.org>
NLSPATH resolver fix: see jelmd#2
The text was updated successfully, but these errors were encountered: