From b667130795267c0afb784a9bad71a5a74e48fde0 Mon Sep 17 00:00:00 2001 From: Nicolas Lacasse Date: Tue, 3 Oct 2023 14:59:20 -0700 Subject: [PATCH] Clean up and re-enable process_vm_readv/writev Some fixes: * First argument of Task.CopyContext should always be the context.Context derived from the currently running task, because it is used to get a CopyScratchBuffer, which must be from the current task. This solved a bunch of data races. * Fix logic around which process is remote and which is local. These were getting mixed up. * Always read iovec structs (local and remote) from the local process's address space, since they are syscall arguments. Only use the remote process address space to read the memory pointed to by the remote iovecs. * Added ptrace permissions check, per linux. * Delete unused code from kernel/task_usermem.go * Rewrote tests so that we read to (write from) a subprocess, rather than the other way around. So we don't need CAP_PTRACE to run the tests. * Also make tests async-signal-safe after call to fork(). I think this was the source of the flakyness on linux previously. PiperOrigin-RevId: 570506366 --- pkg/sentry/kernel/task_usermem.go | 54 +-- pkg/sentry/strace/strace.go | 2 +- pkg/sentry/syscalls/linux/linux64.go | 8 +- pkg/sentry/syscalls/linux/sys_process_vm.go | 240 ++++++----- test/syscalls/BUILD | 6 +- test/syscalls/linux/BUILD | 5 +- test/syscalls/linux/process_vm_read_write.cc | 419 ++++++++++--------- 7 files changed, 375 insertions(+), 359 deletions(-) diff --git a/pkg/sentry/kernel/task_usermem.go b/pkg/sentry/kernel/task_usermem.go index 7309196cd..cc36aa9dc 100644 --- a/pkg/sentry/kernel/task_usermem.go +++ b/pkg/sentry/kernel/task_usermem.go @@ -131,24 +131,18 @@ func (t *Task) CopyInVector(addr hostarch.Addr, maxElemSize, maxTotalSize int) ( // - The caller must be running on the task goroutine. // - t's AddressSpace must be active. func (t *Task) CopyOutIovecs(addr hostarch.Addr, src hostarch.AddrRangeSeq) error { - return copyOutIovecs(t, t, addr, src) -} - -// copyOutIovecs converts src to an array of struct iovecs and copies it to the -// memory mapped at addr. -func copyOutIovecs(ctx marshal.CopyContext, t *Task, addr hostarch.Addr, src hostarch.AddrRangeSeq) error { switch t.Arch().Width() { case 8: if _, ok := addr.AddLength(uint64(src.NumRanges()) * iovecLength); !ok { return linuxerr.EFAULT } - b := ctx.CopyScratchBuffer(iovecLength) + b := t.CopyScratchBuffer(iovecLength) for ; !src.IsEmpty(); src = src.Tail() { ar := src.Head() hostarch.ByteOrder.PutUint64(b[0:8], uint64(ar.Start)) hostarch.ByteOrder.PutUint64(b[8:16], uint64(ar.Length())) - if _, err := ctx.CopyOutBytes(addr, b); err != nil { + if _, err := t.CopyOutBytes(addr, b); err != nil { return err } addr += iovecLength @@ -178,6 +172,15 @@ func (t *Task) CopyInIovecs(addr hostarch.Addr, numIovecs int) (hostarch.AddrRan return hostarch.AddrRangeSeqFromSlice(iovecs), nil } +// CopyInIovecsAsSlice copies in IoVecs and returns them in a slice. +// +// Preconditions: Same as usermem.IO.CopyIn, plus: +// - The caller must be running on the task goroutine or hold t.mu. +// - t's AddressSpace must be active. +func (t *Task) CopyInIovecsAsSlice(addr hostarch.Addr, numIovecs int) ([]hostarch.AddrRange, error) { + return copyInIovecs(t, t, addr, numIovecs) +} + func copyInIovec(ctx marshal.CopyContext, t *Task, addr hostarch.Addr) (hostarch.AddrRangeSeq, error) { if err := checkArch(t); err != nil { return hostarch.AddrRangeSeq{}, err @@ -322,10 +325,9 @@ func (t *Task) IovecsIOSequence(addr hostarch.Addr, iovcnt int, opts usermem.IOO } type taskCopyContext struct { - ctx context.Context - t *Task - opts usermem.IOOpts - allocateNewBuffers bool + ctx context.Context + t *Task + opts usermem.IOOpts } // CopyContext returns a marshal.CopyContext that copies to/from t's address @@ -340,7 +342,7 @@ func (t *Task) CopyContext(ctx context.Context, opts usermem.IOOpts) *taskCopyCo // CopyScratchBuffer implements marshal.CopyContext.CopyScratchBuffer. func (cc *taskCopyContext) CopyScratchBuffer(size int) []byte { - if ctxTask, ok := cc.ctx.(*Task); ok && !cc.allocateNewBuffers { + if ctxTask, ok := cc.ctx.(*Task); ok { return ctxTask.CopyScratchBuffer(size) } return make([]byte, size) @@ -357,13 +359,6 @@ func (cc *taskCopyContext) getMemoryManager() (*mm.MemoryManager, error) { return tmm, nil } -// WithTaskMutexLocked runs the given function with the task's mutex locked. -func (cc *taskCopyContext) WithTaskMutexLocked(fn func() error) error { - cc.t.mu.Lock() - defer cc.t.mu.Unlock() - return fn() -} - // CopyInBytes implements marshal.CopyContext.CopyInBytes. // // Preconditions: Same as usermem.IO.CopyIn, plus: @@ -392,25 +387,6 @@ func (cc *taskCopyContext) CopyOutBytes(addr hostarch.Addr, src []byte) (int, er return tmm.CopyOut(cc.ctx, addr, src, cc.opts) } -// CopyOutIovecs converts src to an array of struct iovecs and copies it to the -// memory mapped at addr for Task. -// -// Preconditions: Same as usermem.IO.CopyOut, plus: -// - The caller must be running on the task goroutine or hold the cc.t.mu -// - t's AddressSpace must be active. -func (cc *taskCopyContext) CopyOutIovecs(addr hostarch.Addr, src hostarch.AddrRangeSeq) error { - return copyOutIovecs(cc, cc.t, addr, src) -} - -// CopyInIovecs copies in IoVecs for taskCopyContext. -// -// Preconditions: Same as usermem.IO.CopyIn, plus: -// - The caller must be running on the task goroutine or hold the cc.t.mu -// - t's AddressSpace must be active. -func (cc *taskCopyContext) CopyInIovecs(addr hostarch.Addr, numIovecs int) ([]hostarch.AddrRange, error) { - return copyInIovecs(cc, cc.t, addr, numIovecs) -} - type ownTaskCopyContext struct { t *Task opts usermem.IOOpts diff --git a/pkg/sentry/strace/strace.go b/pkg/sentry/strace/strace.go index e43287a26..c6b4d13a1 100644 --- a/pkg/sentry/strace/strace.go +++ b/pkg/sentry/strace/strace.go @@ -392,7 +392,7 @@ func (i *SyscallInfo) pre(t *kernel.Task, args arch.SyscallArguments, maximumBlo output = append(output, dump(t, args[arg].Pointer(), args[arg+1].SizeT(), maximumBlobSize, LogAppDataAllowed /* content */)) case WriteIOVec: output = append(output, iovecs(t, args[arg].Pointer(), int(args[arg+1].Int()), LogAppDataAllowed /* content */, uint64(maximumBlobSize))) - case IOVec: + case ReadIOVec, IOVec: output = append(output, iovecs(t, args[arg].Pointer(), int(args[arg+1].Int()), false /* content */, uint64(maximumBlobSize))) case SendMsgHdr: output = append(output, msghdr(t, args[arg].Pointer(), LogAppDataAllowed /* content */, uint64(maximumBlobSize))) diff --git a/pkg/sentry/syscalls/linux/linux64.go b/pkg/sentry/syscalls/linux/linux64.go index 327a4d5e3..e95411f56 100644 --- a/pkg/sentry/syscalls/linux/linux64.go +++ b/pkg/sentry/syscalls/linux/linux64.go @@ -362,8 +362,8 @@ var AMD64 = &kernel.SyscallTable{ 307: syscalls.Supported("sendmmsg", SendMMsg), 308: syscalls.Supported("setns", Setns), 309: syscalls.Supported("getcpu", Getcpu), - 310: syscalls.ErrorWithEvent("process_vm_readv", linuxerr.ENOSYS, "", []string{"gvisor.dev/issue/158"}), // TODO(b/260724654) - 311: syscalls.ErrorWithEvent("process_vm_writev", linuxerr.ENOSYS, "", []string{"gvisor.dev/issue/158"}), // TODO(b/260724654) + 310: syscalls.Supported("process_vm_readv", ProcessVMReadv), + 311: syscalls.Supported("process_vm_writev", ProcessVMWritev), 312: syscalls.CapError("kcmp", linux.CAP_SYS_PTRACE, "", nil), 313: syscalls.CapError("finit_module", linux.CAP_SYS_MODULE, "", nil), 314: syscalls.ErrorWithEvent("sched_setattr", linuxerr.ENOSYS, "gVisor does not implement a scheduler.", []string{"gvisor.dev/issue/264"}), // TODO(b/118902272) @@ -685,8 +685,8 @@ var ARM64 = &kernel.SyscallTable{ 267: syscalls.Supported("syncfs", Syncfs), 268: syscalls.Supported("setns", Setns), 269: syscalls.Supported("sendmmsg", SendMMsg), - 270: syscalls.ErrorWithEvent("process_vm_readv", linuxerr.ENOSYS, "", []string{"gvisor.dev/issue/158"}), // TODO(b/260724654) - 271: syscalls.ErrorWithEvent("process_vm_writev", linuxerr.ENOSYS, "", []string{"gvisor.dev/issue/158"}), // TODO(b/260724654) + 270: syscalls.Supported("process_vm_readv", ProcessVMReadv), + 271: syscalls.Supported("process_vm_writev", ProcessVMWritev), 272: syscalls.CapError("kcmp", linux.CAP_SYS_PTRACE, "", nil), 273: syscalls.CapError("finit_module", linux.CAP_SYS_MODULE, "", nil), 274: syscalls.ErrorWithEvent("sched_setattr", linuxerr.ENOSYS, "gVisor does not implement a scheduler.", []string{"gvisor.dev/issue/264"}), // TODO(b/118902272) diff --git a/pkg/sentry/syscalls/linux/sys_process_vm.go b/pkg/sentry/syscalls/linux/sys_process_vm.go index ff4ce0e71..afff8532d 100644 --- a/pkg/sentry/syscalls/linux/sys_process_vm.go +++ b/pkg/sentry/syscalls/linux/sys_process_vm.go @@ -15,33 +15,35 @@ package linux import ( + "fmt" + "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/errors/linuxerr" "gvisor.dev/gvisor/pkg/hostarch" + "gvisor.dev/gvisor/pkg/marshal" "gvisor.dev/gvisor/pkg/sentry/arch" "gvisor.dev/gvisor/pkg/sentry/kernel" "gvisor.dev/gvisor/pkg/usermem" ) -type vmReadWriteOp int +type processVMOpType int const ( - localReadLocalWrite vmReadWriteOp = iota - remoteReadLocalWrite - localReadRemoteWrite + processVMOpRead = iota + processVMOpWrite ) // ProcessVMReadv implements process_vm_readv(2). func ProcessVMReadv(t *kernel.Task, sysno uintptr, args arch.SyscallArguments) (uintptr, *kernel.SyscallControl, error) { - return processVMRW(t, args, false /*isWrite*/) + return processVMOp(t, args, processVMOpRead) } // ProcessVMWritev implements process_vm_writev(2). func ProcessVMWritev(t *kernel.Task, sysno uintptr, args arch.SyscallArguments) (uintptr, *kernel.SyscallControl, error) { - return processVMRW(t, args, true /*isWrite*/) + return processVMOp(t, args, processVMOpWrite) } -func processVMRW(t *kernel.Task, args arch.SyscallArguments, isWrite bool) (uintptr, *kernel.SyscallControl, error) { +func processVMOp(t *kernel.Task, args arch.SyscallArguments, op processVMOpType) (uintptr, *kernel.SyscallControl, error) { pid := kernel.ThreadID(args[0].Int()) lvec := hostarch.Addr(args[1].Pointer()) liovcnt := int(args[2].Int64()) @@ -49,6 +51,7 @@ func processVMRW(t *kernel.Task, args arch.SyscallArguments, isWrite bool) (uint riovcnt := int(args[4].Int64()) flags := args[5].Int() + // Parse the flags. switch { case flags != 0 || liovcnt < 0 || @@ -56,132 +59,159 @@ func processVMRW(t *kernel.Task, args arch.SyscallArguments, isWrite bool) (uint liovcnt > linux.UIO_MAXIOV || riovcnt > linux.UIO_MAXIOV: return 0, nil, linuxerr.EINVAL - case lvec == 0 || rvec == 0: - return 0, nil, linuxerr.EFAULT case liovcnt == 0 || riovcnt == 0: return 0, nil, nil + case lvec == 0 || rvec == 0: + return 0, nil, linuxerr.EFAULT } - localProcess := t.ThreadGroup().Leader() - if localProcess == nil { + // Determine local and remote processes. + // Local process is always the caller. + localTask := t.ThreadGroup().Leader() + if localTask == nil { return 0, nil, linuxerr.ESRCH } - remoteThreadGroup := localProcess.PIDNamespace().ThreadGroupWithID(pid) + // Remote process is the pid specified in the syscall arguments. It is + // allowed to be the same as the caller process. + remoteThreadGroup := localTask.PIDNamespace().ThreadGroupWithID(pid) if remoteThreadGroup == nil { return 0, nil, linuxerr.ESRCH } - remoteProcess := remoteThreadGroup.Leader() + remoteTask := remoteThreadGroup.Leader() + if remoteTask.ExitState() >= kernel.TaskExitInitiated { + return 0, nil, linuxerr.ESRCH + } - isRemote := localProcess == remoteProcess + // man 2 process_vm_read: "Permission to read from or write to another + // process is governed by a ptrace access mode + // PTRACE_MODE_ATTACH_REALCREDS check; see ptrace(2)." + if !localTask.CanTrace(remoteTask, true /* attach */) { + return 0, nil, linuxerr.EPERM + } - // For the write case, we read from the local process and write to the remote process. - op := localReadLocalWrite - if isWrite { - if isRemote { - op = remoteReadLocalWrite + // Figure out which processes and arguments (local or remote) are for + // writing and which are for reading, based on the operation. + var opArgs processVMOpArgs + switch op { + case processVMOpRead: + // Read from remote process and write into local. + opArgs = processVMOpArgs{ + readCtx: remoteTask.CopyContext(t, usermem.IOOpts{}), + readAddr: rvec, + readIovecCount: riovcnt, + writeCtx: localTask.CopyContext(t, usermem.IOOpts{AddressSpaceActive: true}), + writeAddr: lvec, + writeIovecCount: liovcnt, } - return doProcessVMReadWrite(localProcess, remoteProcess, lvec, rvec, liovcnt, riovcnt, op) + case processVMOpWrite: + // Read from local process and write into remote. + opArgs = processVMOpArgs{ + readCtx: localTask.CopyContext(t, usermem.IOOpts{AddressSpaceActive: true}), + readAddr: lvec, + readIovecCount: liovcnt, + writeCtx: remoteTask.CopyContext(t, usermem.IOOpts{}), + writeAddr: rvec, + writeIovecCount: riovcnt, + } + default: + panic(fmt.Sprintf("unknown process vm op type: %v", op)) } - // For the read case, we read from the remote process and write to the local process. - if isRemote { - op = localReadRemoteWrite + + var ( + n int + err error + ) + if localTask == remoteTask { + // No need to lock remote process's task mutex since it is the + // same as this process. + n, err = doProcessVMOpMaybeLocked(t, opArgs) + } else { + // Need to take remote process's task mutex. + remoteTask.WithMuLocked(func(*kernel.Task) { + n, err = doProcessVMOpMaybeLocked(t, opArgs) + }) } - return doProcessVMReadWrite(remoteProcess, localProcess, rvec, lvec, riovcnt, liovcnt, op) + if err != nil { + return 0, nil, err + } + return uintptr(n), nil, nil } -func doProcessVMReadWrite(rProcess, wProcess *kernel.Task, rAddr, wAddr hostarch.Addr, rIovecCount, wIovecCount int, op vmReadWriteOp) (uintptr, *kernel.SyscallControl, error) { - rCtx := rProcess.CopyContext(rProcess, usermem.IOOpts{}) - wCtx := wProcess.CopyContext(wProcess, usermem.IOOpts{}) +type processVMOpArgs struct { + readCtx marshal.CopyContext + readAddr hostarch.Addr + readIovecCount int + writeCtx marshal.CopyContext + writeAddr hostarch.Addr + writeIovecCount int +} - var wCount int - doProcessVMReadWriteMaybeLocked := func() error { - rIovecs, err := rCtx.CopyInIovecs(rAddr, rIovecCount) +func doProcessVMOpMaybeLocked(t *kernel.Task, args processVMOpArgs) (int, error) { + // Copy IOVecs in to kernel. + readIovecs, err := t.CopyInIovecsAsSlice(args.readAddr, args.readIovecCount) + if err != nil { + return 0, err + } + writeIovecs, err := t.CopyInIovecsAsSlice(args.writeAddr, args.writeIovecCount) + if err != nil { + return 0, err + } + + // Get scratch buffer from the calling task. + // Size should be max be size of largest read iovec. + var bufSize int + for _, readIovec := range readIovecs { + if int(readIovec.Length()) > bufSize { + bufSize = int(readIovec.Length()) + } + } + buf := t.CopyScratchBuffer(bufSize) + + // Number of bytes written. + var n int + for _, readIovec := range readIovecs { + if len(writeIovecs) == 0 { + break + } + + buf = buf[0:int(readIovec.Length())] + bytes, err := args.readCtx.CopyInBytes(readIovec.Start, buf) + if linuxerr.Equals(linuxerr.EFAULT, err) { + return n, nil + } if err != nil { - return err + return n, err } - wIovecs, err := wCtx.CopyInIovecs(wAddr, wIovecCount) - if err != nil { - return err + if bytes != int(readIovec.Length()) { + return n, nil } - bufSize := 0 - for _, rIovec := range rIovecs { - if int(rIovec.Length()) > bufSize { - bufSize = int(rIovec.Length()) + start := 0 + for bytes > start && 0 < len(writeIovecs) { + writeLength := int(writeIovecs[0].Length()) + if writeLength > (bytes - start) { + writeLength = bytes - start } - } - - var buf []byte - // We need to copy the called task's scratch buffer so we don't get a data race. If we are - // reading a remote process's memory, then we are on the writer's task goroutine, so use - // the write context's scratch buffer. - if op == remoteReadLocalWrite { - buf = wCtx.CopyScratchBuffer(bufSize) - } else { - buf = rCtx.CopyScratchBuffer(bufSize) - } - - for _, rIovec := range rIovecs { - if len(wIovecs) <= 0 { - break - } - - buf = buf[0:int(rIovec.Length())] - bytes, err := rCtx.CopyInBytes(rIovec.Start, buf) + out, err := args.writeCtx.CopyOutBytes(writeIovecs[0].Start, buf[start:writeLength+start]) + n += out + start += out if linuxerr.Equals(linuxerr.EFAULT, err) { - return nil + return n, nil } if err != nil { - return err + return n, err } - if bytes != int(rIovec.Length()) { - return nil + if out != writeLength { + return n, nil } - start := 0 - for bytes > start && 0 < len(wIovecs) { - writeLength := int(wIovecs[0].Length()) - if writeLength > (bytes - start) { - writeLength = bytes - start - } - out, err := wCtx.CopyOutBytes(wIovecs[0].Start, buf[start:writeLength+start]) - wCount += out - start += out - if linuxerr.Equals(linuxerr.EFAULT, err) { - return nil - } - if err != nil { - return err - } - if out != writeLength { - return nil - } - wIovecs[0].Start += hostarch.Addr(out) - if !wIovecs[0].WellFormed() { - return err - } - if wIovecs[0].Length() == 0 { - wIovecs = wIovecs[1:] - } + writeIovecs[0].Start += hostarch.Addr(out) + if !writeIovecs[0].WellFormed() { + return n, err + } + if writeIovecs[0].Length() == 0 { + writeIovecs = writeIovecs[1:] } } - return nil } - - var err error - - switch op { - case remoteReadLocalWrite: - err = rCtx.WithTaskMutexLocked(doProcessVMReadWriteMaybeLocked) - case localReadRemoteWrite: - err = wCtx.WithTaskMutexLocked(doProcessVMReadWriteMaybeLocked) - - case localReadLocalWrite: - // in the case of local reads/writes, we don't have to lock the task mutex, because we are - // running on the top of the task's goroutine already. - err = doProcessVMReadWriteMaybeLocked() - default: - panic("unsupported operation passed") - } - - return uintptr(wCount), nil, err + return n, nil } diff --git a/test/syscalls/BUILD b/test/syscalls/BUILD index 64fcb2450..e95a1e594 100644 --- a/test/syscalls/BUILD +++ b/test/syscalls/BUILD @@ -1,5 +1,5 @@ -load("//tools:defs.bzl", "more_shards", "most_shards") load("//test/runner:defs.bzl", "syscall_test") +load("//tools:defs.bzl", "more_shards", "most_shards") package( default_applicable_licenses = ["//:license"], @@ -1189,6 +1189,10 @@ syscall_test( test = "//test/syscalls/linux:processes_test", ) +syscall_test( + test = "//test/syscalls/linux:process_vm_read_write_test", +) + syscall_test( test = "//test/syscalls/linux:deleted_test", ) diff --git a/test/syscalls/linux/BUILD b/test/syscalls/linux/BUILD index 7ad457d20..67caaac8b 100644 --- a/test/syscalls/linux/BUILD +++ b/test/syscalls/linux/BUILD @@ -4632,18 +4632,17 @@ cc_binary( ) cc_binary( - name = "process_vm_read_write", + name = "process_vm_read_write_test", testonly = 1, srcs = ["process_vm_read_write.cc"], linkstatic = 1, deps = [ gtest, - "//test/util:capability_util", "//test/util:logging", - "//test/util:multiprocess_util", "//test/util:posix_error", "//test/util:test_main", "//test/util:test_util", + "@com_google_absl//absl/cleanup", "@com_google_absl//absl/strings", ], ) diff --git a/test/syscalls/linux/process_vm_read_write.cc b/test/syscalls/linux/process_vm_read_write.cc index c0b4d10c2..d3e9b6346 100644 --- a/test/syscalls/linux/process_vm_read_write.cc +++ b/test/syscalls/linux/process_vm_read_write.cc @@ -22,6 +22,7 @@ #include #include +#include #include #include #include @@ -34,11 +35,10 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" +#include "absl/cleanup/cleanup.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_join.h" -#include "test/util/linux_capability_util.h" #include "test/util/logging.h" -#include "test/util/multiprocess_util.h" #include "test/util/posix_error.h" #include "test/util/test_util.h" @@ -49,59 +49,62 @@ namespace { class TestIovecs { public: - TestIovecs(std::vector& data) { - data_ = std::vector(data.size()); - initial_ = std::vector(data.size()); + TestIovecs(std::vector data) { + data_.resize(data.size()); + iovecs_.resize(data.size()); for (size_t i = 0; i < data.size(); ++i) { data_[i] = data[i]; - initial_[i] = data[i]; - struct iovec iov; - iov.iov_len = data_[i].size(); - iov.iov_base = data_[i].data(); - iovecs_.push_back(iov); bytes_ += data[i].size(); + iovecs_[i].iov_len = data_[i].size(); + iovecs_[i].iov_base = data_[i].data(); } } - bool compare(std::vector other) { - auto want = absl::StrJoin(other, ""); - auto got = absl::StrJoin(data_, ""); - // If the other buffer is smaller than this, make sure the remaining bytes - // haven't been overwritten. - if (want.size() < got.size()) { - auto initial = absl::StrJoin(initial_, ""); - want = absl::StrCat(want, initial.substr(want.size())); + void erase() { + for (size_t i = 0; i < data_.size(); i++) { + data_[i].clear(); } - // If the other buffer is smaller, truncate it so we can compare the two. - if (want.size() > got.size()) { - want = want.substr(0, got.size()); - } - if (want != got) { - std::cerr << "Mismatch buffers:\n want: " << want << "\n got: " << got - << std::endl; - return false; - } - - return true; } - std::vector marshal() { - std::vector ret(iovecs_.size()); - for (size_t i = 0; i < iovecs_.size(); ++i) { - ret[i] = &iovecs_[i]; - } - return ret; - } - - ssize_t total_bytes() { return bytes_; } - - private: - ssize_t bytes_ = 0; + // Backing data that will be read/written. std::vector data_; - std::vector initial_; - std::vector iovecs_; + + // Total size of data_. + ssize_t bytes_ = 0; + + // Iovec structs that point into data_ + std::vector iovecs_; }; +// bytes_match checks that the two TestIovecs are at least min_bytes in length, +// and that they agree in the first min_bytes. +bool bytes_match(TestIovecs first_iov, TestIovecs second_iov, + size_t min_bytes) { + auto first = absl::StrJoin(first_iov.data_, ""); + if (first.size() < min_bytes) { + std::cout << "First buffer smaller than min_bytes: " << min_bytes + << " buffer: " << first << std::endl; + return false; + } + first = first.substr(0, min_bytes); + + auto second = absl::StrJoin(second_iov.data_, ""); + if (second.size() < min_bytes) { + std::cout << "First buffer smaller than min_bytes: " << min_bytes + << " buffer: " << second << std::endl; + return false; + } + second = second.substr(0, min_bytes); + + if (first != second) { + std::cout << "Mismatch buffers:\n first: " << first + << "\n second: " << second << std::endl; + return false; + } + + return true; +} + struct ProcessVMTestCase { std::string test_name; std::vector local_data; @@ -110,115 +113,6 @@ struct ProcessVMTestCase { using ProcessVMTest = ::testing::TestWithParam; -bool ProcessVMCallsNotSupported() { - struct iovec iov = {}; - // Flags should be 0. - ssize_t ret = process_vm_readv(0, &iov, 1, &iov, 1, 10); - if (ret != 0 && errno == ENOSYS) return true; - - ret = process_vm_writev(0, &iov, 1, &iov, 1, 10); - return ret != 0 && errno == ENOSYS; -} - -// TestReadvSameProcess calls process_vm_readv in the same process with -// various local/remote buffers. -TEST_P(ProcessVMTest, TestReadvSameProcess) { - SKIP_IF(ProcessVMCallsNotSupported()); - auto local_data = GetParam().local_data; - auto remote_data = GetParam().remote_data; - TestIovecs local_iovecs(local_data); - TestIovecs remote_iovecs(remote_data); - - auto local = local_iovecs.marshal(); - auto remote = remote_iovecs.marshal(); - auto expected_bytes = - std::min(remote_iovecs.total_bytes(), local_iovecs.total_bytes()); - EXPECT_THAT(process_vm_readv(getpid(), *(local.data()), local.size(), - *(remote.data()), remote.size(), 0), - SyscallSucceedsWithValue(expected_bytes)); - EXPECT_TRUE(local_iovecs.compare(remote_data)); -} - -// TestReadvSubProcess repeats the previous test in a forked process. -TEST_P(ProcessVMTest, TestReadvSubProcess) { - SKIP_IF(ProcessVMCallsNotSupported()); - SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE((HaveCapability(CAP_SYS_PTRACE)))); - auto local_data = GetParam().local_data; - auto remote_data = GetParam().remote_data; - - TestIovecs remote_iovecs(remote_data); - auto remote = remote_iovecs.marshal(); - auto remote_ptr = remote[0]; - auto remote_size = remote.size(); - auto remote_total_bytes = remote_iovecs.total_bytes(); - - const std::function fn = [local_data, remote_data, remote_ptr, - remote_size, remote_total_bytes] { - std::vector local_fn_data = local_data; - TestIovecs local_iovecs(local_fn_data); - auto local = local_iovecs.marshal(); - int ret = process_vm_readv(getppid(), local[0], local.size(), remote_ptr, - remote_size, 0); - auto expected_bytes = - std::min(remote_total_bytes, local_iovecs.total_bytes()); - TEST_CHECK_MSG( - ret == expected_bytes, - absl::StrCat("want: ", expected_bytes, " got: ", ret).c_str()); - TEST_CHECK(local_iovecs.compare(remote_data)); - }; - EXPECT_THAT(InForkedProcess(fn), IsPosixErrorOkAndHolds(0)); -} - -// TestWritevSameProcess calls process_vm_writev in the same process with -// various local/remote buffers. -TEST_P(ProcessVMTest, TestWritevSameProcess) { - SKIP_IF(ProcessVMCallsNotSupported()); - auto local_data = GetParam().local_data; - auto remote_data = GetParam().remote_data; - - TestIovecs local_iovecs(local_data); - TestIovecs remote_iovecs(remote_data); - - auto local = local_iovecs.marshal(); - auto remote = remote_iovecs.marshal(); - auto expected_bytes = - std::min(remote_iovecs.total_bytes(), local_iovecs.total_bytes()); - EXPECT_THAT(process_vm_writev(getpid(), remote[0], remote.size(), local[0], - local.size(), 0), - SyscallSucceedsWithValue(expected_bytes)); - EXPECT_TRUE(local_iovecs.compare(remote_data)); -} - -// TestWritevSubProcess repeats the previous test in a forked process. -TEST_P(ProcessVMTest, TestWritevSubProcess) { - SKIP_IF(ProcessVMCallsNotSupported()); - SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE((HaveCapability(CAP_SYS_PTRACE)))); - auto local_data = GetParam().local_data; - auto remote_data = GetParam().remote_data; - TestIovecs remote_iovecs(remote_data); - auto remote = remote_iovecs.marshal(); - auto remote_ptr = remote[0]; - auto remote_size = remote.size(); - auto remote_total_bytes = remote_iovecs.total_bytes(); - - const std::function fn = [local_data, remote_ptr, remote_size, - remote_total_bytes] { - std::vector local_fn_data = local_data; - TestIovecs local_iovecs(local_fn_data); - auto local = local_iovecs.marshal(); - int ret = process_vm_writev(getppid(), local[0], local.size(), remote_ptr, - remote_size, 0); - auto expected_bytes = - std::min(remote_total_bytes, local_iovecs.total_bytes()); - TEST_CHECK_MSG( - ret == expected_bytes, - absl::StrCat("want: ", expected_bytes, " got: ", ret).c_str()); - }; - - EXPECT_THAT(InForkedProcess(fn), IsPosixErrorOkAndHolds(0)); - EXPECT_TRUE(remote_iovecs.compare(local_data)); -} - INSTANTIATE_TEST_SUITE_P( ProcessVMTests, ProcessVMTest, ::testing::ValuesIn( @@ -243,8 +137,110 @@ INSTANTIATE_TEST_SUITE_P( return info.param.test_name; }); +// TestReadvSameProcess calls process_vm_readv in the same process with various +// local/remote buffers. +TEST_P(ProcessVMTest, TestReadvSameProcess) { + TestIovecs local(GetParam().local_data); + TestIovecs remote(GetParam().remote_data); + + auto want_size = std::min(remote.bytes_, local.bytes_); + EXPECT_THAT( + process_vm_readv(getpid(), local.iovecs_.data(), local.iovecs_.size(), + remote.iovecs_.data(), remote.iovecs_.size(), 0), + SyscallSucceedsWithValue(want_size)); + EXPECT_TRUE(bytes_match(local, remote, want_size)); +} + +// TestReadvSubProcess reads data from a forked child process. +TEST_P(ProcessVMTest, TestReadvSubProcess) { + TestIovecs local = TestIovecs(GetParam().local_data); + TestIovecs remote = TestIovecs(GetParam().remote_data); + + pid_t pid = fork(); + TEST_CHECK_SUCCESS(pid); + if (pid == 0) { + // Child. This is the "remote" process. + // Wait for parent to read data. + sleep(10); // NOLINT - SleepFor is not async-signal-safe. + _exit(0); + } + + auto cleanup = absl::MakeCleanup([&] { kill(pid, SIGKILL); }); + + // Erase the string data in parent's copy of remote, to make sure we are + // reading data from the child. + remote.erase(); + + // Compare against actual remote (not what we sent to the child, since we + // emptied it). + TestIovecs want_remote = TestIovecs(GetParam().remote_data); + auto want_size = std::min(local.bytes_, want_remote.bytes_); + ASSERT_THAT(process_vm_readv(pid, local.iovecs_.data(), local.iovecs_.size(), + remote.iovecs_.data(), remote.iovecs_.size(), 0), + SyscallSucceedsWithValue(want_size)); + EXPECT_TRUE(bytes_match(local, want_remote, want_size)); +} + +// TestWritevSameProcess calls process_vm_readv in the same process with various +// local/remote buffers. +TEST_P(ProcessVMTest, TestWritevSameProcess) { + TestIovecs local(GetParam().local_data); + TestIovecs remote(GetParam().remote_data); + + auto want_size = std::min(remote.bytes_, local.bytes_); + EXPECT_THAT( + process_vm_writev(getpid(), local.iovecs_.data(), local.iovecs_.size(), + remote.iovecs_.data(), remote.iovecs_.size(), 0), + SyscallSucceedsWithValue(want_size)); + EXPECT_TRUE(bytes_match(local, remote, want_size)); +} + +// TestWritevSubProcess writes data to a forked child process. +TEST_P(ProcessVMTest, TestWritevSubProcess) { + TestIovecs local(GetParam().local_data); + TestIovecs remote(GetParam().remote_data); + + // A pipe is used to wait on the write call from the parent, so we can block + // asserting until the write is complete. + int pipefd[2]; + ASSERT_THAT(pipe(pipefd), SyscallSucceeds()); + + pid_t pid = fork(); + TEST_CHECK_SUCCESS(pid); + if (pid == 0) { + // Child. This is the "remote" process. + close(pipefd[1]); + + // Wait on pipefd. It will be closed after the parent has written. + char buf; + TEST_CHECK_SUCCESS(read(pipefd[0], &buf, sizeof(buf))); + close(pipefd[0]); + + // Check the data. This will exit non-0 in the case of a mismatch. + auto want_size = std::min(local.bytes_, remote.bytes_); + TEST_CHECK(bytes_match(local, remote, want_size)); + + _exit(0); + } + + auto cleanup = absl::MakeCleanup([&] { + int status = 0; + EXPECT_THAT(waitpid(pid, &status, 0), SyscallSucceeds()); + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); + }); + + // Write to the remote. + auto want_size = std::min(local.bytes_, remote.bytes_); + ASSERT_THAT( + process_vm_writev(pid, local.iovecs_.data(), local.iovecs_.size(), + remote.iovecs_.data(), remote.iovecs_.size(), 0), + SyscallSucceedsWithValue(want_size)); + + // Now that we've written, close pipefd to signal the child can continue. + close(pipefd[1]); +} + TEST(ProcessVMInvalidTest, NonZeroFlags) { - SKIP_IF(ProcessVMCallsNotSupported()); struct iovec iov = {}; // Flags should be 0. EXPECT_THAT(process_vm_readv(0, &iov, 1, &iov, 1, 10), @@ -254,13 +250,11 @@ TEST(ProcessVMInvalidTest, NonZeroFlags) { } TEST(ProcessVMInvalidTest, NullLocalIovec) { - SKIP_IF(ProcessVMCallsNotSupported()); struct iovec iov = {}; pid_t child = fork(); if (child == 0) { - while (true) { - sleep(1); - } + sleep(10); // NOLINT - SleepFor is not async-signal-safe. + _exit(0); } EXPECT_THAT(process_vm_readv(child, nullptr, 1, &iov, 1, 0), @@ -273,29 +267,26 @@ TEST(ProcessVMInvalidTest, NullLocalIovec) { } TEST(ProcessVMInvalidTest, NULLRemoteIovec) { - SKIP_IF(ProcessVMCallsNotSupported()); - const std::function fn = [] { - std::string contents = "3263827"; - struct iovec child_iov; - child_iov.iov_base = contents.data(); - child_iov.iov_len = contents.size(); + std::string contents = "3263827"; + struct iovec iov; + iov.iov_base = contents.data(); + iov.iov_len = contents.size(); - pid_t parent = getppid(); - int ret = - process_vm_readv(parent, &child_iov, contents.length(), nullptr, 1, 0); - TEST_CHECK(ret == -1); - TEST_CHECK(errno == EFAULT || errno == EINVAL); + pid_t pid = fork(); + TEST_CHECK_SUCCESS(pid); + if (pid == 0) { + sleep(10); // NOLINT - SleepFor is not async-signal-safe. + _exit(0); + } + auto cleanup = absl::MakeCleanup([&] { kill(pid, SIGKILL); }); - ret = - process_vm_writev(parent, &child_iov, contents.length(), nullptr, 1, 0); - TEST_CHECK(ret == -1); - TEST_CHECK(errno == EFAULT || errno == EINVAL); - }; - ASSERT_THAT(InForkedProcess(fn), IsPosixErrorOkAndHolds(0)); + EXPECT_THAT(process_vm_readv(pid, &iov, contents.length(), nullptr, 1, 0), + SyscallFailsWithErrno(::testing::AnyOf(EFAULT, EINVAL))); + EXPECT_THAT(process_vm_writev(pid, &iov, contents.length(), nullptr, 1, 0), + SyscallFailsWithErrno(::testing::AnyOf(EFAULT, EINVAL))); } TEST(ProcessVMInvalidTest, ProcessNoExist) { - SKIP_IF(ProcessVMCallsNotSupported()); struct iovec iov; EXPECT_THAT(process_vm_readv(-1, &iov, 1, &iov, 1, 0), SyscallFailsWithErrno(::testing::AnyOf(ESRCH, EFAULT))); @@ -304,43 +295,26 @@ TEST(ProcessVMInvalidTest, ProcessNoExist) { } TEST(ProcessVMInvalidTest, GreaterThanIOV_MAX) { - SKIP_IF(ProcessVMCallsNotSupported()); std::string contents = "3263827"; struct iovec iov; iov.iov_base = contents.data(); - auto iov_addr = &iov; - const std::function fn = [=] { - struct iovec child_iov; - std::string contents = "3263827"; - child_iov.iov_base = contents.data(); - child_iov.iov_len = contents.size(); + iov.iov_len = contents.size(); - pid_t parent = getppid(); - TEST_CHECK_MSG(-1 == process_vm_readv(parent, &child_iov, 1, iov_addr, - IOV_MAX + 1, 0) && - errno == EINVAL, - "read remote_process_over_IOV_MAX"); + pid_t pid = fork(); + TEST_CHECK_SUCCESS(pid); + if (pid == 0) { + sleep(10); // NOLINT - SleepFor is not async-signal-safe. + _exit(0); + } + auto cleanup = absl::MakeCleanup([&] { kill(pid, SIGKILL); }); - TEST_CHECK_MSG(-1 == process_vm_writev(parent, &child_iov, 1, iov_addr, - IOV_MAX + 3, 0) && - errno == EINVAL, - "write remote process over IOV_MAX"); - - TEST_CHECK_MSG(-1 == process_vm_readv(parent, &child_iov, IOV_MAX + 2, - iov_addr, 1, 0) && - errno == EINVAL, - "read local process over IOV_MAX"); - - TEST_CHECK_MSG(-1 == process_vm_writev(parent, &child_iov, IOV_MAX + 8, - iov_addr, 1, 0) && - errno == EINVAL, - "write local process over IOV_MAX"); - }; - EXPECT_THAT(InForkedProcess(fn), IsPosixErrorOkAndHolds(0)); + EXPECT_THAT(process_vm_readv(pid, &iov, 1, &iov, IOV_MAX + 1, 0), + SyscallFailsWithErrno(EINVAL)); + EXPECT_THAT(process_vm_writev(pid, &iov, 1, &iov, IOV_MAX + 1, 0), + SyscallFailsWithErrno(EINVAL)); } TEST(ProcessVMInvalidTest, PartialReadWrite) { - SKIP_IF(ProcessVMCallsNotSupported()); std::string iov_content_1 = "1138"; std::string iov_content_2 = "3720"; struct iovec iov[2]; @@ -371,8 +345,6 @@ TEST(ProcessVMInvalidTest, PartialReadWrite) { } TEST(ProcessVMTest, WriteToZombie) { - SKIP_IF(ProcessVMCallsNotSupported()); - SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE((HaveCapability(CAP_SYS_PTRACE)))); char* data = {0}; pid_t child; ASSERT_THAT(child = fork(), SyscallSucceeds()); @@ -388,6 +360,41 @@ TEST(ProcessVMTest, WriteToZombie) { ASSERT_THAT(process_vm_writev(child, &iov, 1, &iov, 1, 0), SyscallFailsWithErrno(ESRCH)); } + +// TestReadvNull calls process_vm_readv with null iovecs and checks that they +// succeed but return 0; +TEST(ProcessVMTest, TestReadvNull) { + TestIovecs local(std::vector{"foo"}); + TestIovecs remote(std::vector{"bar"}); + + // Pass 0 for local. + EXPECT_THAT(process_vm_readv(getpid(), 0, 0, remote.iovecs_.data(), + remote.iovecs_.size(), 0), + SyscallSucceedsWithValue(0)); + + // Pass 0 for remote. + EXPECT_THAT(process_vm_readv(getpid(), local.iovecs_.data(), + local.iovecs_.size(), 0, 0, 0), + SyscallSucceedsWithValue(0)); +} + +// TestWritevNull calls process_vm_writev with null iovecs and checks that they +// succeed but return 0; +TEST(ProcessVMTest, TestWritevNull) { + TestIovecs local(std::vector{"foo"}); + TestIovecs remote(std::vector{"bar"}); + + // Pass 0 for local. + EXPECT_THAT(process_vm_writev(getpid(), 0, 0, remote.iovecs_.data(), + remote.iovecs_.size(), 0), + SyscallSucceedsWithValue(0)); + + // Pass 0 for remote. + EXPECT_THAT(process_vm_writev(getpid(), local.iovecs_.data(), + local.iovecs_.size(), 0, 0, 0), + SyscallSucceedsWithValue(0)); +} + } // namespace } // namespace testing } // namespace gvisor