file: Make fget() and getvnode() consistent about initializing *fpp
ClosedPublic
Actions

Authored by markj on Feb 7 2022, 3:19 PM.

Details

Reviewers

kib
mjg

Commits

rG300cfb96fc22: file: Make fget*() and getvnode*() consistent about initializing *fpp

Summary

Most fget*() functions initialize the output parameter to NULL. Make them
all behave consistently. This fixes at least one bug in a consumer,
_filemon_wrapper_openat().

Reported by: syzbot+01c0459408f896a5933a@syzkaller.appspotmail.com

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Feb 7 2022, 3:19 PM

Herald added a subscriber: imp. · View Herald TranscriptFeb 7 2022, 3:19 PM

markj requested review of this revision.Feb 7 2022, 3:19 PM

Harbormaster completed remote builds in B44319: Diff 102456.Feb 7 2022, 3:19 PM

kib added inline comments.Feb 7 2022, 3:23 PM

sys/kern/vfs_syscalls.c
4322	Should this return handled?

markj added inline comments.Feb 7 2022, 3:26 PM

sys/kern/vfs_syscalls.c
4322	fget_unlocked() modifies `*fpp` only when it returns success. I think it's ok to rely on that here. This diff is just extending that semantic to getvnode()/getvnode_path().

kib accepted this revision.Feb 7 2022, 3:39 PM

kib added inline comments.

sys/kern/vfs_syscalls.c
4322	My reaction is to the fact that 'modifying only on success' != 'setting to NULL on failure'. It is somewhat confusing for getvnode() now.

This revision is now accepted and ready to land.Feb 7 2022, 3:39 PM

markj added inline comments.Feb 7 2022, 4:53 PM

sys/kern/vfs_syscalls.c
4322	Hmm. fget() explicitly initializes `fpp = NULL`, but fget_unlocked() does not. This results in inconsistencies: fget_cap() may or may not initialize `fpp = NULL` in the error case depending on whether CAPABILITIES is defined. IMO fget_unlocked() should change as well.

Make fget_unlocked_seq() private to kern_descrip.c
Make fget* consistent about initializing the output pointer. Some routines were doing it, some not, but it seems preferable to provide some guarantees there to help ensure that buggy callers fail closed.

This revision now requires review to proceed.Feb 7 2022, 5:43 PM

Harbormaster completed remote builds in B44320: Diff 102458.Feb 7 2022, 5:43 PM

markj retitled this revision from file: Make getvnode*() set the returned file handle to NULL upon error to file: Make fget*() and getvnode*() consistent about initializing *fpp.Feb 7 2022, 5:44 PM

markj edited the summary of this revision. (Show Details)

markj added a reviewer: mjg.

kib accepted this revision.Feb 7 2022, 7:06 PM

This revision is now accepted and ready to land.Feb 7 2022, 7:06 PM

Closed by commit rG300cfb96fc22: file: Make fget*() and getvnode*() consistent about initializing *fpp (authored by markj). · Explain WhyFeb 8 2022, 6:37 PM

This revision was automatically updated to reflect the committed changes.

markj added a commit: rG300cfb96fc22: file: Make fget*() and getvnode*() consistent about initializing *fpp.

I see I'm late. Just a remark that this nullifying is pretty weird and thing to do instead is to set the pointer to a poisoned value when running with invariants, while patching all the consumers to not look at it if an error was returned. Maybe I'll get around to it.