Similar to the above-mentioned case, UB per C23 §7.4.1.

Similar to above-mentioned occurrences, UB per C23 §7.4.1.

Same story, UB per C23 §7.4.1.

The return cast on the other hand is just to say the narrowing is deliberate, so it'd keep the line quiet under conversion-class diagnostics; which we don't have enabled at the time, but I still think it'd be a good practice.

This is incredibly wasteful. The ctype(3) functions are locale-aware and therefore slow, and the work they are doing here is needlessly duplicated by the later strtoul(3) call. It would be more efficient (and far more readable) to use direct comparisons and convert as you go. It would also save you from having to copy the digits before converting. Same for the octal conversion below.

Why only allocated memory? Surely a local or global non-const array will work fine? Also, we usually use the term “buffer”, not “block of memory”.

Do you think that maybe in 2026 you could do better than crash? Like take a size argument?

This function actually unexpands rather than expands. Otherwise it would not be able to operate in-place.

Passing a constant to this function invokes undefined behavior. Whether or not it segfaults is beside the point. Either way, the compiler won't let you do it, unless you force its hand.

“may safely share the buffer” is academic since you don't offer the caller a choice. It would be germane if the function took two arguments and the caller could choose to pass the same buffer to both, cf. strunvis(3).

That's not how C-style octal escapes work, DEL (ASCII 127) is encoded as \177, not \0177.

