Merge branch 'docs-hacking2': docs/hacking on bash pitfalls and bash versions.
commit 1fcf6f35d8856013c926ba6256d35ef78e64f795 · parent 64a113ffaf · parent bf37240054
· browse this tree
Ximianth, 2026-10-23 23:29:14 UTC
diff --git a/docs/hacking/bash-pitfalls.md b/docs/hacking/bash-pitfalls.md new file mode 100644 index 0000000..043cb1c --- /dev/null +++ b/docs/hacking/bash-pitfalls.md @@ -0,0 +1,185 @@ +Bash Pitfalls +============= + +Bash is hard to use safely. +There's a lot of non-obvious behaviors. +This file is reference material for the pitfalls we know of that have already steered this library. + +(There's also a whole series of details that *have changed* across versions, +and people have a wild range of bash versions running around in the wild that they expect things to work with. +But see the sibling [bash versions](bash-versions.md) file for more that.) + + +Using assoc arrays safely +------------------------ + +Assoc arrays are extremely touchy in bash and very easy to use unsafely. +There's a shocking number of ways that strings can get *evaluated* instead of used as strings. + +Read all of these sections carefully before writing +anything remotely resembling `name[$key]`, +especially with any key that isn't entirely a literal. + +Broadly, the problem is: given the very spicy string assigned here to `k`: + +```bash + k='`y[$(cmd)]`' +``` + +...one must write code which handles an assoc array with such a string as a key without ever interpreting that string. + +Handling assoc arrays correctly is absolutely critical if there's any part of their keys that can contain user-supplied input. +Even if the key strings are fully controlled by the program, it's wise to use only the safest forms of access +so that the code has some defense in depth, can be maintained without fear, +and so that safe-only-contextually idioms don't get cargo-culted into unsafe contexts. + +### assoc arrays must be declared as such + +Only assoc arrays -- declared with `-A` -- treat keys as plain strings. +Any other array treats its subscript as arithmetic, +and arithmetic expansion mode is the source of most of the "runs code in the key" problems. + +And any array syntax used on values that aren't already declared as assoc arrays +will silently make indexed arrays instead. + +You're at risk whenever an array keyed by data wasn't declared `-A` before its first use, +including when you assign to a name that doesn't exist yet, +or write to one through a nameref. + +```bash +# `put` will assign a spicy key into a given array. +k='y[$(cmd)]' +put() { local -n arr=$1; arr[$k]=v; } + +# this is safe: the key is stored as the literal string, +# because `good` is an associative array. +declare -A good; put good + +# **this is unsafe and will execute code**, +# because `bad` becomes an indexed array, +# and then arithmetic expansion runs on its keys, +# which can cause arbitrary execution! +put bad +``` + +### Never pass an assoc array element to a builtin or to arithmetic + +Builtins that take a variable name (`printf -v`, `unset`, `declare`, and the `-v` test) parse that name themselves, +after the shell has already expanded it, and does arithmetic expansion on it. +This is yet another place where a key has a shot at getting evaluated! + +(The bash man page calls this "multiple evaluation", under the `assoc_expand_once` shopt. +It's fairly insane! +But also not reliably disableable; the `assoc_expand_once` shopt didn't exist until bash `5.x`.) + +All of these are dangerous: `printf -v "a[$k]"`, `unset "a[$k]"`, `[[ -v a[$k] ]]`, or `declare -n r="a[$k]"`, or any `a[$k]` expressions inside `(( ))`. + +Use the key only in assignment and parameter expansion syntax, which bash parses once: + +```bash +a[$k]=v # assign +"${a[$k]}" # read +"${a[$k]-}" # read, where it may be missing +${a[$k]+set} # test whether it exists +"${!a[@]}" # iterate keys, in no particular order +``` + +Note: `[[ -v ]]` and `(( ))` only do this on bash 4.4 through 5.1, +so using tests on a current bash to check that we're not doing something unsafe isn't enough; +see also [bash versions](bash-versions.md). + +### Never unset a single data-keyed element + +No spelling of `unset` on one associative array element is right on every bash: +`unset "a[$k]"` runs code in the key, +and `unset 'a[$k]'` silently does nothing when `assoc_expand_once` is on. + +Drop the whole array and rebuild it instead. + + +Reading files +------------- + +### use blocks to open an fd and check properties on it without TOCTOU + +`read` and `mapfile` report a read error the same way as end of file. +This is problematic. + +And a directory, for one exciting example, reads as empty input *with no error*, in both `read` and `mapfile`. + +Handling this with tests like `-f` or `-r` would help, except... +if one does that one a path string either before or after opening it, +that'd be a time-of-check-time-of-use bug. + +So what can we do? + +Open the fd first; then check on it. + +Use a redirect to get an fd; +and send it to a block, so you can do multiple operations on it. +Like this: + +```bash +# A missing or unreadable file fails the redirect itself. +# But the `-d` check also makes sure we didn't open a directory! +{ [[ ! -d /dev/stdin ]] && mapfile lines; } <"$file" +``` + +### NUL bytes vanish unless you check for them + +Bash strings can't hold NUL, and each reader loses it differently: +`mapfile` cuts off the string at the null byte; +`$(...)` drops it with a warning; +and `read -N` drops it silently. + +Use `IFS= read -r -d '' _` to read such a string: +it'll at least *tell* you if you hit a NUL. +Exit code 0 means it stopped at a NUL, while 1 means it read to end of file. + + +Writing library functions +------------------------- + +Library code runs under whatever situation and shopts the caller has set. +That makes life tricky. + +In this library, we mostly centralized handling of that into `_testmark_call` +and wrap roughly everything with that. + +(You can go read `_testmark_call` to see what it does. +It and its docs are their own SSOT.) + +There's still two things that have to be known everywhere, +because they couldn't be solved in one place with wrappers. + +### check every exit code + +We've been unable to use `set -e` as a strict mode (unfortunately). + +That means an awful lot of `cmd || return 1` is required. + +### treat unset variables as fatal + +We turned `set -u` on in our library code, as a strict mode to catch as many bugs as possible. + +Mostly this is good and obvious. + +There's a couple pretty frequent "oof" sources, though: +reading an element of an array that may be missing, +or taking `${#a[@]}` of an array that may never have been assigned can both count as reads of unset variables. + +Solution: use `${a[$k]-}` or `${a[$k]+set}` for presence, +and iterate with `"${!a[@]}"` rather than counting +(or just... make sure your arrays are initialized, which... you should be doing anyway). + + +Quadratic operations +-------------------- + +Don't do quadratic things on document-sized data. +Two innocent-looking ones: + +- **Slicing an array per item.** `"${a[@]:i:n}"` walks the array from its start every time, so slicing out every hunk is quadratic. + Instead, loop over the indexes: `for ((j = i; j < i + n; j++))`. +- **Substituting over a whole document.** `${s//pat/rep}` is quadratic in the number of matches (CRLF to LF over a 1.2 MB string took 15 s on 4.4). + Instead, work per line, as the parse does when it strips CR. diff --git a/docs/hacking/bash-versions.md b/docs/hacking/bash-versions.md new file mode 100644 index 0000000..fcf135e --- /dev/null +++ b/docs/hacking/bash-versions.md @@ -0,0 +1,42 @@ +Bash Versions +============= + +In the wild, all of these will be seen: + +- `5.x` series -- that's the recent. +- `4.x` series -- still shipped in some distros have slow release cycles. +- `3.2` series -- shipped by Certain Device Manufacturers with a Funny Relationship to Licensing. + +This frankly kinda sucks; especially the `3.x` one. + + +Minimum Version Requirement +--------------------------- + +**We have a hard minimum version requirement** in this library: `4.4`. +We stop here because making it work on anything further back would be both a total rewrite +and also staggeringly hard (if not impossible) to make actually *safe*. + +### Older versions are detected and rejected explicitly. + +We're quite sure things either aren't going to work or aren't going to be safe in older versions, +so where that's the case, we check for those versions and reject them clearly. + +Usually I don't like version number checks and prefer feature detection. +However, with bash, that's just... not super realistic. So, version number checks it is. + +This is better than trying to amble along and failing in more obscure ways. + + +Bash Versions and Testing +------------------------- + +**Some things in the pitfalls document are only unsafe on some versions of bash**. + +**That means we need to run our test on older versions of bash** in order to make sure +we aren't regressing on our safety when running on those versions. +All tests passing on the lastest version of bash doesn't necessarily mean the code is safe and shippable. + +(Specifically, two of the assoc-key traps in the pitfalls doc only bite on bash 4.4 through 5.1.) + +We haven't automated a test harness for this. Diligence is current manually supplied.