In D52527#1199611, @kevans wrote:

I did wonder for a second if the /rescue variants of shells do or should assume setugid behavior and avoid loading dotfiles in case something in the profile or other bits finds a way to make the shell unusable, but I didn't really convince myself.

It seems reasonable to me to use .profile and other files from root's home directory in single user mode as well. Having no .profile at all in single user mode would be bad. I don't think special logic in pkg for hard links is worth it for this particular special case.

In D49201#1122836, @pstef wrote:

In D49201#1122784, @jilles wrote:

This adds a non-POSIX feature (although it is pretty common); is the goal to make sh a mostly feature-complete alternative to bash?

This adds a non-POSIX feature (although it is pretty common); is the goal to make sh a mostly feature-complete alternative to bash?

Given that set -e is a controversial issue (see the disagreement here and in the "Conclusions" section of https://mywiki.wooledge.org/BashFAQ/105 ), I don't think the man page should recommend anything about it, although it should mention some pitfalls in a neutral manner.

The immediately preceding sentence about shell functions is an example of unexpected behaviour of set -e. The part "all commands of the function are considered to be tested as well" may result in code unexpectedly executing in the function and the function returning a different return status.

This looks like a reasonable quick fix, but I think it's unfortunate the /root directory permissions are specified both in bsd.dirs.mk variables and in the BSD.root.dist mtree file. It looks like the bsd.dirs.mk variables are required because of pkgbase.

I agree with the analysis, but the variables should still be defined inside main (with static) so they cannot be used outside. Some of the other functions in this file have their own smark locals that should not be mixed up with main's.

To me, it looks like that situation can't actually happen. If there is an error or SIGINT during early initialization, state will still be 0 and the if (setjmp(main_handler.loc)) block will immediately call exitshell(exitstatus) without touching smark. If I'm wrong, please provide a reproduction scenario.

This doesn't follow the Don't Repeat Yourself principle, but currently I don't have a better idea how to express this code.

Code is OK like this.

Adding support for escape sequences might make it important to increase PROMPTLEN.

In D37620#855668, @corvink wrote:

Thanks for the hint. kill %+ and jobs -p %+ are already working. So, no patch required. Might be a good idea to mention it in the man page.

How do other shells with a similar option handle this? For example, zsh has a TRAPS_ASYNC option, but does not document how to access the interrupted job in the trap action.

In hindsight, this is how it should have been in the first place, since it's the same thing Linux distributions do. They (used to?) place *.csh files too for sourcing from /etc/csh.login.

Since a similar effect can be obtained by having the shell ignore the signal via trap '' TTOU, I'm not sure it's a good idea to add an option for this.

Fine with me to add this to make some compiler/linter happy, even though non-static array bounds in parameters don't really do anything.

Oh, the unset obviously defeats the purpose. Defending against someone copying /etc/profile to /usr/local/etc/profile without leaving stray variables in the user's environment is a little harder.

In D35836#823357, @pstef wrote:

@jilles is this patch in a good shape now?

This should have testcases.

Thanks, I have also encountered this issue.

In D35840#814030, @pstef wrote:

In a way, this has nothing to do with bash. I want sh PS4 to be able to generate some kind of timestamps when executing a script with tracing on. The way bash does it is just a convention which I think is not worth diverging from.

A little unfortunate to have these strange backslash sequences spread out further, but it's nothing that bash doesn't do.

POSIX indeed leaves this behavior unspecified, but Linux chooses the same as we do. Might want to add something like

POSIX leaves the behavior of
.Fn sigtimedwait
with a
.Dv NULL
.Fa timeout
pointer unspecified.

to the STANDARDS section.

In D21939#769857, @allanjude wrote:

Interestingly, chroot() no longer requires root privileges. Does that change things here?

I agree that OPTIND=1 is needed, but I have another reason for it. POSIX requires that a script set OPTIND=1 between using different sets of parameters with getopts; otherwise, the results are unspecified.

OK. Thanks for the reminder.

This is more consistent, and probably more useful (while read loops tend not to treat EOF and read errors differently).

OK, but perhaps this fix should be in libedit instead. The same issue can be seen in other libedit clients such as ftp(1).

This functionality clearly fills a gap, but the provided interface is too low level for applications to call. The code in lib/libc/gen/posix_spawn.c uses various non-standard facilities to handle things like signals correctly. Depending on how this is planned to be used, additional libc code may be useful, but it need not be part of this review.

In D30464#689064, @stephane.rochoy_stormshield.eu wrote:

In D30464#685956, @jilles wrote:

Looks reasonable, assuming veriexec itself is reasonable (in many cases, it seems to me that verifying the root filesystem would be a simpler and more reliable approach).

The -- was added so that source pathnames starting with - would not break. I guess destination pathnames starting with - already did not work before D25551, so that was not a regression. This could be fixed differently by prepending ./ if the name starts with -.

In D30498#686136, @fernape wrote:

As this is a small fix to the EXAMPLE, is it necessary to bump .Dd at commit time?

Looks reasonable, assuming veriexec itself is reasonable (in many cases, it seems to me that verifying the root filesystem would be a simpler and more reliable approach).

In D30330#681137, @a.wolk_fudosecurity.com wrote:

Unfortunately, some ports like PostgreSQL redefine start_cmd which would make _oomprotect="ALL" silently not work for the database. I am not sure where it would be a good place, but I think it would be worthwhile to document that redefining ${name}_cmd has such a pitfall.

The comment is correct. LOG_PID has no effect and the process ID is always included. This change was made along with the move from RFC 3164 to RFC 5424 log messages.

Some people may get negatively surprised if they first encounter this change in a git update. A post to -current@ and Relnotes: yes are probably a good idea.

I like it better if EFAULT is reserved as much as possible to the case where a syscall argument points to an invalid memory location for the calling process, which is undefined behaviour according to POSIX and other standards and often behaves poorly (for example, completing an operation but discarding the result). If the address in the target process passed to PT_READ_I is invalid, this is fully defined and may not be fully avoidable (for example if the traced process is running concurrently).

There should be documentation about this in the man page.

In D29493#661261, @bapt wrote:

@jilles do you think we should add an option to disable the load and save so people willing to have a giant history while not suffering from the load/save can do it? if yes do you have any options to suggest?