Question: should the path-string syscall wrappers fail with EFAULT on an unreadable path pointer, and if so centrally in the path resolver or per wrapper? #9368

Activity

1. AliZedan1A commented on Oct 21, 2026

   

   AliZedan1A

   on Oct 21, 2026

   Contributor

   More actions

   I would keep the copy primitive central, but invoke it per wrapper. A readability probe in ResolvePathError_nid_no_patch is not sufficient and would still leave host crashes:

   • openat, mkdirat, fstatat and unlinkat build a native path directly for relative names, without calling the resolver.
   • symlink must copy both strings, but its target is intentionally never resolved.
   • Several wrappers read *path, call strlen, construct string_view(path) or log the pointer before they reach ResolvePathError_nid_no_patch.
   • ResolvePath_nid_no_patch also has trusted internal callers ("/app0/...", generated std::string::c_str() values and library paths). Making that low-level resolver classify every input as guest memory mixes two contracts.
   • Checking readability and then continuing to use the original pointer leaves a check/use race. FreeBSD copyinstr instead gives the syscall an owned snapshot.

   The reusable part should therefore be something equivalent to:

   int CopyGuestPath_nid_no_patch(const char* source, std::string& destination);

   It should use the same checked host-specific read mechanism as the existing ReadGuest/CopyIovecs work, copy through the first NUL into owned storage, inspect at most MAXPATHLEN (1024) bytes, return EFAULT when any required byte is unreadable, and ENAMETOOLONG when no NUL occurs in those 1024 bytes. Reading in page-bounded chunks avoids 1024 system calls while never reading beyond the page containing a terminator. The resolver can then take the copied string/string_view; no wrapper should dereference the original pointer afterward.

   Each syscall wrapper still needs to call that helper where FreeBSD performs its copyinstr, because validation precedence is part of the ABI. For two-path operations, copy in the kernel order and stop after the first copy error. The same applies to the proposed output-struct work: a shared CopyGuestOut primitive is preferable to repeating process_vm_writev / VirtualQuery / Mach code, but wrappers should decide when it runs and should fill a local object first.

   A focused regression test should cover, on all three hosts:

   • an unreadable first byte;
   • a readable prefix whose NUL lies in an unreadable next page;
   • a NUL in the last readable byte before a guard page (success);
   • 1023 bytes plus NUL (success) and 1024 bytes without NUL (ENAMETOOLONG);
   • both operands and precedence for link, symlink and rename;
   • relative *at paths, proving the non-resolver paths are covered;
   • no filesystem side effect after a copy failure.

   So my recommendation is option 2 at the wrapper boundary, backed by one shared checked C-string copy helper rather than repeated ad hoc pointer checks. This also separates the path topic cleanly from the broader 207-function bad-pointer sweep.
2. MarcZX10 commented on Oct 21, 2026

   

   MarcZX10

   on Oct 21, 2026

   Contributor

   More actions

   I found one path consumer that still appears outside #9423: access_nid_postfix in core/libs/prx/libc/src/Filesystem.cpp checks null and then evaluates *path before calling ResolvePath_nid_no_patch(path). The current #9474 symlink-target change also leaves that entry path untouched. Unless access is intentionally outside this syscall issue, an unreadable non-null pointer can still fault in the host before it returns guest EFAULT; it may be worth listing as remaining scope or covering in a follow-up. I have not tested this case locally.
3. adriadam10

   closed this as completedin #9423on Oct 25, 2026