libbsdconf: parse from a bounded in-memory buffer
ClosedPublic
Actions

Authored by dteske on Thu, Sep 17, 12:59 AM.

Details

Reviewers

fuz
kfv

Commits

rG74cab6e2371f: libbsdconf: parse from a bounded in-memory buffer

Summary

Copy the descriptor into a buffer of at most 64 MiB (raise it with
BSDCONF_MAX_BYTES) and tokenize with bsdconf_scan(), the walker
bsdconf_put() already uses. Input above the cap fails with EFBIG.
Bump libbsdconf to 1.2.0 and sysconf(8) to 2.0.

Suggested by: fuz

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 77138
Build 74021: arc lint + arc unit

Event Timeline

dteske created this revision.Thu, Sep 17, 12:59 AM

Herald added subscribers: ziaee, jonathan, imp. · View Herald TranscriptThu, Sep 17, 12:59 AM

dteske requested review of this revision.Thu, Sep 17, 12:59 AM

dteske added a parent revision: D59745: libbsdconf: Remove unnecessary parens.

Harbormaster completed remote builds in B77009: Diff 186933.Thu, Sep 17, 12:59 AM

fuz added inline comments.Thu, Sep 17, 11:22 AM

lib/libbsdconf/bsdconf.h
42	Assuming we use semantic versioning here, what is the API break that requires a major version update?
lib/libbsdconf/bsdconf_stmt.c
196	This potentially stomps over the errno value reported by `read`.
206	There are some theoretical reasons why growing the cap by the golden ration (approx. 13/8) is better, but doubling the size is certainly workable.
207	This makes the intent easier to understand. Or use `ckd_mul` from `<stdckdint.h>` to check for overflow (requires C23).

dteske added inline comments.Mon, Sep 21, 3:29 PM

lib/libbsdconf/bsdconf.h
42	You're right. The API did not break, but I was thinking in terms of functionality. However, the logic remains that a major bump should coincide with a change to SHLIB_MAJOR -- and since I don't feel that the changes have risen to the need to bump the library major, that we likewise shouldn't be bumping version major here. I'll drop the major back to 1 and bump minor (resetting patch level to zero as well)

dteske added inline comments.Mon, Sep 21, 3:48 PM

lib/libbsdconf/bsdconf_stmt.c

207

We still compile with POSIX.1-2008 / C99 so I'd rather avoid the use of ckd_mul at this time.

ASIDE: POSIX.1-2008 / C99 in libbsdconf verified with a test-compile locked to those versions (see below).

cd /usr/src/lib/libbsdconf
for f in bsdconf.c bsdconf_format.c bsdconf_format_generic.c \
    bsdconf_format_loader.c bsdconf_format_make.c bsdconf_format_src.c \
    bsdconf_format_sysctl.c bsdconf_put.c bsdconf_stmt.c bsdconf_string.c
do
	cc -std=c99 -D_POSIX_C_SOURCE=200809L \
	    -Werror=implicit-function-declaration -Werror=undef \
	    -I. -c "$f" -o /tmp/${f%.c}.o
done
ls -ltr /tmp/*.o
for f in bsdconf.c bsdconf_format.c bsdconf_format_generic.c \
    bsdconf_format_loader.c bsdconf_format_make.c bsdconf_format_src.c \
    bsdconf_format_sysctl.c bsdconf_put.c bsdconf_stmt.c bsdconf_string.c
do
	rm -fv /tmp/${f%.c}.o
done

dteske added inline comments.Mon, Sep 21, 4:15 PM

lib/libbsdconf/bsdconf_stmt.c
206	Golden ratio steps from 8 KiB are not page multiples, so the early rungs often pay the next page anyway. Doubling from 8 KiB already lands on page boundaries

dteske edited the summary of this revision. (Show Details)Mon, Sep 21, 4:22 PM

Address review comments from fuz:

Drop BSDCONF_VERSION to 1.2.0 (no API break, so no major bump; SHLIB_MAJOR stays 1). sysconf(8) remains 2.0 on its own clock.
Preserve errno across free(3) on the slurp error paths.
After doubling, treat wrap as ncap <= cap (with ncap > maxb still clamping to the ceiling).

Harbormaster completed remote builds in B77138: Diff 187339.Mon, Sep 21, 4:23 PM

dteske marked 4 inline comments as done.Mon, Sep 21, 4:24 PM

Looks good!

lib/libbsdconf/bsdconf.h
42	I don't think we changed functionality in any backwards-incompatible way here. Callers expecting the previous semantics will be fine and I see no way they won't be.
lib/libbsdconf/bsdconf_stmt.c
207	You don't want to do multiples of pages as afaik jemalloc prepends metadata to the allocation, so it'll always just be one almost full page more than what we need. In any case, golden ratio increments make it easier to reuse the space freed from previous allocations. Not an important point though.

This revision is now accepted and ready to land.Mon, Sep 21, 8:09 PM

dteske marked an inline comment as done.Tue, Sep 22, 10:18 PM

dteske added inline comments.

lib/libbsdconf/bsdconf.h
42	I think bumping MINOR is appropriate because if they wanted to parse a 100MiB file, previously no problem while now they need to modify a documented environment variable. That's the only incompatibility from previous behavior
lib/libbsdconf/bsdconf_stmt.c
207	I looked this up, and was not aware before you brought it to my attention. If you still want golden ratio, just say the word and I'll do a follow-up.