Repository navigation
feat(libkernel): keep the SIGIO owner of a pipe for F_GETOWN and F_SETOWN - #9557
DavidSamir wants to merge 3 commits into
Conversation
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The owner state and focused pipe tests are coherent on inspection, but there is a concrete invalid-pointer gap in the ioctl path: FIOSETOWN directly dereferences argument (*static_cast<const int*>(argument)) before validating it. FIOGETOWN explicitly returns EFAULT for a null output pointer, while FIOSETOWN with null can fault the host instead of returning guest EFAULT. Please add checked copy-in for the four-byte input (including an unreadable non-null pointer) and a regression test, matching the checked output handling. Also, the overlap report flags live shared debt edits in #9256, #8544 and #9109; those authors must agree the ordering and preserve each debt change, with a Depends on entry if sequencing is required. Finally this branch is based on 8c26eac3, 23 commits behind current main dce41a89478876a8e3ea9c346b1dac00bb31c8cd; please update and rerun relevant tests before approval. Hosted policy checks pass, product build jobs are skipped; Windows test evidence in the body is author-reported and I did not run it locally.
harisibrar1
left a comment
harisibrar1
left a comment
There was a problem hiding this comment.
Checked against main 4a17c71 and FreeBSD releng/11.4 (sys/kern/sys_pipe.c:1392-1410, sys/kern/kern_descrip.c:1043-1148, sys/fs/fifofs/fifo_vnops.c):
- The owner rules match
fsetown/fgetown: 0 clears,ESRCHkeeps the old owner, and the owner is per open file and shared bydup. The weak_ptr map is reset correctly bypipe()andclose(). - A small correction on the
FIOSETOWNNULL point:Ioctlalready returnsEFAULTfor a NULLIOC_INargument atDescriptorControl.cpp:559, before the dereference at:581. Only a non-NULL unreadable pointer gets through, andFIONBIOon main does the same. A checked copy-in would be nice for both, plus a test that NULLFIOSETOWNgivesEFAULT, which isn't covered yet. - On Linux/macOS, a host socket that is not a guest socket is classified as
Kind::Socket, soF_GETOWN/F_SETOWNon it throw. The debt sentence only names the character device and random device. Could you add that case? - On FreeBSD all opens of a FIFO share one
pipe_sigio(fifo_vnops.cpassesfip->fi_pipeto every open). Here twoopen()s of the same host FIFO keep separate owners. Worth a short debt note.
Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.
…cked read FIOSETOWN and FIONBIO read their int argument by dereferencing the guest pointer, so an unreadable non-null pointer faulted the host instead of failing with EFAULT. Both now copy the int in with ReadProcessMemory, process_vm_readv or mach_vm_read_overwrite. The owner test covers a null and an unreadable argument. The debt line notes that a host socket that is not a guest socket still throws, and that two opens of one FIFO keep separate owners.
|
Thanks @MarcZX10 and @harisibrar1. New commits, no force-push:
Re-run on the merge (Windows 11, WinLibs GCC 15.2): For the debt-row overlaps (#9256, #8544, #9109), I'm asking those authors for an order and will record it in the checklist. AI-assisted: yes |
|
The checked copy-in for FIOSETOWN/FIONBIO and the added debt notes address the concrete feedback from the earlier review; the author-reported Windows focused tests pass. I checked this head against current upstream/main: the merge is clean and no intervening main commit changes DescriptorControl.cpp, the new owner test, or the debt file, but it remains 3 ahead/11 behind (merge base d6eafa5). Please update to current main and rerun the focused fcntl/owner tests and conventions. The debt-line length is still reported as a warning by conventions (0 errors). The shared debt edit also needs recorded agreement with #9256, #8544, #9109 and the adjacent #9561 update; please add Depends on where an order is required. I found the order proposal to #9256, but no confirmation from the affected author yet. Product and relinker jobs are skipped; I did not run tests locally because the filesystem is full. |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The invalid-pointer safety issue is addressed, and the author reports the focused Windows tests passing. The remaining blockers are repository integration requirements: this branch is 3 ahead/11 behind current upstream/main 4777cac (merge base d6eafa5), so update and rerun relevant tests; and record merge-order agreement for the shared libkernel debt edit with #9256, #8544, #9109 and #9561, declaring Depends on if required. Conventions reports 0 errors with the inherited long debt-row warning. Hosted product/relinker tests are skipped. I did not run tests locally because the filesystem is full.
harisibrar1
left a comment
harisibrar1
left a comment
There was a problem hiding this comment.
Rechecked at f8698a0:
FIOSETOWNandFIONBIOnow copy their argument in throughReadGuestInt(DescriptorControl.cpp:554-574). It follows main'sCopyIovecschecks (null and wrap, thenReadProcessMemory/process_vm_readv/mach_vm_read_overwritewith a full-length check), so an unreadable pointer givesEFAULTand the owner is left unchanged. The newnullptrand0x10cases inGuestKernelFcntlOwnercover both.- The fcntl debt entry now covers the host socket on Linux/macOS and the per-open FIFO owner. All my points are addressed, thanks.
Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.
|
@DavidSamir thanks for this one. It conflicts with |
|
#15081 changes the guest flock copy-in at the top of RecordLock in DescriptorControl.cpp, next to the lines this PR changes. Proposal: yours first, mine rebases. |
What
fcntlthrew forF_GETOWNandF_SETOWNon every descriptor, andioctlthrew forFIOGETOWNandFIOSETOWN. They now follow FreeBSD 11:kern_fcntlturnsF_GETOWNandF_SETOWNintofo_ioctl(FIOGETOWN/FIOSETOWN)(sys/kern/kern_descrip.c:579-598).pipe_ioctlkeeps the end's SIGIO owner throughfsetownandfgetown(sys/kern/sys_pipe.c:1392-1399,kern_descrip.c:1043and:1148). Here:ESRCH, aspfind/pgfindfind no such process or group;dup,dup2andF_DUPFDshare it.app0.zar: all four fail withENOTTY, asvn_ioctlhands them toVOP_IOCTL(sys/kern/vfs_vnops.c:1488).No
SIGIOis ever sent.O_ASYNCstill throws, so a stored owner never implies a signal. The owner is kept in aweak_ptrmap keyed on the descriptor's shared status object, which leaves the descriptor table'sEntryandShareDescriptorunchanged.tcgetpgrp/tcsetpgrpon a pipe (TIOCGPGRP/TIOCSPGRP) will follow in a separate PR.Tested
Windows 11 Pro 26200, WinLibs GCC 15.2.0 r7, Release. This is CPU-side, so no GPU is involved.
New test
guest_kernel_fcntl_owner(core/libs/tests/GuestKernelFcntlOwner.cpp), which covers:ESRCHforpid + 1and-(pid + 1)with the owner kept; sharing throughdup; the other end staying independent; theFIOSETOWN/FIOGETOWNforms, includingEFAULTfor a NULLFIOGETOWNargument.ENOTTYfrom all four requests.EBADF.Results:
guest_kernel_fcntl,guest_dup,guest_kqueueandguest_terminalpass in 3 runs of the set. In one earlier run of the set, right after a partial rebuild,guest_kqueueaborted. It didn't reproduce in 5 solo runs and 3 more runs of the set.dupsharing);ESRCHcheck, it fails at line 60.tools/check_conventions.py: 0 errors. Its one warning is the length of thefcntldebt entry, which was already 4476 characters onmain.tools/pr_overlap.py --branchplusgit merge-tree. fix(libkernel_sys): fail fchdir on a pipe or socket with EINVAL #9131 and feat(libkernel): report open, read and close notes on watched files #7121 merge cleanly. fix(libkernel): change O_APPEND on a Windows file through fcntl #9256, fix(libkernel): report O_RDWR from F_GETFL on both ends of a pipe #8544 and fix(libc): fail with ENOTDIR for a file named with a trailing slash on Windows #9109 rewrite the same longfcntldebt line indocs/dev/debt/libkernel.md, so this PR conflicts with them on that line only. The code itself doesn't conflict, and whichever merges second keeps both changes.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)