From 66ab1212649a9e4f2a3bdce187ef8cfc44da7b10 Mon Sep 17 00:00:00 2001 From: Paul Floyd Date: Sat, 30 Aug 2025 22:05:12 +0200 Subject: [PATCH] FreeBSD realpathat syscall: fix length of written memory and add a testcase There were no tests for /proc/currrproc/file and the length written in post was out by one --- .gitignore | 1 + coregrind/m_syswrap/syswrap-freebsd.c | 52 +++++++-------------- none/tests/freebsd/Makefile.am | 6 ++- none/tests/freebsd/readlinkat2.cpp | 57 +++++++++++++++++++++++ none/tests/freebsd/readlinkat2.stderr.exp | 0 none/tests/freebsd/readlinkat2.vgtest | 4 ++ 6 files changed, 85 insertions(+), 35 deletions(-) create mode 100644 none/tests/freebsd/readlinkat2.cpp create mode 100644 none/tests/freebsd/readlinkat2.stderr.exp create mode 100644 none/tests/freebsd/readlinkat2.vgtest diff --git a/.gitignore b/.gitignore index 80924f997c..4eb477c089 100644 --- a/.gitignore +++ b/.gitignore @@ -2331,6 +2331,7 @@ none/tests/freebsd/bug499212 /none/tests/freebsd/osrel /none/tests/freebsd/readlinkat +/none/tests/freebsd/readlinkat2 /none/tests/freebsd/swapcontext /none/tests/freebsd/fexecve /none/tests/freebsd/hello_world diff --git a/coregrind/m_syswrap/syswrap-freebsd.c b/coregrind/m_syswrap/syswrap-freebsd.c index 1a3bbe2143..6679e58fb5 100644 --- a/coregrind/m_syswrap/syswrap-freebsd.c +++ b/coregrind/m_syswrap/syswrap-freebsd.c @@ -1183,7 +1183,8 @@ PRE(sys_revoke) // SYS_symlink 57 // generic -static void do_readlink(const HChar* path, HChar *buf, SizeT bufsize, SyscallStatus* status, Bool* curproc_file) +// returns whether caller needs to set SfMayBlock in flags +static Bool do_readlink(const HChar* path, HChar *buf, SizeT bufsize, SyscallStatus* status) { HChar name[30]; VG_(sprintf)(name, "/proc/%d/file", VG_(getpid)()); @@ -1191,43 +1192,32 @@ static void do_readlink(const HChar* path, HChar *buf, SizeT bufsize, SyscallSta && (VG_(strcmp)(path, name) == 0 || VG_(strcmp)(path, "/proc/curproc/file") == 0)) { vg_assert(VG_(resolved_exename)); - Int len = VG_(snprintf)(buf, bufsize, "%s", VG_(resolved_exename)); + Int len = VG_(snprintf)(buf, bufsize, "%s", VG_(resolved_exename)) + 1; SET_STATUS_Success(len); - *curproc_file = True; + return False; } + return True; } // SYS_readlink 58 // ssize_t readlink(const char *restrict path, char *restrict buf, size_t bufsiz); PRE(sys_readlink) { - FUSE_COMPATIBLE_MAY_BLOCK(); - Word saved = SYSNO; - Bool curproc_file = False; - PRINT("sys_readlink ( %#" FMT_REGWORD "x(%s), %#" FMT_REGWORD "x, %llu )", ARG1, (char*)(Addr)ARG1, ARG2, (ULong)ARG3); PRE_REG_READ3(long, "readlink", const char *, path, char *, buf, int, bufsiz); - PRE_MEM_RASCIIZ( "readlink(path)", ARG1 ); - PRE_MEM_WRITE( "readlink(buf)", ARG2,ARG3 ); + PRE_MEM_RASCIIZ("readlink(path)", ARG1); + PRE_MEM_WRITE("readlink(buf)", ARG2, ARG3); - if (VG_(have_slash_proc) == True) - { - /* - * Handle the case where readlink is looking at /proc/curproc/file or - * /proc//file - */ - do_readlink((const HChar *)ARG1, (HChar *)ARG2, (SizeT)ARG3, status, &curproc_file); + if (VG_(have_slash_proc) == False || do_readlink((const HChar *)ARG1, (HChar *)ARG2, (SizeT)ARG3, status)) { + *flags |= SfMayBlock; } +} - if (!curproc_file) { - /* Normal case */ - SET_STATUS_from_SysRes( VG_(do_syscall3)(saved, ARG1, ARG2, ARG3)); - } - if (SUCCESS && RES > 0) { - POST_MEM_WRITE( ARG2, RES ); - } +POST(sys_readlink) +{ + POST_MEM_WRITE(ARG2, RES); } // SYS_execve 59 @@ -5447,29 +5437,23 @@ POST(sys_openat) } } -// @todo PJF make this generic? // SYS_readlinkat 500 // ssize_t readlinkat(int fd, const char *restrict path, char *restrict buf, // size_t bufsize); PRE(sys_readlinkat) { - Bool curproc_file = False; - - PRINT("sys_readlinkat ( %" FMT_REGWORD "u, %#" FMT_REGWORD "x(%s), %#" FMT_REGWORD "x, %llu )", ARG1,ARG2,(char*)ARG2,ARG3,(ULong)ARG4); + PRINT("sys_readlinkat ( %" FMT_REGWORD "u, %#" FMT_REGWORD "x(%s), %#" FMT_REGWORD "x, %llu )", + ARG1, ARG2, (char*)ARG2, ARG3, (ULong)ARG4); PRE_REG_READ4(ssize_t, "readlinkat", int, fd, const char *, path, char *, buf, int, bufsize); ML_(fd_at_check_allowed)(SARG1, (const HChar*)ARG2, "readlinkat", tid, status); PRE_MEM_RASCIIZ( "readlinkat(path)", ARG2 ); PRE_MEM_WRITE("readlinkat(buf)", ARG3, ARG4); - if (VG_(have_slash_proc) == True) { - /* - * Handle the case where readlinkat is looking at /proc/curproc/file or - * /proc//file. - */ + if (VG_(have_slash_proc) == False || do_readlink((const HChar *)ARG2, (HChar *)ARG3, (SizeT)ARG4, status)) { // @todo PJF there is still the case where fd refers to / or /proc or /proc/pid // or /proc/curproc and path is relative pid/file, curproc/file or just file - do_readlink((const HChar *)ARG2, (HChar *)ARG3, (SizeT)ARG4, status, &curproc_file); + *flags |= SfMayBlock; } } @@ -7294,7 +7278,7 @@ const SyscallTableEntry ML_(syscall_table)[] = { BSDX_(__NR_revoke, sys_revoke), // 56 GENX_(__NR_symlink, sys_symlink), // 57 - BSDX_(__NR_readlink, sys_readlink), // 58 + BSDXY(__NR_readlink, sys_readlink), // 58 GENX_(__NR_execve, sys_execve), // 59 GENX_(__NR_umask, sys_umask), // 60 diff --git a/none/tests/freebsd/Makefile.am b/none/tests/freebsd/Makefile.am index 5c46d27d36..66ef0c47ca 100644 --- a/none/tests/freebsd/Makefile.am +++ b/none/tests/freebsd/Makefile.am @@ -57,6 +57,8 @@ EXTRA_DIST = \ open_client.stderr.exp \ readlinkat.vgtest \ readlinkat.stderr.exp \ + readlinkat2.vgtest \ + readlinkat2.stderr.exp \ sanity_level_thread.vgtest \ sanity_level_thread.stderr.exp \ swapcontext.vgtest \ @@ -74,7 +76,7 @@ EXTRA_DIST = \ check_PROGRAMS = \ auxv bug452274 bug498317 bug499212 fexecve hello_world open_client \ - osrel proc_pid_file readlinkat sanity_level_thread swapcontext \ + osrel proc_pid_file readlinkat readlinkat2 sanity_level_thread swapcontext \ umtx_shm_creat usrstack AM_CFLAGS += $(AM_FLAG_M3264_PRI) @@ -89,6 +91,8 @@ open_client_SOURCES = open_client.cpp proc_pid_file_SOURCES = proc_pid_file.cpp readlinkat_SOURCES = readlinkat.cpp readlinkat_CXXFLAGS = ${AM_CXXFLAGS} @FLAG_W_NO_UNINITIALIZED@ +readlinkat2_SOURCES = readlinkat2.cpp +readlinkat2_CXXFLAGS = ${AM_CXXFLAGS} @FLAG_W_NO_UNINITIALIZED@ sanity_level_thread_SOURCES = sanity_level_thread.cpp sanity_level_thread_LDFLAGS = ${AM_LDFLAGS} -pthread diff --git a/none/tests/freebsd/readlinkat2.cpp b/none/tests/freebsd/readlinkat2.cpp new file mode 100644 index 0000000000..0aef71d7f7 --- /dev/null +++ b/none/tests/freebsd/readlinkat2.cpp @@ -0,0 +1,57 @@ +#include +#include +#include +#include +#include +#include +#include + +int main(int argc, char** argv) +{ + char linkedPath[MAXPATHLEN]; + char selfAbsolutePath[MAXPATHLEN]; + auto pid{getpid()}; + std::string pidString{std::to_string(pid)}; + std::string procPidFile{std::string("/proc/") + pidString + "/file"}; + realpath(argv[0], selfAbsolutePath); + std::string selfAbsolutePathString(selfAbsolutePath); + + ssize_t res = readlinkat(AT_FDCWD, "/proc/curproc/file", linkedPath, MAXPATHLEN); + if (res == -1) + { + std::cerr << "Error: readlinkat test 1 failed\n"; + } + else + { + if (selfAbsolutePathString != linkedPath) + { + std::cerr << "Error: readlinkat test 1 unexpected resolved path - " << linkedPath << '\n'; + } + } + + res = readlinkat(AT_FDCWD, procPidFile.c_str(), linkedPath, MAXPATHLEN); + if (res == -1) + { + std::cerr << "Error: readlinkat test 2 failed\n"; + } + else + { + if (selfAbsolutePathString != linkedPath) + { + std::cerr << "Error: readlinkat test 2 unexpected resolved path - " << linkedPath << '\n'; + } + } + + // @todo PJF do some tests with cwd as /proc /proc/PID and /proc/curproc + // and a rlative path to 'file' + // not yet implemented in Valgrind + chdir("/proc"); + + // @todo PJF do some tests as above but with fd as /proc /proc/PID and /proc/curproc + int slash; + if ((slash = open("/", O_DIRECTORY | O_RDONLY)) == -1) + { + throw std::runtime_error("failed to open /"); + } + close(slash); +} diff --git a/none/tests/freebsd/readlinkat2.stderr.exp b/none/tests/freebsd/readlinkat2.stderr.exp new file mode 100644 index 0000000000..e69de29bb2 diff --git a/none/tests/freebsd/readlinkat2.vgtest b/none/tests/freebsd/readlinkat2.vgtest new file mode 100644 index 0000000000..1decb1ee0b --- /dev/null +++ b/none/tests/freebsd/readlinkat2.vgtest @@ -0,0 +1,4 @@ +# FreeBSD doesn't always have a /proc filesystem +prereq: test -d /proc +prog: readlinkat2 +vgopts: -q -- 2.47.3