Details

Reviewers

kib
mckusick

Group Reviewers

cheri

Summary

Rather than passing the format string to printf and forcing the
arguments to be (u)intmax_t, partially parse format strings and cast
p1 and p2 to the correct type before running the individual format
though printf. This restructure has a couple motivatations:

We can skip formats that make no sense (floating point, %n, etc.).
It is possible to special case the printing of pointers in the CHERI case.

The first case is motivated by a suggestion from the audiance at
one of Kirk's BSDCan talks on exterr to allow userspace to set exterr
status. Allowing arbitrary format strings including %n creates a
write-what-where gadget so we need to not do that.

The second case is motivated by our experinces with CHERI and debugging
mmap issues using a different textual error reporting framework. With
CHERI, pointers are more than integer addresses and it's useful to
include more details. Doing so will follow in a future commit.

When the new code encounters an inappropriate format it includes
a diagnostic and in most cases prints the format untouched.

Effort: CHERI upstreaming
Sponsored by: Innovate UK

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 75038
Build 71921: arc lint + arc unit

Event Timeline

brooks created this revision.Mon, Jul 6, 4:06 PM

Herald added subscribers: ziaee, imp. · View Herald TranscriptMon, Jul 6, 4:06 PM

brooks requested review of this revision.Mon, Jul 6, 4:06 PM

Harbormaster completed remote builds in B74551: Diff 181371.Mon, Jul 6, 4:06 PM

Herald added a reviewer: cheri. · View Herald TranscriptMon, Jul 6, 4:06 PM

Herald added a reviewer: cheri. · View Herald Transcript

brooks added a parent revision: D58057: exterr: clear tags from arguments on CHERI targets.Mon, Jul 6, 4:06 PM

brooks added a child revision: D58059: runtime: add the ability to set exterrors in userspace.

Having had overnight to think through how to implement AT_RENAME_EXCHANGE in UFS, it will require a journal. If the operation is single threaded or at least kept to a bounded number in parallel, the log can be kept in an area that fits in the disk block holding the superblock. The journal entry simply needs to keep entries for the two names being exchanged. Specifically, the inode number of their resident directory, their name, and the inode number that they should reference. A flag in the superblock can indicate that the journal needs to be checked. If so, fsck can look up each name and verify that it references the correct inode number correcting it if it is wrong.

Does this operation allow one or both of the names to reference a directory? If so implementing it is going to be a lot more complicated since it will need to deal with '..' resolution if the names are in different directories. The code implementing it is going to start looking like rename which is no small source of maintenance headaches over the years.

Oops, sorry, posted to the wrong phabricator review.

kib added inline comments.Tue, Jul 7, 2:52 AM

include/exterr.h
18 ↗	(On Diff #181371)	Why the declaration is in this review, and not in D58059?
lib/libc/gen/uexterr_format.c
65	The function is static, it does not need to be in private namespace.
227

Address feedback

Harbormaster completed remote builds in B74806: Diff 181994.Tue, Jul 14, 8:05 PM

kib added inline comments.Sat, Jul 18, 1:35 PM

lib/libc/gen/uexterr_format.c
203	Why do we need to pass over these formats? If we exclude strings/floats etc from the strcspn argument and this cases. what would change?
224	I prefer to avoid asserts in the libraries, esp. the system libraries. This imposes a policy on the app which should be left to the app itself.

brooks added inline comments.Mon, Jul 20, 9:44 AM

lib/libc/gen/uexterr_format.c
203	We could allow them if there's a sensible case from uint64_t storage to float, but I doubt that makes sense in kernel space. It might in user space. I don't think there's a sensible thing to do with strings. You could create the storage as a char[8], but that's really low value. The reason I don't exclude them from the format is that I want to cleanly pass unhandled formats through untouched to provide something developers can see. If we don't handle them explicitly then a format like "%f%d" would print "%d" because we'd find the second `%` as the format. I think that would be confusing.
224	I can move default to the passthrough case.

Drop used of __unreachable()

Harbormaster completed remote builds in B74951: Diff 182281.Mon, Jul 20, 9:48 AM

kib added inline comments.Mon, Jul 20, 3:59 PM

lib/libc/gen/uexterr_format.c
224	I wonder if it makes sense to print something special there, to indicate that the format is not supported. After all, we are defining the behavior, not the historical precedent. It is also useful since you intend to allow users to set the error.