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

diff --git a/.gitignore b/.gitignore
index 80924f997cc117ba91864c7d8928df1effa66b5b..4eb477c0890f94f8fd0c3444fc6537358bb0efe5 100644 (file)
--- 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 1a3bbe214360a0a2eae5f42cfe481945b811ddf9..6679e58fb5047685c204095eedc7e5b7e4c7f6b8 100644 (file)
--- 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/<pid>/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/<pid>/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 5c46d27d36e4c4b1523adc8d4c6277971de9fb3c..66ef0c47ca414b15ae2c06a164ba7f9745a7091a 100644 (file)
--- 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 (file)
index 0000000..0aef71d
--- /dev/null
+++ b/none/tests/freebsd/readlinkat2.cpp
@@ -0,0 +1,57 @@
+#include <unistd.h>
+#include <fcntl.h>
+#include <sys/types.h>
+#include <sys/param.h>
+#include <iostream>
+#include <cstring>
+#include <cstdlib>
+
+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 (file)
index 0000000..e69de29

diff --git a/none/tests/freebsd/readlinkat2.vgtest b/none/tests/freebsd/readlinkat2.vgtest
new file mode 100644 (file)
index 0000000..1decb1e
--- /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