Menu
▾
▴
[Valgrind-developers] [valgrind] Bug 505673 - Valgrind crashes with an internal error and SIGBUS when the guest tries to open its own
|
From: Paul F. <pa...@so...> - 2025-07-19 15:10:49
|
https://sourceware.org/cgit/valgrind/commit/?id=7fb17b67f40eb8197c45b5f575daf4fa77d16faa commit 7fb17b67f40eb8197c45b5f575daf4fa77d16faa Author: Paul Floyd <pj...@wa...> Date: Sat Jul 19 15:10:31 2025 +0200 Bug 505673 - Valgrind crashes with an internal error and SIGBUS when the guest tries to open its own file with O_WRONLY|O_CREAT|O_TRUNC This is all quite messy. It affects open() openat() and openat2() (the last of which is Linux only). On Linux we also need to check for /proc/self/exe and /proc/PID/exe. On Linux there are also a couple of RESOLVE flags for openat2() that mean _don't_ check /proc magic links. In the general case we need to have some reference to check whether the filename matches the guest filename. So I've added that as VG_(resolved_exename) (which I was already using on FreeBSD). The pathname also needs to be canonicalised. It may be a relative path, symlink or use RESOLVE_IN_ROOT. That uses VG_(realpath) (again which was already present for FreBSD). On illumos the man page says that opening running binaries for writing failes with errno set to ETXTBSY but that's not what the open functions do - they just open the file. So I've done nothing for illumos or Solaris. Maybe I'll open an illumos ticket. I haven't tried on Darwin. The Linux open functions with /proc/self/exe and /proc/PID/exe were just calling dup on the fd that we hold for the client exe. That means that we were ignoring any other flags. That has now changed. If the open doesn't fail because the WRONLY/RDWR flags are set then the syscall gets called from the PRE wrapper using VG_(resolved_exename) instewad of the /proc pathname. I haven't tried to handle all of the Linux openat2 RESOLVE* flags. RESOLVE_NO_MAGICLINKS is handled and I see the LTS test openat202 now passing, so this should also fix Bug 506910. I'm not sure that VG_(realpath) handles all forms of weird path resolution on Linux (on FreeBSD it uses a syscall so that should work OK). Diff: --- .gitignore | 2 + NEWS | 2 + coregrind/m_initimg/initimg-freebsd.c | 3 + coregrind/m_initimg/initimg-linux.c | 63 +++++- coregrind/m_libcfile.c | 20 +- coregrind/m_syswrap/syswrap-freebsd.c | 33 +++ coregrind/m_syswrap/syswrap-generic.c | 48 ++++- coregrind/m_syswrap/syswrap-linux.c | 222 +++++++++++++++------ coregrind/pub_core_libcfile.h | 3 +- include/pub_tool_libcfile.h | 2 - memcheck/tests/x86-linux/scalar_openat2.stderr.exp | 6 +- none/tests/freebsd/Makefile.am | 6 +- none/tests/freebsd/open_client.cpp | 108 ++++++++++ none/tests/freebsd/open_client.stderr.exp | 0 none/tests/freebsd/open_client.vgtest | 3 + none/tests/linux/Makefile.am | 3 + none/tests/linux/open_client.cpp | 191 ++++++++++++++++++ none/tests/linux/open_client.stderr.exp | 0 none/tests/linux/open_client.vgtest | 2 + 19 files changed, 643 insertions(+), 74 deletions(-) diff --git a/.gitignore b/.gitignore index 69e182f757..44454d08bb 100644 --- a/.gitignore +++ b/.gitignore @@ -1895,6 +1895,7 @@ /none/tests/linux/mremap4 /none/tests/linux/mremap5 /none/tests/linux/mremap6 +/none/tests/linux/open_client /none/tests/linux/pthread-stack /none/tests/linux/stack-overflow @@ -2328,6 +2329,7 @@ none/tests/freebsd/bug499212 /none/tests/freebsd/swapcontext /none/tests/freebsd/fexecve /none/tests/freebsd/hello_world +/none/tests/freebsd/open_client /none/tests/freebsd/proc_pid_file /none/tests/freebsd/sanity_level_thread /none/tests/freebsd/usrstack diff --git a/NEWS b/NEWS index e61b56baf9..af88103641 100644 --- a/NEWS +++ b/NEWS @@ -52,6 +52,8 @@ are not entered into bugzilla tend to get forgotten about or ignored. AMD64_GET_TLSBASE 505228 Wrap linux specific mseal syscall 502968 Wrap linux specific syscalls 457 (listmount) and 458 (statmount) +505673 Valgrind crashes with an internal error and SIGBUS when + the guest tries to open its own file with O_WRONLY|O_CREAT|O_TRUNC 506076 unimplemented fcntl command: 1028 (F_CREATED_QUERY) 506499 Unhandled syscall 592 (exterrctl - FreeBSD 506795 Better report which clone flags are problematic diff --git a/coregrind/m_initimg/initimg-freebsd.c b/coregrind/m_initimg/initimg-freebsd.c index cb98eb9144..5c167e3fb1 100644 --- a/coregrind/m_initimg/initimg-freebsd.c +++ b/coregrind/m_initimg/initimg-freebsd.c @@ -334,6 +334,9 @@ static const struct auxv *find_auxv(const UWord* sp) return (const struct auxv *)sp; } +/* + * @todo PJF Make this multi-platform + */ static Bool try_get_interp(const HChar* args_exe, HChar* interp_out) { HChar hdr[4096]; diff --git a/coregrind/m_initimg/initimg-linux.c b/coregrind/m_initimg/initimg-linux.c index 483b7a3ddd..bcca59da74 100644 --- a/coregrind/m_initimg/initimg-linux.c +++ b/coregrind/m_initimg/initimg-linux.c @@ -385,9 +385,59 @@ struct auxv *find_auxv(UWord* sp) return (struct auxv *)sp; } +/* + * @todo PJF Make this multi-platform + */ +static Bool try_get_interp(const HChar* args_exe, HChar* interp_out) +{ + HChar hdr[4096]; + Int len = sizeof hdr; + SysRes res; + Int fd; + HChar* end; + HChar* cp; + HChar* interp; + + res = VG_(open)(args_exe, VKI_O_RDONLY, 0); + if (sr_isError(res)) { + return False; + } else { + fd = sr_Res(res); + } + + res = VG_(pread)(fd, hdr, len, 0); + + if (sr_isError(res)) { + VG_(close)(fd); + return False; + } else { + len = sr_Res(res); + } + + if (0 != VG_(memcmp)(hdr, "#!", 2)) { + VG_(close)(fd); + return False; + } + + end = hdr + len; + interp = hdr + 2; + while (interp < end && (*interp == ' ' || *interp == '\t')) + interp++; + + for (cp = interp; cp < end && !VG_(isspace)(*cp); cp++) + ; + + *cp = '\0'; + + VG_(sprintf)(interp_out, "%s", interp); + + VG_(close)(fd); + return True; +} + static Addr setup_client_stack( void* init_sp, - HChar** orig_envp, + HChar** orig_envp, const ExeInfo* info, UInt** client_auxv, Addr clstack_end, @@ -953,6 +1003,17 @@ Addr setup_client_stack( void* init_sp, vg_assert((strtab-stringbase) == stringsize); + if (VG_(resolved_exename) == NULL) { + const HChar *exe_name = VG_(find_executable)(VG_(args_the_exename)); + HChar interp_name[VKI_PATH_MAX]; + if (try_get_interp(exe_name, interp_name)) { + exe_name = interp_name; + } + HChar resolved_name[VKI_PATH_MAX]; + VG_(realpath)(exe_name, resolved_name); + VG_(resolved_exename) = VG_(strdup)("initimg-linux.sre.1", resolved_name); + } + /* client_SP is pointing at client's argc/argv */ if (0) VG_(printf)("startup SP = %#lx\n", client_SP); diff --git a/coregrind/m_libcfile.c b/coregrind/m_libcfile.c index 767f34522c..ff1ead4e71 100644 --- a/coregrind/m_libcfile.c +++ b/coregrind/m_libcfile.c @@ -674,15 +674,18 @@ Int VG_(fstat) ( Int fd, struct vg_stat* vgbuf ) # endif } -#if defined(VGO_freebsd) /* extend this to other OSes as and when needed */ SysRes VG_(lstat) ( const HChar* file_name, struct vg_stat* vgbuf ) { SysRes res; VG_(memset)(vgbuf, 0, sizeof(*vgbuf)); -#if (__FreeBSD_version < 1200031) +#if !defined(VGO_freebsd) || (__FreeBSD_version < 1200031) +#if defined(VGO_freebsd) struct vki_freebsd11_stat buf; +#else + struct vki_stat buf; +#endif res = VG_(do_syscall2)(__NR_lstat, (UWord)file_name, (UWord)&buf); #else struct vki_stat buf; @@ -693,7 +696,6 @@ SysRes VG_(lstat) ( const HChar* file_name, struct vg_stat* vgbuf ) } return res; } -#endif #undef TRANSLATE_TO_vg_stat #undef TRANSLATE_statx_TO_vg_stat @@ -1803,7 +1805,6 @@ const HChar *VG_(dirname)(const HChar *path) return buf; } -#if defined(VGO_freebsd) /* * I did look at nicking this from FreeBSD, it's fairly easy to port * but I was put off by the copyright and 3-clause licence @@ -1819,7 +1820,7 @@ Bool VG_(realpath)(const HChar *path, HChar *resolved) { vg_assert(path); vg_assert(resolved); -#if (__FreeBSD_version >= 1300080) +#if defined(VGO_freebsd) && (__FreeBSD_version >= 1300080) return !sr_isError(VG_(do_syscall5)(__NR___realpathat, VKI_AT_FDCWD, (RegWord)path, (RegWord)resolved, VKI_PATH_MAX, 0)); #else // poor man's realpath @@ -1847,7 +1848,13 @@ Bool VG_(realpath)(const HChar *path, HChar *resolved) if (resolved_name[0] == '.' && resolved_name[1] == '/') { resolved_name += 2; } - VG_(snprintf)(resolved, VKI_PATH_MAX, "%s/%s", VG_(get_startup_wd)(), resolved_name); + HChar wd[VKI_PATH_MAX]; +#if defined(VGO_linux) || defined(VGO_solaris) + res = VG_(do_syscall2)(__NR_getcwd, (UWord)wd, VKI_PATH_MAX); +#elif defined(VGO_freebsd) + res = VG_(do_syscall2)(__NR___getcwd, (UWord)wd, VKI_PATH_MAX); +#endif + VG_(snprintf)(resolved, VKI_PATH_MAX, "%s/%s", wd, resolved_name); } else { VG_(snprintf)(resolved, VKI_PATH_MAX, "%s", resolved_name); } @@ -1855,7 +1862,6 @@ Bool VG_(realpath)(const HChar *path, HChar *resolved) return True; #endif } -#endif /*--------------------------------------------------------------------*/ diff --git a/coregrind/m_syswrap/syswrap-freebsd.c b/coregrind/m_syswrap/syswrap-freebsd.c index 79e30f7d37..df223a2a05 100644 --- a/coregrind/m_syswrap/syswrap-freebsd.c +++ b/coregrind/m_syswrap/syswrap-freebsd.c @@ -5347,6 +5347,39 @@ PRE(sys_freebsd11_mknodat) // int openat(int fd, const char *path, int flags, ...); PRE(sys_openat) { + // check that we are not trying to open the client exe for writing + if ((ARG3 & VKI_O_WRONLY) || + (ARG3 & VKI_O_RDWR)) { + vg_assert(VG_(resolved_exename) && VG_(resolved_exename)[0] == '/'); + Int fd = ARG1; + const HChar* path = (const HChar*)ARG2; + if (ML_(safe_to_deref)(path, 1)) { // we need something like a "ML_(safe_to_deref_path)" that does a binary search for the addressable length, and maybe nul + if (fd == VKI_AT_FDCWD) { + HChar tmp[VKI_PATH_MAX]; + if (VG_(realpath)(path, tmp)) { + if (!VG_(strcmp)(tmp, VG_(resolved_exename))) { + SET_STATUS_Failure( VKI_ETXTBSY ); + } + } + } else { + const HChar* dirname; + if (VG_(resolve_filename)(fd, &dirname) == False) { + goto no_client_write; // let the OS do the error handling + } + HChar tmp1[VKI_PATH_MAX]; + VG_(snprintf)(tmp1, VKI_PATH_MAX, "%s/%s", dirname, path); + tmp1[VKI_PATH_MAX - 1] = '\0'; + //VG_(free)((void*)dirname); + HChar tmp2[VKI_PATH_MAX]; + if (VG_(realpath)(tmp1, tmp2)) { + if (!VG_(strcmp)(tmp2, VG_(resolved_exename))) { + SET_STATUS_Failure( VKI_ETXTBSY ); + } + } + } + } + } +no_client_write: if (ARG3 & VKI_O_CREAT) { // 4-arg version PRINT("sys_openat ( %" FMT_REGWORD "u, %#" FMT_REGWORD "x(%s), %" FMT_REGWORD "u, %" FMT_REGWORD "u )",ARG1,ARG2,(char*)ARG2,ARG3,ARG4); diff --git a/coregrind/m_syswrap/syswrap-generic.c b/coregrind/m_syswrap/syswrap-generic.c index 1b3b5e80d6..727766869f 100644 --- a/coregrind/m_syswrap/syswrap-generic.c +++ b/coregrind/m_syswrap/syswrap-generic.c @@ -4602,8 +4602,23 @@ Bool ML_(handle_self_exe_open)(SyscallStatus *status, const HChar *filename, } #endif // defined(VGO_linux) +// int open(const char *path, int flags, mode_t mode); PRE(sys_open) { +#if defined(VGO_linux) + HChar name[30]; // large enough + Bool proc_self_exe = False; + + /* Check for /proc/self/exe or /proc/<pid>/exe case + * first so that we can then use the later checks. */ + VG_(sprintf)(name, "/proc/%d/exe", VG_(getpid)()); + if (ML_(safe_to_deref)( (void*)(Addr)ARG1, 1 ) + && (VG_(strcmp)((HChar *)(Addr)ARG1, name) == 0 + || VG_(strcmp)((HChar *)(Addr)ARG1, "/proc/self/exe") == 0)) { + proc_self_exe = True; + } +#endif + if (ARG2 & VKI_O_CREAT) { // 3-arg version PRINT("sys_open ( %#" FMT_REGWORD "x(%s), %ld, %ld )",ARG1, @@ -4619,13 +4634,39 @@ PRE(sys_open) } PRE_MEM_RASCIIZ( "open(filename)", ARG1 ); + // check that we are not trying to open the client exe for writing + if ((ARG2 & VKI_O_WRONLY) || + (ARG2 & VKI_O_RDWR)) { +#if defined(VGO_linux) + if (proc_self_exe) { + + SET_STATUS_Failure( VKI_ETXTBSY ); + return; + } else { +#endif + vg_assert(VG_(resolved_exename)); + const HChar* path = (const HChar*)ARG1; + if (ML_(safe_to_deref)(path, 1)) { + // we need something like a "ML_(safe_to_deref_path)" that does a binary search for the addressable length, and maybe nul + HChar tmp[VKI_PATH_MAX]; + if (VG_(realpath)(path, tmp)) { + if (!VG_(strcmp)(tmp, VG_(resolved_exename))) { + SET_STATUS_Failure( VKI_ETXTBSY ); + return; + } + } + } +#if defined(VGO_linux) + } +#endif + } + #if defined(VGO_linux) /* Handle the case where the open is of /proc/self/cmdline or /proc/<pid>/cmdline, and just give it a copy of the fd for the fake file we cooked up at startup (in m_main). Also, seek the cloned fd back to the start. */ { - HChar name[30]; // large enough HChar* arg1s = (HChar*) (Addr)ARG1; SysRes sres; @@ -4648,6 +4689,11 @@ PRE(sys_open) if (ML_(handle_auxv_open)(status, (const HChar *)(Addr)ARG1, ARG2) || ML_(handle_self_exe_open)(status, (const HChar *)(Addr)ARG1, ARG2)) return; + + if (proc_self_exe) { + // do the syscall with VG_(resolved_exename) + SET_STATUS_from_SysRes(VG_(do_syscall3)(SYSNO, (Word)VG_(resolved_exename), ARG2, ARG3)); + } #endif // defined(VGO_linux) /* Otherwise handle normally */ diff --git a/coregrind/m_syswrap/syswrap-linux.c b/coregrind/m_syswrap/syswrap-linux.c index bd563d9894..e16d293cd0 100644 --- a/coregrind/m_syswrap/syswrap-linux.c +++ b/coregrind/m_syswrap/syswrap-linux.c @@ -5966,25 +5966,89 @@ PRE(sys_openat) { HChar name[30]; // large enough SysRes sres; + Bool proc_self_exe = False; + + /* Check for /proc/self/exe or /proc/<pid>/exe case + * first so that we can then use the later checks. */ + VG_(sprintf)(name, "/proc/%d/exe", VG_(getpid)()); + if (ML_(safe_to_deref)( (void*)(Addr)ARG2, 1 ) + && (VG_(strcmp)((HChar *)(Addr)ARG2, name) == 0 + || VG_(strcmp)((HChar *)(Addr)ARG2, "/proc/self/exe") == 0)) { + proc_self_exe = True; + } if (ARG3 & VKI_O_CREAT) { // 4-arg version PRINT("sys_openat ( %ld, %#" FMT_REGWORD "x(%s), %ld, %ld )", SARG1, ARG2, (HChar*)(Addr)ARG2, SARG3, SARG4); PRE_REG_READ4(long, "openat", - int, dfd, const char *, filename, int, flags, int, mode); + int, dirfd, const char *, pathname, int, flags, int, mode); } else { // 3-arg version PRINT("sys_openat ( %ld, %#" FMT_REGWORD "x(%s), %ld )", SARG1, ARG2, (HChar*)(Addr)ARG2, SARG3); PRE_REG_READ3(long, "openat", - int, dfd, const char *, filename, int, flags); + int, dirfd, const char *, pathname, int, flags); } - PRE_MEM_RASCIIZ( "openat(filename)", ARG2 ); + PRE_MEM_RASCIIZ( "openat(pathname)", ARG2 ); - /* For absolute filenames, dfd is ignored. If dfd is AT_FDCWD, - filename is relative to cwd. When comparing dfd against AT_FDCWD, + // check that we are not trying to open the client exe for writing + if ((ARG3 & VKI_O_WRONLY) || + (ARG3 & VKI_O_RDWR)) { + if (proc_self_exe) { + SET_STATUS_Failure( VKI_ETXTBSY ); + return; + } else { + vg_assert(VG_(resolved_exename)); + if (!VG_(strncmp)(VG_(resolved_exename), "#!", 2)) { + goto no_client_write; + } + Int fd = ARG1; + const HChar* path = (const HChar*)ARG2; + if (ML_(safe_to_deref)(path, 1)) { + // we need something like a "ML_(safe_to_deref_path)" that does a binary search for the addressable length, and maybe nul + if (path[0] == '/') { + // ignore fd + HChar tmp[VKI_PATH_MAX]; + if (VG_(realpath)(path, tmp)) { + if (!VG_(strcmp)(tmp, VG_(resolved_exename))) { + SET_STATUS_Failure( VKI_ETXTBSY ); + return; + } + } + } else { + if (fd == VKI_AT_FDCWD) { + HChar tmp[VKI_PATH_MAX]; + if (VG_(realpath)(path, tmp)) { + if (!VG_(strcmp)(tmp, VG_(resolved_exename))) { + SET_STATUS_Failure( VKI_ETXTBSY ); + } + } + } else { + const HChar* dirname; + if (VG_(resolve_filename)(fd, &dirname) == False) { + goto no_client_write; // let the OS do the error handling + } + HChar tmp1[VKI_PATH_MAX]; + VG_(snprintf)(tmp1, VKI_PATH_MAX, "%s/%s", dirname, path); + tmp1[VKI_PATH_MAX - 1] = '\0'; + HChar tmp2[VKI_PATH_MAX]; + if (VG_(realpath)(tmp1, tmp2)) { + if (!VG_(strcmp)(tmp2, VG_(resolved_exename))) { + SET_STATUS_Failure( VKI_ETXTBSY ); + return; + } + } + } + } + } + } + } +no_client_write: + + /* For absolute filenames, dirfd is ignored. If dirfd is AT_FDCWD, + filename is relative to cwd. When comparing dirfd against AT_FDCWD, be sure only to compare the bottom 32 bits. */ if (ML_(safe_to_deref)( (void*)(Addr)ARG2, 1 ) && *(Char *)(Addr)ARG2 != '/' @@ -6027,20 +6091,10 @@ PRE(sys_openat) return; } - /* And for /proc/self/exe or /proc/<pid>/exe case. */ + if (proc_self_exe) { - VG_(sprintf)(name, "/proc/%d/exe", VG_(getpid)()); - if (ML_(safe_to_deref)( (void*)(Addr)ARG2, 1 ) - && (VG_(strcmp)((HChar *)(Addr)ARG2, name) == 0 - || VG_(strcmp)((HChar *)(Addr)ARG2, "/proc/self/exe") == 0)) { - sres = VG_(dup)( VG_(cl_exec_fd) ); - SET_STATUS_from_SysRes( sres ); - if (!sr_isError(sres)) { - OffT off = VG_(lseek)( sr_Res(sres), 0, VKI_SEEK_SET ); - if (off < 0) - SET_STATUS_Failure( VKI_EMFILE ); - } - return; + // do the syscall with VG_(resolved_exename) + SET_STATUS_from_SysRes(VG_(do_syscall4)(SYSNO, ARG1, (Word)VG_(resolved_exename), ARG3, ARG4)); } /* Otherwise handle normally */ @@ -13900,43 +13954,109 @@ PRE(sys_openat2) { HChar name[30]; // large enough SysRes sres; - struct vki_open_how * how; + struct vki_open_how* how = (struct vki_open_how *)ARG3; + Bool proc_self_exe = False; + Bool can_deref_how = how && ML_(safe_to_deref)(how, sizeof(*how)); + + // ARG4 is supposed to be sizeof(struct vki_open_how) + // but we can't trust it + if (can_deref_how) { + // check that we are not trying to open the client exe for writing + // this doesn't handle all of the RESOLVE options, there may be cases + // like RESOLVE_NO_XDEV and RESOLVE_BENEATH where the path is + // invalid and we might return the wrong errno + if (how->vki_resolve != VKI_RESOLVE_IN_ROOT && + how->vki_resolve != VKI_RESOLVE_NO_MAGICLINKS) { + /* Check for /proc/self/exe or /proc/<pid>/exe case + * first so that we can then use the later checks. */ + VG_(sprintf)(name, "/proc/%d/exe", VG_(getpid)()); + if (ML_(safe_to_deref)( (void*)(Addr)ARG2, 1 ) + && (VG_(strcmp)((HChar *)(Addr)ARG2, name) == 0 + || VG_(strcmp)((HChar *)(Addr)ARG2, "/proc/self/exe") == 0)) { + proc_self_exe = True; + } + } + } PRINT("sys_openat2 ( %ld, %#" FMT_REGWORD "x(%s), %#" FMT_REGWORD "x, %ld )", SARG1, ARG2, (HChar*)(Addr)ARG2, ARG3, SARG4); PRE_REG_READ4(long, "openat2", - int, dfd, const char *, filename, struct vki_open_how *, how, vki_size_t, size); + int, dirfd, const char *, pathname, struct vki_open_how *, how, vki_size_t, size); - PRE_MEM_RASCIIZ( "openat2(filename)", ARG2 ); + PRE_MEM_RASCIIZ( "openat2(pathname)", ARG2 ); PRE_MEM_READ( "openat2(how)", ARG3, sizeof(struct vki_open_how)); - /* For absolute filenames, dfd is ignored. If dfd is AT_FDCWD, - filename is relative to cwd. When comparing dfd against AT_FDCWD, + if (can_deref_how && + ((how->vki_flags & VKI_O_WRONLY) || + (how->vki_flags & VKI_O_RDWR))) { + if (proc_self_exe) { + SET_STATUS_Failure( VKI_ETXTBSY ); + return; + } else { + vg_assert(VG_(resolved_exename)); + if (!VG_(strncmp)(VG_(resolved_exename), "#!", 2)) { + goto no_client_write; + } + Int fd = ARG1; + const HChar* path = (const HChar*)ARG2; + if (ML_(safe_to_deref)(path, 1)) { + // we need something like a "ML_(safe_to_deref_path)" that does a binary search for the addressable length, and maybe nul + if (path[0] == '/' && how->vki_resolve != VKI_RESOLVE_IN_ROOT) { + // absolute path, ignore fd + HChar tmp[VKI_PATH_MAX]; + if (VG_(realpath)(path, tmp)) { + if (!VG_(strcmp)(tmp, VG_(resolved_exename))) { + SET_STATUS_Failure( VKI_ETXTBSY ); + return; + } + } + } else { + // relative or rooted path + // or RESOLVE_IN_ROOT + if (how->vki_resolve == VKI_RESOLVE_IN_ROOT) { + while (*path == '/' && *path != '\0') { + ++path; + } + } + if (fd == VKI_AT_FDCWD) { + HChar tmp[VKI_PATH_MAX]; + if (VG_(realpath)(path, tmp)) { + if (!VG_(strcmp)(tmp, VG_(resolved_exename))) { + SET_STATUS_Failure( VKI_ETXTBSY ); + return; + } + } + } else { + // build absolute path from fd and path + const HChar* dirname; + if (VG_(resolve_filename)(fd, &dirname) == False) { + goto no_client_write; // let the OS do the error handling + } + HChar tmp1[VKI_PATH_MAX]; + VG_(snprintf)(tmp1, VKI_PATH_MAX, "%s/%s", dirname, path); + tmp1[VKI_PATH_MAX - 1] = '\0'; + HChar tmp2[VKI_PATH_MAX]; + if (VG_(realpath)(tmp1, tmp2)) { + if (!VG_(strcmp)(tmp2, VG_(resolved_exename))) { + SET_STATUS_Failure( VKI_ETXTBSY ); + return; + } + } + } + } + } + } + } + no_client_write: + + /* For absolute filenames, dirfd is ignored. If dirfd is AT_FDCWD, + filename is relative to cwd. When comparing dirfd against AT_FDCWD, be sure only to compare the bottom 32 bits. */ if (ML_(safe_to_deref)( (void*)(Addr)ARG2, 1 ) && *(Char *)(Addr)ARG2 != '/' && ((Int)ARG1) != ((Int)VKI_AT_FDCWD) - && !ML_(fd_allowed)(ARG1, "openat2", tid, False)) + && !ML_(fd_allowed)(ARG1, "openat", tid, False)) SET_STATUS_Failure( VKI_EBADF ); - - how = (struct vki_open_how *)ARG3; - - if (how && ML_(safe_to_deref) (how, sizeof(struct vki_open_how))) { - if (how->vki_mode) { - if (!(how->vki_flags & ((vki_uint64_t)VKI_O_CREAT | VKI_O_TMPFILE))) { - SET_STATUS_Failure( VKI_EINVAL ); - } - } - if (how->vki_resolve & ~((vki_uint64_t)VKI_RESOLVE_NO_XDEV | - VKI_RESOLVE_NO_MAGICLINKS | - VKI_RESOLVE_NO_SYMLINKS | - VKI_RESOLVE_BENEATH | - VKI_RESOLVE_IN_ROOT | - VKI_RESOLVE_CACHED)) { - SET_STATUS_Failure( VKI_EINVAL ); - } - } - /* Handle the case where the open is of /proc/self/cmdline or /proc/<pid>/cmdline, and just give it a copy of the fd for the fake file we cooked up at startup (in m_main). Also, seek the @@ -13972,20 +14092,10 @@ PRE(sys_openat2) return; } - /* And for /proc/self/exe or /proc/<pid>/exe case. */ + if (proc_self_exe) { - VG_(sprintf)(name, "/proc/%d/exe", VG_(getpid)()); - if (ML_(safe_to_deref)( (void*)(Addr)ARG2, 1 ) - && (VG_(strcmp)((HChar *)(Addr)ARG2, name) == 0 - || VG_(strcmp)((HChar *)(Addr)ARG2, "/proc/self/exe") == 0)) { - sres = VG_(dup)( VG_(cl_exec_fd) ); - SET_STATUS_from_SysRes( sres ); - if (!sr_isError(sres)) { - OffT off = VG_(lseek)( sr_Res(sres), 0, VKI_SEEK_SET ); - if (off < 0) - SET_STATUS_Failure( VKI_EMFILE ); - } - return; + // do the syscall with VG_(resolved_exename) + SET_STATUS_from_SysRes(VG_(do_syscall4)(SYSNO, ARG1, (Word)VG_(resolved_exename), ARG3, ARG4)); } /* Otherwise handle normally */ diff --git a/coregrind/pub_core_libcfile.h b/coregrind/pub_core_libcfile.h index dc243bf7fe..8411ad3687 100644 --- a/coregrind/pub_core_libcfile.h +++ b/coregrind/pub_core_libcfile.h @@ -110,9 +110,8 @@ extern Int VG_(mkstemp) ( const HChar* part_of_name, /*OUT*/HChar* fullname ); return if the working directory couldn't be found. */ extern void VG_(record_startup_wd) ( void ); -#if defined(VGO_freebsd) +/* Resolves a path to a canonical absolute path */ extern Bool VG_(realpath)(const HChar *path, HChar *resolved); -#endif #endif // __PUB_CORE_LIBCFILE_H diff --git a/include/pub_tool_libcfile.h b/include/pub_tool_libcfile.h index ff4c0d3a57..5b912d31c1 100644 --- a/include/pub_tool_libcfile.h +++ b/include/pub_tool_libcfile.h @@ -81,9 +81,7 @@ extern Int VG_(pipe) ( Int fd[2] ); extern Off64T VG_(lseek) ( Int fd, Off64T offset, Int whence ); extern SysRes VG_(stat) ( const HChar* file_name, struct vg_stat* buf ); -#if defined(VGO_freebsd) extern SysRes VG_(lstat) ( const HChar* file_name, struct vg_stat* buf ); -#endif extern Int VG_(fstat) ( Int fd, struct vg_stat* buf ); extern SysRes VG_(dup) ( Int oldfd ); extern SysRes VG_(dup2) ( Int oldfd, Int newfd ); diff --git a/memcheck/tests/x86-linux/scalar_openat2.stderr.exp b/memcheck/tests/x86-linux/scalar_openat2.stderr.exp index 367da9dbf2..c666f9fb48 100644 --- a/memcheck/tests/x86-linux/scalar_openat2.stderr.exp +++ b/memcheck/tests/x86-linux/scalar_openat2.stderr.exp @@ -1,11 +1,11 @@ ----------------------------------------------------- 437: __NR_openat2 4s 2m ----------------------------------------------------- -Syscall param openat2(dfd) contains uninitialised byte(s) +Syscall param openat2(dirfd) contains uninitialised byte(s) ... by 0x........: main (scalar_openat2.c:15) -Syscall param openat2(filename) contains uninitialised byte(s) +Syscall param openat2(pathname) contains uninitialised byte(s) ... by 0x........: main (scalar_openat2.c:15) @@ -17,7 +17,7 @@ Syscall param openat2(size) contains uninitialised byte(s) ... by 0x........: main (scalar_openat2.c:15) -Syscall param openat2(filename) points to unaddressable byte(s) +Syscall param openat2(pathname) points to unaddressable byte(s) ... by 0x........: main (scalar_openat2.c:15) Address 0x........ is not stack'd, malloc'd or (recently) free'd diff --git a/none/tests/freebsd/Makefile.am b/none/tests/freebsd/Makefile.am index 5d209c6d27..0bbbffdad6 100644 --- a/none/tests/freebsd/Makefile.am +++ b/none/tests/freebsd/Makefile.am @@ -53,6 +53,8 @@ EXTRA_DIST = \ ksh_test.ksh \ ksh_test.stderr.exp \ ksh_test.stdout.exp \ + open_client.vgtest \ + open_client.stderr.exp \ sanity_level_thread.vgtest \ sanity_level_thread.stderr.exp \ swapcontext.vgtest \ @@ -69,7 +71,7 @@ EXTRA_DIST = \ usrstack.stdout.exp check_PROGRAMS = \ - auxv bug452274 bug498317 bug499212 fexecve hello_world osrel \ + auxv bug452274 bug498317 bug499212 fexecve hello_world open_client osrel \ proc_pid_file sanity_level_thread swapcontext umtx_shm_creat usrstack AM_CFLAGS += $(AM_FLAG_M3264_PRI) @@ -80,7 +82,7 @@ osrel_CFLAGS = ${AM_CFLAGS} swapcontext_CFLAGS = ${AM_CFLAGS} hello_world_SOURCES = hello_world.cpp - +open_client_SOURCES = open_client.cpp proc_pid_file_SOURCES = proc_pid_file.cpp sanity_level_thread_SOURCES = sanity_level_thread.cpp diff --git a/none/tests/freebsd/open_client.cpp b/none/tests/freebsd/open_client.cpp new file mode 100644 index 0000000000..454c0a3dc7 --- /dev/null +++ b/none/tests/freebsd/open_client.cpp @@ -0,0 +1,108 @@ +// For Bug 505673 +// Valgrind crashes with an internal error and SIGBUS when the guest tries to open its own file with O_WRONLY|O_CREAT|O_TRUNC +#include <fcntl.h> +#include <cerrno> +#include <stdexcept> +#include <vector> +#include <unistd.h> +#include <sys/syscall.h> +#include <unistd.h> + +int main(int argc, char** argv) +{ + std::vector<int> flags{O_WRONLY|O_CREAT|O_TRUNC, O_WRONLY, O_RDWR}; + + // On FreeBSD libc open uses syscall openat (at least on 14.2) + for (auto f : flags) + { + int res = open(argv[0], f, 0666); + if (-1 != res) + { + throw std::runtime_error("open should have failed"); + } + else + { + if (errno != ETXTBSY) + { + throw std::runtime_error("errno should be ETXTBSY"); + } + } + } + + // repeat the above, but with syscall SYS_open + for (auto f : flags) + { + int res = syscall(SYS_open, argv[0], f, 0666); + if (-1 != res) + { + throw std::runtime_error("open should have failed"); + } + else + { + if (errno != ETXTBSY) + { + throw std::runtime_error("errno should be ETXTBSY"); + } + } + } + + int dotdot; + if ((dotdot = open("..", O_DIRECTORY | O_RDONLY)) == -1) + { + throw std::runtime_error("failed to open ,."); + } + else + { + for (auto f : flags) + { + int res = openat(dotdot, "freebsd/open_client", f, 0666); + if (-1 != res) + { + throw std::runtime_error("open should have failed"); + } + else + { + if (errno != ETXTBSY) + { + throw std::runtime_error("errno should be ETXTBSY"); + } + } + } + } + close(dotdot); + + chdir(".."); + + // check that relative paths work + for (auto f : flags) + { + int res = open("freebsd/open_client", f, 0666); + if (-1 != res) + { + throw std::runtime_error("open should have failed"); + } + else + { + if (errno != ETXTBSY) + { + throw std::runtime_error("errno should be ETXTBSY"); + } + } + } + + for (auto f : flags) + { + int res = syscall(SYS_open, "freebsd/open_client", f, 0666); + if (-1 != res) + { + throw std::runtime_error("open should have failed"); + } + else + { + if (errno != ETXTBSY) + { + throw std::runtime_error("errno should be ETXTBSY"); + } + } + } +} diff --git a/none/tests/freebsd/open_client.stderr.exp b/none/tests/freebsd/open_client.stderr.exp new file mode 100644 index 0000000000..e69de29bb2 diff --git a/none/tests/freebsd/open_client.vgtest b/none/tests/freebsd/open_client.vgtest new file mode 100644 index 0000000000..12b148de4e --- /dev/null +++ b/none/tests/freebsd/open_client.vgtest @@ -0,0 +1,3 @@ +prog: open_client +vgopts: -q + diff --git a/none/tests/linux/Makefile.am b/none/tests/linux/Makefile.am index dbb535902e..c81ffff548 100644 --- a/none/tests/linux/Makefile.am +++ b/none/tests/linux/Makefile.am @@ -17,6 +17,7 @@ EXTRA_DIST = \ mremap4.stderr.exp mremap4.vgtest \ mremap5.stderr.exp mremap5.vgtest \ mremap6.stderr.exp mremap6.vgtest \ + open_client.stderr.exp open_client.vgtest \ pthread-stack.stderr.exp pthread-stack.vgtest \ stack-overflow.stderr.exp stack-overflow.vgtest @@ -32,6 +33,7 @@ check_PROGRAMS = \ mremap4 \ mremap5 \ mremap6 \ + open_client \ pthread-stack \ stack-overflow @@ -45,6 +47,7 @@ AM_CXXFLAGS += $(AM_FLAG_M3264_PRI) # Special needs clonev_LDADD = -lpthread +open_client_SOURCES = open_client.cpp pthread_stack_LDADD = -lpthread stack_overflow_CFLAGS = $(AM_CFLAGS) @FLAG_W_NO_UNINITIALIZED@ \ diff --git a/none/tests/linux/open_client.cpp b/none/tests/linux/open_client.cpp new file mode 100644 index 0000000000..4b336e7f02 --- /dev/null +++ b/none/tests/linux/open_client.cpp @@ -0,0 +1,191 @@ +// For Bug 505673 +// Valgrind crashes with an internal error and SIGBUS when the guest tries to open its own file with O_WRONLY|O_CREAT|O_TRUNC +#include <fcntl.h> +#include <cerrno> +#include <stdexcept> +#include <string> +#include <vector> +#include <unistd.h> +#include <sys/syscall.h> +#include <unistd.h> +#include <linux/openat2.h> + +int main(int argc, char** argv) +{ + std::vector<__u64> flags{O_WRONLY|O_CREAT|O_TRUNC, O_WRONLY, O_RDWR}; + auto pid = getpid(); + auto ppe = std::string("/proc/") + std::to_string(pid) + "/exe"; + std::vector<std::string> names{argv[0], "/proc/self/exe", ppe}; + int dotdot; + + for (const auto& n : names) + { + for (auto f : flags) + { + int res = open(n.c_str(), f, 0666); + if (-1 != res) + { + throw std::runtime_error("open should have failed"); + } + else + { + if (errno != ETXTBSY) + { + throw std::runtime_error("errno should be ETXTBSY"); + } + } + } + + for (auto f : flags) + { + int res = syscall(SYS_open, n.c_str(), f, 0666); + + if (-1 != res) + { + throw std::runtime_error("open should have failed"); + } + else + { + if (errno != ETXTBSY) + { + throw std::runtime_error("errno should be ETXTBSY"); + } + } + } + } + + if ((dotdot = open("..", O_DIRECTORY | O_RDONLY)) == -1) + { + throw std::runtime_error("failed to open ,."); + } + else + { + for (auto f : flags) + { + int res = openat(dotdot, "linux/open_client", f, 0666); + if (-1 != res) + { + throw std::runtime_error("open should have failed"); + } + else + { + if (errno != ETXTBSY) + { + throw std::runtime_error("errno should be ETXTBSY"); + } + } + } + } + +#if defined(SYS_openat2) + for (const auto& n : names) + { + for (auto f : flags) + { + struct open_how oh = { .flags=f, .mode=((f&static_cast<__u64>(O_CREAT))?0666UL:0UL), .resolve=0 }; + int res = syscall(SYS_openat2, AT_FDCWD, n.c_str(), &oh, sizeof(oh)); + if (-1 != res) + { + throw std::runtime_error("openat2 should have failed"); + } + else + { + if (errno != ETXTBSY) + { + throw std::runtime_error("errno should be ETXTBSY"); + } + } + } + } + + for (auto f : flags) + { + struct open_how oh = { .flags=f, .mode=((f&static_cast<__u64>(O_CREAT))?0666UL:0UL), .resolve=0 }; + int res = syscall(SYS_openat2, dotdot, "linux/open_client", &oh, sizeof(oh)); + if (-1 != res) + { + throw std::runtime_error("openat2 should have failed"); + } + else + { + if (errno != ETXTBSY) + { + throw std::runtime_error("errno should be ETXTBSY"); + } + } + } + + for (auto f : flags) + { + struct open_how oh = { .flags=f, .mode=((f&static_cast<__u64>(O_CREAT))?0666UL:0UL), .resolve=RESOLVE_IN_ROOT }; + int res = syscall(SYS_openat2, dotdot, "/linux/open_client", &oh, sizeof(oh)); + if (-1 != res) + { + throw std::runtime_error("openat2 should have failed"); + } + else + { + if (errno != ETXTBSY) + { + throw std::runtime_error("errno should be ETXTBSY"); + } + } + } +#endif + + close(dotdot); + + chdir(".."); + + // check that relative paths work + for (auto f : flags) + { + int res = open("linux/open_client", f, 0666); + if (-1 != res) + { + throw std::runtime_error("open should have failed"); + } + else + { + if (errno != ETXTBSY) + { + throw std::runtime_error("errno should be ETXTBSY"); + } + } + } + + for (auto f : flags) + { + int res = syscall(SYS_open, "linux/open_client", f, 0666); + if (-1 != res) + { + throw std::runtime_error("open should have failed"); + } + else + { + if (errno != ETXTBSY) + { + throw std::runtime_error("errno should be ETXTBSY"); + } + } + } + +#if defined(SYS_openat2) + for (auto f : flags) + { + struct open_how oh = { .flags=f, .mode=((f&static_cast<__u64>(O_CREAT))?0666UL:0UL), .resolve=0 }; + int res = syscall(SYS_openat2, AT_FDCWD, "linux/open_client", &oh, sizeof(oh)); + if (-1 != res) + { + throw std::runtime_error("openat2 should have failed"); + } + else + { + if (errno != ETXTBSY) + { + throw std::runtime_error("errno should be ETXTBSY"); + } + } + } +#endif +} diff --git a/none/tests/linux/open_client.stderr.exp b/none/tests/linux/open_client.stderr.exp new file mode 100644 index 0000000000..e69de29bb2 diff --git a/none/tests/linux/open_client.vgtest b/none/tests/linux/open_client.vgtest new file mode 100644 index 0000000000..6b930efe6c --- /dev/null +++ b/none/tests/linux/open_client.vgtest @@ -0,0 +1,2 @@ +prog: open_client +vgopts: -q |
