Details
- Reviewers
jilles
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 68322 Build 65205: arc lint + arc unit
Event Timeline
This is correct, and useful if there is a practical use case that frequently does $(alias) (but not $(alias NAME); a little more code would be needed). Please check the use cases so we add the right amount of logic here.
There are a few more builtins like fc, hash and readonly that could conditionally be executed in the same process but adding the code to allow that may not be worth it.
Note to people wanting to port this to other ash variants: this depends on option handling in alias so things like alias -=something are an error and do not create an alias (check tests/builtins/alias4.0).
I'm only intending this to target $(alias) and $(alias --). I don't think it would be common but I've at least written some code that duplicates some aliases with different handling if they exist, and found $(alias) forking unexpectedly.
Is alias -= a thing?
In poudriere, I found $(alias) but not any other form. So that suggests the current diff is as it should be.
Is alias -= a thing?
It writes an error message and returns status 2, which is safe in the same process. There is no problem in FreeBSD sh. However, there are ash variants where that command creates an alias named - which is not safe in the same process. So I think it would be good to add something like this "Note to people wanting to port this to other ash variants" to the commit message.
The check also accepts alias - to be run in the same process but that is a read-only operation and therefore also safe.
Revisiting your first comment as I was too hasty before. The change is not ready but I appreciate the acceptance.
I should mention I have a tendency to think of what something "should" do rather than what someone might practically do. Even with this change Poudriere bundles a copy of sh, but it seemed to be small enough to warrant upstreaming, but you're right that other cases should be considered.
I've been experimenting with some weird forking-builtin stuff lately and I had an assert that $(cmd) shouldn't be forking which blew up on $(alias).
There are a few more builtins like fc, hash and readonly that could conditionally be executed in the same process but adding the code to allow that may not be worth it.
I think anything that prints to stdout, is read-only, and possibly used in a script, should be supported in safe_builtin(). readonly, hash, and alias NAME fit that. Maybe not fc? I will check to see what other cases are missing.
Note to people wanting to port this to other ash variants:
I understand now you're wanting to convey these things in the commit message, and that others consider our /bin/sh as a "reference". I'll keep that in mind.
this depends on option handling in alias so things like alias -=something are an error and do not create an alias (check tests/builtins/alias4.0).
I should have said upfront I was thinking of $(alias --), due to nextopt() eating the -- _after_ the safe_builtin() check on every builtin.
That thought about nextopt() made me misread safe_builtin while looking for the "perfect" spot. But this change makes alias -- the only special one and the others checking for optional arguments. So my change is not in the right place.
I reviewed the cases. The only missing ones were the ones you mentioned. alias, alias NAME, hash, hash -v. readonly [-p] is covered by EXPORTCMD.
Ah even an interactive shell doing a fork on $(fc -l) doesn't make sense. But I'm willing to skip that one.
sh: Avoid builtin cmdsubst forking in more cases.
This adds support for: $(alias), $(alias NAME), $(hash), and $(hash -v).
The only remaining case is $(fc) which isn't covered as it is too
complex to support for little gain given it is only used interactively.
| bin/sh/eval.c | ||
|---|---|---|
| 798 | I wonder what argc == 0 case is? Do we care to change this to argc == 1 or just leave the <= 1 pattern? | |
| bin/sh/eval.c | ||
|---|---|---|
| 797–799 | The command hash -v does exactly the same as the command hash, so I don't really understand why this needs a special case. Merely allowing hash by itself would suffice, right? Something like hash -v NAME writes more detailed information but it may also modify the hash table. Also, it doesn't feel very suitable for use in scripts: for example, displaying a function definition is very incomplete. Note that the "list of remembered locations" of utilities is not part of the list in XCU 2.12 Shell Execution Environment, so a subshell need not isolate it. In fact, executing something like listing=$(ls /) adds /bin/ls to the list shown by hash. So it would be POSIXly correct to make hash always safe. Perhaps it's less confusing to only consider hash by itself safe, though. | |
| 798 | The argc == 0 case is a command without a command word (only redirections and/or assignments). It always corresponds to CMDBUILTIN and BLTINCMD, which has safe_builtin_always. So it would indeed be OK to change argc <= 1 to argc == 1 in this function. | |
| 802 | Nit: strchr(argv[1], '=') == NULL need not be parenthesized. | |
| bin/sh/eval.c | ||
|---|---|---|
| 797–799 | Given the subshell environment aspect it makes sense to make it always safe, since we're just avoiding a fork. /* XCU 2.12 Shell Execution Environment does not require remembered locations be reset in subshells. */ if (idx == HASHCMD) return (1); | |
| bin/sh/eval.c | ||
|---|---|---|
| 797–799 | Making "hash" always safe would be fine, or it could be done via -n in builtins.def. The "confusion" could be that $(hash -v foo bar) and $(hash -v foo; hash -v bar) behave differently but I don't think it matters much. A comment in the code is indeed good. | |