buf, src, dst` instead of source, chr, and pos would be easier to follow.

Please keep in mind that style(9) mandates sorting variables first in order of decreasing size, then alphabetically.

Why only allocated memory? Surely a local or global non-const array will work fine?

We are not checking read(2) for failure, so bsdconf_fparse() could spin forever when it fails. read() returns −1 on error, which != 0 holds true for, so every loop treats a failure as "more data available". For example, assume a descriptor open on a directory (lseek(2) succeeds, so the spool path is not taken; read(2) then fails with EISDIR forever).

Worse, the directive-length loop below increments n on every failed read and the buffer is grown to n + 1, so it is an unbounded realloc() growth loop as well as a hang. Besides, since n is uint32_t, at 4 GiB it wraps and read(fd, directive, n) is then called with a length far exceeding the allocation. bsdconf_spool() and bsdconf_readfile() both handle r < 0 correctly with if (r < 0) { if (errno == EINTR) continue; goto fail; }.

It's nothing critical, but I recommend adding the t > value guard here for parity with the upcoming occurrences for #/; trim. Safe today as the value scan always leaves t on real content first, but I think it could be a latent one-byte underflow if that invariant ever broke.

After counting a backslash run, p already points at the next character and this continue statement does another p++, so a byte after an even-length run is never examined. Unescaped # / " after \ pairs can pass validation and be written verbatim; round-trips then lose or truncate data (e.g. a\\#b becomes a\). Please re-evaluate that character when (nbs & 1) == 0 (you could possibly drop the continue or adjust p).

Besides, while here, please clarify and document whether BSDCONF_PUT_UNQUOTED values are literal bytes or pre-escaped text, since the reader runs strunexpand() but the writer does not escape.

bsdconf_put() always runs the mkstemp/write/fsync/rename path even when every option is CHECK or the value is unchanged. That violates the documented "without modifying the file" contract, bumps mtime, severs hard links, and requires write access to the directory (fails for unprivileged callers on an equal-value check). Please skip the commit when nothing would change; especially when all actions are CHECK. Same root cause seems to make idempotent writes (a=1 when already 1) non-idempotent; sysconf -c is fine because it never calls put, but library consumers are not.

The pre-scan skips the argument after a cluster whose last character is one of the option letters taking an argument. Attached forms like sysconf -f/boot/loader.conf loader rc are mishandled, as here for example, it would treat rc as the target and exec sysrc with the wrong argv.

To my understanding, any -f/-j/-k/-R value ending in A a c d D E e F f i j k l L n N q R v x is affected. Since this exists only to detect the rc pass-through, perhaps we could use real getopt parsing or at least treat attached arguments as self-contained so we don't consume the following argv element.

The behaviour here may confuse users even though it is correct for each format.

For make/src/src-env, a trailing + on the name selects the make += operator and we replace the whole assignment. So, for instance, an existing CFLAGS+=-g -pipe plus sysconf make CFLAGS+=-O2 becomes CFLAGS+=-O2, dropping -g -pipe.

For every other target, trailing +/- is a list edit and merge_list_requests() appends or strikes words in the current value, so IGNORE_LIST+=c, for example, merges into what is already there.

From maintainer perspective this matches each consumer’s syntax and looks just fine; from user perspective, though, the same CLI spelling has different effects on different targets, easily misused, especially with -q that'd hide the old -> new echo that would otherwise warn them.

I don't know whether we should internally turn it into something well established to avoid such cases and prevent foreseen misuse, but it concerns me a little, to be honest.

Somewhat related to the other comment atop bsdconf_put(), but I recommend consulting opt->result & BSDCONF_VALUE_CHANGED, regardless.

This is mostly a matter of personal preference, so it may be entirely subjective. I just think it would improve the user experience, but I'm happy with whatever you decide.

You've excluded check here, but do_checks() touches no file, so it is read-only like pure reads, right? So it's possibly worth dropping it from the condition here so checks get the same Capsicum path.

Confirmed — thank you. Attached forms were mishandled exactly as you
described (-f/boot/loader.conf loader rc treated rc as the target).

Took the second path you suggested: walk the cluster like getopt would and
treat an attached argument as self-contained, so only a trailing
option-letter with a separate argv (-f PATH) consumes the next element.
Left the pre-scan hand-rolled rather than calling getopt here, since this
path has to leave argv pristine for the rc → sysrc(8) pass-through
(and we already special-case --help/--version before getopt for the
same reason plain getopt cannot express them).

Agreed on the UX footgun — same spelling, different effect, and -q hides
the old -> new that would otherwise tip people off.

I'd rather keep the per-consumer semantics than invent a third spelling:
make/src/src-env += is the assignment operator and replaces; elsewhere
+=/-= are sysrc(8)-style list merges. That matches what each file's
reader actually does, and sysconf.8 already calls it out at the
CFLAGS+=-DDEBUG example. Happy to sharpen that wording if you think it
still undersells the surprise; I don't want to change the behavior
itself without a stronger reason.

Yes, do_checks() only compares; it never calls bsdconf_put(). Dropped
!check from the scan_pass() condition so -c gets the same sandbox as
pure reads (still excluding sysctl -d, whose kernel queries the sandbox
would deny).

Failed += replicated. Working on a fix. For others reading this:

$ sysconf make -a
USE_PACKAGE_DEPENDS: yes
MAKE_JOBS_UNSAFE: yes
DEVELOPER: yes
$ sudo sysconf make CFLAGS+="-g -pipe"
CFLAGS:  -> -g -pipe
$ sysconf make -a
USE_PACKAGE_DEPENDS: yes
MAKE_JOBS_UNSAFE: yes
DEVELOPER: yes
CFLAGS: -g -pipe
$ sudo sysconf make CFLAGS+=-O2
CFLAGS: -g -pipe -> -O2
$ sysconf make -a
USE_PACKAGE_DEPENDS: yes
MAKE_JOBS_UNSAFE: yes
DEVELOPER: yes
CFLAGS: -O2
$ cat /etc/make.conf
# ONLY = do not build missing dependencies from source, just fail
#?USE_PACKAGE_DEPENDS_ONLY=yes

USE_PACKAGE_DEPENDS=yes
MAKE_JOBS_UNSAFE=yes
DEVELOPER=yes
CFLAGS+=-O2

Well that's not going to pass muster. Not one bit. There's actually two ways += can work in the make context, it can either add a new += line because += is supported in make, or we can modify the last assignment (or we can introduce an option to allow the user to choose -- I say we keep it simple and in config formats that support += themselves, like make, we append a new line after the last assignment that uses the += operator).

This also means we need to make sure that we implement support for reading += from confs that support it so that we can accurately report the real value.

Right now, we have a verifiable, reproducible bug which needs addressing that you found.

$ make -V foo

$ sudo sysconf make foo=1
foo:  -> 1
$ make -V foo
1
$ echo 'foo+=2' | sudo tee -a /etc/make.conf
foo+=2
$ make -V foo
1 2
$ tail -n 2 /etc/make.conf
foo=1
foo+=2
$ sysconf make foo
foo: 2

make -V variable gets it right while sysconf make variable gets it wrong.

Good catch.

Piling-on to my own thought process, ... holy moly ... I just realized there's some real value-add we're leaving on the table here that should be addressed.

Just how sysconf loader ... is multi-file and addresses how FreeBSD loader gets its values (in the same vein that sysrc's whole purpose is to make sure you can interrogate as well as modify the way the boot process and service sees values in each respective context) sysconf make can (and should) be multi-file as well.

Explained simply, make -V CFLAGS references more than just /etc/make.conf to get the effective value of CFLAGS to compilation with make.

As such, sysconf make ... should reference the same variables. However, I just realized that I ultimately want more than just -v in our implementation (which currently shows only /etc/make.conf and the final value -- at-present, the incorrect value to boot).

Going to have to refactor/rewrite sysconf make to implement (proposed) -V to provide more verbosity -- display all the places that a variable's contents is changed (including appends, since make supports += as a language itself).

This is kind of a weird design that makes things very racy and non-reentrant (multiple threads or multiple program parts fighting over the same sentinel they have written to).

I don't get why you don't just return NULL on failure to find an option. Returning a pointer a sentinel instead of a null pointer is not generally considered to be good design, permitting writes to the sentinel doubly so. Just hides errors when callers failed to account for the sentinel, whereas failing to account for a null pointer return generates an easily debuggable loud null pointer dereference.

This whole logic could probably be simplified a lot if you just copied the file into an in-memory buffer first (e.g. using open_memstream) and then processed the file as an array of characters instead of doing a gazillion of IO calls. Config files are small enough that they fit into memory, particularly on modern machines.

And then make all of these bool.

You could use open_memstream to have a self-resizing buffer for the directive instead of manually reallocating.

Why do we need to call realpath here? Why not just open path directly?

Could this be a _Bool instead?

Do we really need four separate numerical types or would one (or two, singed/unsigned) suffice?

What about floating point values?

It's usually more future-proof to name things according to what they do (e.g. BSD_FORMAT_MAYQUOTE) instead of what they are for, in case new applications for existing formats obtain. Not an objection, just an item to think about.

Could these be enumerations?

Less magic numbers.

This logic is incorrect if path ends in a trailing slash.

It might be easier to copy path into an on-stack buffer and then call basename on it instead of trying to reinvent it from scratch.

Easier to read, avoids magic numbers.

No need to reinvent the wheel.

These could be bools.

These look like they should all be bools.

This may malfunction if the original file is read-only and in any case is afflicted by umask. Also restoring the setuid/setgid bits seems dangerous. I'd create the file with mode 0600 and the fchmod to sb.st_mode & (0777) for the backup.

This goes into an endless loop if the end of a medium (e.g. a tape) is reached, in which case write will keep returning 0.

You should at least error out in this case.

This function fails to account for the file growing while it is being read. It may be less error-prone to avoid the TOCTTOU race by reading the file into a variable-length buffer (e.g. using open_memstream) until EOF is encountered.

This can make the file an executable setuid binary which seems like an easy footgun causing vulnerabilities. Consider masking to 0666 to avoid this sort of issue.

Might as well use memcmp here.

This seems awfully inefficient, running in O(nm) time. How about this instead?

This should be designed to either always be in-place or always be out-of-place but not randomly one or the other.
The code can be made more efficient by using strstr to look for candidates for replacement as suggested for bsdconf_strcount.

No love for getprogname()?

These could all be bools.

Are you sure that shouldn't be 0666 as usual?

make downstream consumers happy with a simple date format.

bool?

A candidate for sysctlnametomib?

sysctlnametomib?