From 96b0067ca3e2dbc6471936e3ff2ca78235e32ae3 Mon Sep 17 00:00:00 2001 From: Matthew Reese Date: Wed, 22 Oct 2025 10:50:22 -0700 Subject: [PATCH 01/12] feat: add very basic temporary file API --- include/asmgrader/api/tempfile.hpp | 48 +++++++++ include/asmgrader/asmgrader.hpp | 1 + src/CMakeLists.txt | 1 + src/api/tempfile.cpp | 159 +++++++++++++++++++++++++++++ tests/CMakeLists.txt | 1 + tests/test_tempfile.cpp | 77 ++++++++++++++ 6 files changed, 287 insertions(+) create mode 100644 include/asmgrader/api/tempfile.hpp create mode 100644 src/api/tempfile.cpp create mode 100644 tests/test_tempfile.cpp diff --git a/include/asmgrader/api/tempfile.hpp b/include/asmgrader/api/tempfile.hpp new file mode 100644 index 0000000..52e309d --- /dev/null +++ b/include/asmgrader/api/tempfile.hpp @@ -0,0 +1,48 @@ +/// \file +/// Provides an API for creating and managing temporary files in the context of tests +#pragma once + +#include +#include +#include + +#include + +#include +#include +#include +#include + +namespace asmgrader { + +/// Interface onto a temporary file. +class TempFile : NonMovable +{ +public: + TempFile(); + explicit TempFile(u16 perms); + ~TempFile() noexcept; + + std::string read_all(); + void write(std::string_view str); + void truncate(); + + std::filesystem::path path() const { return file_info_.path; } + + std::string path_str() const { return file_info_.path.string(); } + + [[nodiscard]] static std::filesystem::path unique_path(); + +private: + struct FileInfo + { + std::filesystem::path path; + std::fstream handle; + }; + + [[nodiscard]] static FileInfo generate_unique_file(); + + FileInfo file_info_; +}; + +} // namespace asmgrader diff --git a/include/asmgrader/asmgrader.hpp b/include/asmgrader/asmgrader.hpp index 351ab8d..8b3f230 100644 --- a/include/asmgrader/asmgrader.hpp +++ b/include/asmgrader/asmgrader.hpp @@ -4,6 +4,7 @@ #include // IWYU pragma: export #include // IWYU pragma: export #include // IWYU pragma: export +#include // IWYU pragma: export #include // IWYU pragma: export #include // IWYU pragma: export #include // IWYU pragma: export diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index c8baec2..1982dd4 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -55,6 +55,7 @@ set( user/cl_args.cpp + api/tempfile.cpp api/assignment.cpp api/test_context.cpp api/syntax_highlighter.cpp diff --git a/src/api/tempfile.cpp b/src/api/tempfile.cpp new file mode 100644 index 0000000..0b5d1e5 --- /dev/null +++ b/src/api/tempfile.cpp @@ -0,0 +1,159 @@ +#include "api/tempfile.hpp" + +#include "common/aliases.hpp" +#include "logging.hpp" + +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace asmgrader { + +namespace fs = std::filesystem; + +TempFile::TempFile() + : file_info_{generate_unique_file()} {} + +TempFile::TempFile(u16 perms) + : TempFile() { + auto fs_perms = static_cast(perms); + if ((fs_perms & ~fs::perms::mask) != fs::perms::none) { + LOG_ERROR("Invalid file perms specified ({:o}); Not setting permissions.", perms); + return; + } + + std::error_code err; + fs::permissions(file_info_.path, fs_perms, err); + if (err != std::error_code{}) { + LOG_ERROR("Failed to set file permissions to {:o} for {} : {}", perms, file_info_.path, err); + } +} + +TempFile::~TempFile() noexcept { + std::error_code err; + fs::remove(file_info_.path, err); + if (err != std::error_code{}) { + LOG_ERROR("Failed to remove tempfile {} : {}", file_info_.path, err); + } +} + +std::string TempFile::read_all() { + // seek to the beginning of the file + file_info_.handle.seekg(0); + + std::stringstream strm; + strm << file_info_.handle.rdbuf(); + return strm.str(); +} + +void TempFile::write(std::string_view str) { + // seek to the end of the file + file_info_.handle.seekg(0, std::ios::end); + file_info_.handle << str; +} + +void TempFile::truncate() { + std::error_code err; + fs::resize_file(file_info_.path, 0, err); + if (err != std::error_code{}) { + LOG_ERROR("Failed truncate tempfile {} : {}", file_info_.path, err); + } +} + +std::filesystem::path TempFile::unique_path() { + // Adapted from https://stackoverflow.com/a/79631059 + // Courtesy of Ted Lyngmo + // NOLINTBEGIN + + static const auto chars = [] { + // the characters you'd like to include + auto arr = + std::to_array({'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', + 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', + 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z'}); + // randomize the order of the characters: + ranges::shuffle(arr, std::mt19937(std::random_device{}())); + return arr; + }(); + + constexpr auto length_of_basename = 8; + + thread_local auto baseidx = [&] { + // randomize the initial state: + std::array rv{}; + std::mt19937 prng(std::random_device{}()); + std::uniform_int_distribution dist(0, chars.size() - 1); + ranges::generate(rv, [&] { return static_cast(dist(prng)); }); + return rv; + }(); + + // create the basename from the baseidices: + thread_local auto basename = [&] { + std::array rv{}; + ranges::transform(baseidx, rv.data(), [&](auto idx) { return chars[idx]; }); + return rv; + }(); + + fs::path tmpdir = fs::temp_directory_path(); + + constexpr auto num_tries = 10'000; + + for (int i = 0; i < num_tries; ++i) { // try a few different filenames + // generate the next basename: + size_t idx = length_of_basename; + + do { + --idx; + baseidx[idx] = (baseidx[idx] + 1) % chars.size(); + basename[idx] = chars[baseidx[idx]]; + } while (idx && baseidx[idx] == 0); + + // ...and create a full path: + fs::path tmpfile_path = tmpdir / basename.data(); + + // < C++23, so ios::noreplace is not available + if (!fs::exists(tmpfile_path)) { + return tmpfile_path; + } + } + // NOLINTEND + + LOG_FATAL("Failed to generate a unique filename tmpdir={} | num_tries={} | baseidx={} | basename={}", tmpdir, + num_tries, baseidx, basename); + throw std::runtime_error("Failed to generate a unique filename"); +} + +TempFile::FileInfo TempFile::generate_unique_file() { + TempFile::FileInfo ret{}; + + // FIXME: technically could have a race condition + auto path = unique_path(); + + ret.handle.open(path, std::ios::in | std::ios::out | std::ios::trunc); + ret.path = std::move(path); + + LOG_DEBUG("Creating a temporary file at {}", ret.path); + if (!ret.handle) { + LOG_FATAL("Failed to create temporary file {}", ret.path); + throw std::runtime_error("Failed to create temporary file"); + } + + return ret; +} + +} // namespace asmgrader diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 4f789d5..f2af051 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -27,6 +27,7 @@ set( test_registers_state.cpp test_file_searcher.cpp test_byte_ranges.cpp + test_tempfile.cpp ) ##### Simple assembly executable diff --git a/tests/test_tempfile.cpp b/tests/test_tempfile.cpp new file mode 100644 index 0000000..49d2da9 --- /dev/null +++ b/tests/test_tempfile.cpp @@ -0,0 +1,77 @@ +#include "catch2_custom.hpp" + +#include "api/tempfile.hpp" +#include "common/aliases.hpp" + +#include +#include + +namespace fs = std::filesystem; + +TEST_CASE("temp file creation and removal") { + fs::path path = [] { + asmgrader::TempFile tmp{}; + REQUIRE(fs::exists(tmp.path())); + return tmp.path(); + }(); + + REQUIRE_FALSE(fs::exists(path)); + + auto paths = [] { + asmgrader::TempFile tmp1{}; + REQUIRE(fs::exists(tmp1.path())); + + asmgrader::TempFile tmp2{}; + REQUIRE(fs::exists(tmp2.path())); + + asmgrader::TempFile tmp3{}; + REQUIRE(fs::exists(tmp3.path())); + + return std::vector{tmp1.path(), tmp2.path(), tmp3.path()}; + }(); + + for (const auto& p : paths) { + REQUIRE_FALSE(fs::exists(p)); + } + + path = asmgrader::TempFile{}.path(); + REQUIRE_FALSE(fs::exists(path)); +} + +TEST_CASE("temp files with specified permissions") { + auto verify_perms = [](asmgrader::u16 perms) { + asmgrader::TempFile tmp1{perms}; + REQUIRE(static_cast(fs::status(tmp1.path()).permissions()) == perms); + }; + + verify_perms(0000); + verify_perms(0777); + verify_perms(0700); + verify_perms(0770); + verify_perms(0666); + verify_perms(0660); + verify_perms(0606); + verify_perms(0066); + verify_perms(0006); +} + +TEST_CASE("temp file reading and writing") { + using Catch::Matchers::IsEmpty; + + asmgrader::TempFile tmp{}; + + REQUIRE_THAT(tmp.read_all(), IsEmpty()); + tmp.truncate(); + REQUIRE_THAT(tmp.read_all(), IsEmpty()); + tmp.write(""); + REQUIRE_THAT(tmp.read_all(), IsEmpty()); + + tmp.write("hello, world"); + REQUIRE(tmp.read_all() == "hello, world"); + + tmp.write("\n123"); + REQUIRE(tmp.read_all() == "hello, world\n123"); + + tmp.truncate(); + REQUIRE_THAT(tmp.read_all(), IsEmpty()); +} From 7c6dd29919cd6ff565d9254a8e8d12eec866012d Mon Sep 17 00:00:00 2001 From: Matthew Reese Date: Wed, 22 Oct 2025 10:51:08 -0700 Subject: [PATCH 02/12] fix: ensure open fds are marked as FD_CLOEXEC in parent proc --- include/asmgrader/subprocess/subprocess.hpp | 4 +++ src/subprocess/subprocess.cpp | 39 +++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/include/asmgrader/subprocess/subprocess.hpp b/include/asmgrader/subprocess/subprocess.hpp index 885fe4f..69fdc63 100644 --- a/include/asmgrader/subprocess/subprocess.hpp +++ b/include/asmgrader/subprocess/subprocess.hpp @@ -85,6 +85,10 @@ class Subprocess : NonCopyable Result read_stdout_poll_impl(int timeout_ms); + /// Marks all open fds (other than 0,1,2) as FD_CLOEXEC so that they get closed in the child proc + /// Run in the PARENT process. + Expected<> mark_cloexec_all() const; + /// Reads any data on the stdout pipe to stdout_buffer_ Result read_stdout_impl(); diff --git a/src/subprocess/subprocess.cpp b/src/subprocess/subprocess.cpp index 7ba7cf4..260ded8 100644 --- a/src/subprocess/subprocess.cpp +++ b/src/subprocess/subprocess.cpp @@ -14,7 +14,10 @@ #include #include #include +#include +#include #include +#include #include #include #include @@ -237,6 +240,10 @@ Result Subprocess::create(const std::string& exec, const std::vector Subprocess::init_child() { TRYE(linux::close(stdin_pipe_.write_fd), SyscallFailure); TRYE(linux::close(stdout_pipe_.read_fd), SyscallFailure); + namespace fs = std::filesystem; + + for (const auto& entry : fs::directory_iterator("/proc/self/fd")) { + int fd = std::stoi(entry.path().filename().string()); + + // skip stdin, stdout, stderr + if (fd <= 2) { + continue; + } + + // auto res = linux::close(fd); + // + // // If close(2) failed for a reason other than the fd not existing, return an error + // if (!res && res != linux::make_error_code(EBADF)) { + // return ErrorKind::SyscallFailure; + // } + } + return {}; } @@ -286,4 +311,18 @@ Result Subprocess::init_parent() { return {}; } +Expected<> Subprocess::mark_cloexec_all() const { + namespace fs = std::filesystem; + for (const auto& entry : fs::directory_iterator("/proc/self/fd")) { + int fd = std::stoi(entry.path().filename().string()); + + if (fd > 2) { + int flags = TRY(linux::fcntl(fd, F_GETFD)); + TRY(linux::fcntl(fd, F_SETFD, flags | FD_CLOEXEC)); + } + } + + return {}; +} + } // namespace asmgrader From 7be50c6521cc32a923e4f74e5e67983063fe9e41 Mon Sep 17 00:00:00 2001 From: Matthew Reese Date: Wed, 22 Oct 2025 10:52:27 -0700 Subject: [PATCH 03/12] chore: refactor run result --- include/asmgrader/subprocess/run_result.hpp | 62 +++++++++++++++++++-- src/CMakeLists.txt | 1 - src/subprocess/run_result.cpp | 29 ---------- 3 files changed, 56 insertions(+), 36 deletions(-) delete mode 100644 src/subprocess/run_result.cpp diff --git a/include/asmgrader/subprocess/run_result.hpp b/include/asmgrader/subprocess/run_result.hpp index 74e9cba..354ccf2 100644 --- a/include/asmgrader/subprocess/run_result.hpp +++ b/include/asmgrader/subprocess/run_result.hpp @@ -1,5 +1,10 @@ #pragma once +#include + +#include +#include + namespace asmgrader { class RunResult @@ -7,18 +12,63 @@ class RunResult public: enum class Kind { Exited, Killed, SignalCaught }; - static RunResult make_exited(int code); - static RunResult make_killed(int code); - static RunResult make_signal_caught(int code); + static constexpr RunResult make_exited(int code); + static constexpr RunResult make_killed(int code); + static constexpr RunResult make_signal_caught(int code); + + constexpr Kind get_kind() const; + constexpr int get_code() const; - Kind get_kind() const; - int get_code() const; + constexpr bool operator==(const RunResult&) const = default; private: - RunResult(Kind kind, int code); + constexpr RunResult(Kind kind, int code); Kind kind_; int code_; }; +constexpr RunResult::RunResult(Kind kind, int code) + : kind_{kind} + , code_{code} {} + +constexpr RunResult RunResult::make_exited(int code) { + return {Kind::Exited, code}; +} + +constexpr RunResult RunResult::make_killed(int code) { + return {Kind::Killed, code}; +} + +constexpr RunResult RunResult::make_signal_caught(int code) { + return {Kind::SignalCaught, code}; +} + +constexpr RunResult::Kind RunResult::get_kind() const { + return kind_; +} + +constexpr int RunResult::get_code() const { + return code_; +} + +static constexpr auto RUN_SUCCESS = RunResult::make_exited(0); + +constexpr std::string_view format_as(const RunResult::Kind& from) { + switch (from) { + case RunResult::Kind::Exited: + return "Exited"; + case RunResult::Kind::Killed: + return "Killed"; + case RunResult::Kind::SignalCaught: + return "SignalCaught"; + default: + return ""; + } +} + +inline std::string format_as(const RunResult& from) { + return fmt::format("{}({})", from.get_kind(), from.get_code()); +} + } // namespace asmgrader diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 1982dd4..0093c31 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -36,7 +36,6 @@ set( subprocess/tracer.cpp subprocess/memory/memory_io_base.cpp subprocess/memory/ptrace_memory_io.cpp - subprocess/run_result.cpp common/terminal_checks.cpp diff --git a/src/subprocess/run_result.cpp b/src/subprocess/run_result.cpp deleted file mode 100644 index 0926487..0000000 --- a/src/subprocess/run_result.cpp +++ /dev/null @@ -1,29 +0,0 @@ -#include "subprocess/run_result.hpp" - -namespace asmgrader { - -RunResult::RunResult(Kind kind, int code) - : kind_{kind} - , code_{code} {} - -RunResult RunResult::make_exited(int code) { - return {Kind::Exited, code}; -} - -RunResult RunResult::make_killed(int code) { - return {Kind::Killed, code}; -} - -RunResult RunResult::make_signal_caught(int code) { - return {Kind::SignalCaught, code}; -} - -RunResult::Kind RunResult::get_kind() const { - return kind_; -} - -int RunResult::get_code() const { - return code_; -} - -} // namespace asmgrader From 1af46f8ba779009624f279b539adcb037e8c797d Mon Sep 17 00:00:00 2001 From: Matthew Reese Date: Wed, 22 Oct 2025 10:53:11 -0700 Subject: [PATCH 04/12] feat: add api for process statistics and minor api and logging tweaks --- include/asmgrader/api/process_statistics.hpp | 43 ++++++++++++++++++ include/asmgrader/api/test_context.hpp | 11 ++++- include/asmgrader/common/linux.hpp | 42 +++++++++--------- include/asmgrader/subprocess/tracer.hpp | 3 ++ src/CMakeLists.txt | 1 + src/api/process_statistics.cpp | 46 ++++++++++++++++++++ src/api/test_context.cpp | 27 +++++++++++- 7 files changed, 150 insertions(+), 23 deletions(-) create mode 100644 include/asmgrader/api/process_statistics.hpp create mode 100644 src/api/process_statistics.cpp diff --git a/include/asmgrader/api/process_statistics.hpp b/include/asmgrader/api/process_statistics.hpp new file mode 100644 index 0000000..a7833f9 --- /dev/null +++ b/include/asmgrader/api/process_statistics.hpp @@ -0,0 +1,43 @@ +/// \file +/// Provides an API for basic process statistics +#pragma once + +#include +#include +#include +#include + +#include + +namespace asmgrader { + +/// Obtains statistics of a process with a given PID. +/// Provides various accessors returning POD types. +/// +/// For now, features are extremely limited. +/// +/// Makes use of proc(5) +class ProcessStats +{ +public: + /// Open File Descriptor(s) Info POD + struct OpenFds + { + std::vector fds; + }; + + explicit ProcessStats(pid_t pid); + + OpenFds open_fds() const { return open_fds_; } + +private: + std::filesystem::path procfs_path(std::string_view suffix) const; + + OpenFds get_open_fds() const; + + pid_t pid_; + + OpenFds open_fds_; +}; + +} // namespace asmgrader diff --git a/include/asmgrader/api/test_context.hpp b/include/asmgrader/api/test_context.hpp index e860128..d07a5af 100644 --- a/include/asmgrader/api/test_context.hpp +++ b/include/asmgrader/api/test_context.hpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -107,7 +108,15 @@ class TestContext AsmFunction find_function(std::string name); /// Run the program normally from `_start`, stopping at the first exit(2) or exit_group(2) syscall invocation - RunResult run(); + Result run(); + + /// Run the program from `_start`, stopping at the first syscall matching syscallnr + /// OR the first exit(2) or exit_group(2) syscall invocation [whichever happens first] + Result run_until(u64 syscallnr); + + /// Obtain statistics for the subprocess being tested + /// Data is more limited when the process is not stopped! + ProcessStats stats(); private: bool require_impl(bool condition, const std::string& description, diff --git a/include/asmgrader/common/linux.hpp b/include/asmgrader/common/linux.hpp index 5937ad9..afcfdee 100644 --- a/include/asmgrader/common/linux.hpp +++ b/include/asmgrader/common/linux.hpp @@ -52,7 +52,7 @@ inline Expected write(int fd, std::string_view data) { if (res == -1) { auto err = make_error_code(errno); - LOG_DEBUG("write(fd={}, data={:?}, size={}) failed: '{}'", fd, data, data.size(), err); + LOG_TRACE("write(fd={}, data={:?}, size={}) failed: '{}'", fd, data, data.size(), err); return err; } @@ -71,7 +71,7 @@ inline Expected read(int fd, size_t count) { // NOLINT if (res == -1) { auto err = make_error_code(errno); - LOG_DEBUG("read(fd={}, count={}) failed: '{}'", fd, count, err); + LOG_TRACE("read(fd={}, count={}) failed: '{}'", fd, count, err); return err; } @@ -92,10 +92,12 @@ inline Expected<> close(int fd) { if (res == -1) { auto err = make_error_code(errno); - LOG_DEBUG("close failed: '{}'", err); + LOG_TRACE("close(fd={}) failed: '{}'", fd, err); return err; } + LOG_TRACE("close(fd={}) = {}", fd, res); + return {}; } @@ -107,12 +109,12 @@ inline Expected<> kill(pid_t pid, int sig) { if (res == -1) { auto err = make_error_code(errno); - LOG_DEBUG("kill(pid={}, sig={}) failed: '{}'", pid, sig, err); + LOG_TRACE("kill(pid={}, sig={}) failed: '{}'", pid, sig, err); return err; } - LOG_DEBUG("kill(pid={}, sig={}) = {}", pid, sig, res); + LOG_TRACE("kill(pid={}, sig={}) = {}", pid, sig, res); return {}; } @@ -127,7 +129,7 @@ inline Expected<> access(gsl::czstring path, int mode) { if (res == -1) { auto err = make_error_code(errno); - LOG_DEBUG("access(path={:?}, mode={}) failed '{}'", path, mode, err); + LOG_TRACE("access(path={:?}, mode={}) failed '{}'", path, mode, err); return err; } @@ -160,9 +162,9 @@ inline Expected<> execve(const std::string& exec, const std::vector auto err = make_error_code(errno); if (res == -1) { - LOG_DEBUG("execve failed: '{}'", err); + LOG_TRACE("execve failed: '{}'", err); } else { - LOG_DEBUG("execve failed (INVALID RETURN CODE = {}): '{}'", res, err); + LOG_TRACE("execve failed (INVALID RETURN CODE = {}): '{}'", res, err); } return err; @@ -182,7 +184,7 @@ inline Expected fork() { if (res == -1) { auto err = make_error_code(errno); - LOG_DEBUG("fork failed: '{}'", err); + LOG_TRACE("fork failed: '{}'", err); return err; } @@ -201,7 +203,7 @@ inline Expected open(const std::string& pathname, int flags, mode_t mode = if (res == -1) { auto err = make_error_code(errno); - LOG_DEBUG("open(pathname={:?}, flags={}, mode={}) failed: '{}'", pathname, flags, mode, err); + LOG_TRACE("open(pathname={:?}, flags={}, mode={}) failed: '{}'", pathname, flags, mode, err); return err; } @@ -217,7 +219,7 @@ inline Expected lseek(int fd, off_t offset, int whence) { if (res == -1) { auto err = make_error_code(errno); - LOG_DEBUG("lseek failed: '{}'", err); + LOG_TRACE("lseek failed: '{}'", err); return err; } @@ -233,9 +235,9 @@ inline Expected<> dup2(int oldfd, int newfd) { auto err = make_error_code(errno); if (res == -1) { - LOG_DEBUG("dup2 failed: '{}'", err); + LOG_TRACE("dup2 failed: '{}'", err); } else { - LOG_DEBUG("dup2 failed (INVALID RETURN CODE = {}): '{}'", res, err); + LOG_TRACE("dup2 failed (INVALID RETURN CODE = {}): '{}'", res, err); } return err; @@ -254,7 +256,7 @@ inline Expected ioctl(int fd, unsigned long request, void* argp) { if (res == -1) { auto err = make_error_code(errno); - LOG_DEBUG("ioctl failed: '{}'", err); + LOG_TRACE("ioctl failed: '{}'", err); return err; } @@ -278,7 +280,7 @@ inline Expected fcntl(int fd, int cmd, std::optional arg = std::nullop if (res == -1) { auto err = make_error_code(errno); - LOG_DEBUG("fcntl failed: '{}'", err); + LOG_TRACE("fcntl failed: '{}'", err); return err; } @@ -295,7 +297,7 @@ inline Expected waitid(idtype_t idtype, id_t id, int options = WSTOPP if (res == -1) { auto err = make_error_code(errno); - LOG_DEBUG("waitid(idtype={}, id={}, options={}) failed: '{}'", fmt::underlying(idtype), id, options, err); + LOG_TRACE("waitid(idtype={}, id={}, options={}) failed: '{}'", fmt::underlying(idtype), id, options, err); return err; } @@ -314,7 +316,7 @@ inline Expected<> raise(int sig) { if (res == -1) { auto err = std::error_code(errno, std::system_category()); - LOG_DEBUG("raise({}) failed: '{}'", sig, err); + LOG_TRACE("raise({}) failed: '{}'", sig, err); return err; } @@ -343,7 +345,7 @@ inline Expected pipe2(int flags = 0) { if (res == -1) { auto err = make_error_code(errno); - LOG_DEBUG("pipe(..., flags={}) failed: '{}'", flags, err); + LOG_TRACE("pipe(..., flags={}) failed: '{}'", flags, err); return err; } @@ -378,7 +380,7 @@ inline Expected ptrace(int request, pid_t pid = 0, AddrT addr = NULL, Data if (errno) { auto err = make_error_code(errno); - LOG_DEBUG("ptrace(req={}, pid={}, addr={}, data={}) failed: '{}'", request, pid, format_or_unknown(addr), + LOG_TRACE("ptrace(req={}, pid={}, addr={}, data={}) failed: '{}'", request, pid, format_or_unknown(addr), format_or_unknown(data), err); return err; @@ -399,7 +401,7 @@ inline Expected stat(const std::string& pathname) { if (res == -1) { auto err = make_error_code(errno); - LOG_DEBUG("stat failed: '{}'", err); + LOG_TRACE("stat failed: '{}'", err); return err; } diff --git a/include/asmgrader/subprocess/tracer.hpp b/include/asmgrader/subprocess/tracer.hpp index 144f1d5..c2f3826 100644 --- a/include/asmgrader/subprocess/tracer.hpp +++ b/include/asmgrader/subprocess/tracer.hpp @@ -30,6 +30,7 @@ #include #include +#include #include #include // pid_t #include // user_regs_struct, user_fpregs_struct (user_fpsimd_struct) @@ -115,6 +116,8 @@ class Tracer std::uintptr_t get_mmapped_addr() const { return mmaped_address_; } + pid_t get_pid() const { return pid_; }; + private: /// Ensure that invariants hold /// pid_ parent is this process diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 0093c31..fb5ed3c 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -55,6 +55,7 @@ set( user/cl_args.cpp api/tempfile.cpp + api/process_statistics.cpp api/assignment.cpp api/test_context.cpp api/syntax_highlighter.cpp diff --git a/src/api/process_statistics.cpp b/src/api/process_statistics.cpp new file mode 100644 index 0000000..5025625 --- /dev/null +++ b/src/api/process_statistics.cpp @@ -0,0 +1,46 @@ +#include "api/process_statistics.hpp" + +#include "logging.hpp" + +#include + +#include +#include +#include + +#include + +namespace asmgrader { + +namespace fs = std::filesystem; + +ProcessStats::ProcessStats(pid_t pid) + : pid_{pid} + , open_fds_{get_open_fds()} {} + +fs::path ProcessStats::procfs_path(std::string_view suffix) const { + constexpr std::string_view procfs_template = "/proc/{}"; + fs::path base_path = fmt::format(procfs_template, pid_); + return base_path / suffix; +} + +ProcessStats::OpenFds ProcessStats::get_open_fds() const { + const fs::path procfs_fds = procfs_path("fd/"); + + if (!fs::exists(procfs_fds)) { + LOG_WARN("Could not get open fds : path {} does not exist (pid={})", procfs_fds, pid_); + return {}; + } + + OpenFds res; + + for (const auto& dir_entry : fs::directory_iterator{procfs_fds}) { + std::string filename = dir_entry.path().filename().string(); + + LOG_DEBUG("Got filename: {:?}", filename); + } + + return res; +} + +} // namespace asmgrader diff --git a/src/api/test_context.cpp b/src/api/test_context.cpp index 7dff727..3a2cbd5 100644 --- a/src/api/test_context.cpp +++ b/src/api/test_context.cpp @@ -2,6 +2,7 @@ #include "api/asm_buffer.hpp" #include "api/metadata.hpp" +#include "api/process_statistics.hpp" #include "api/registers_state.hpp" #include "api/requirement.hpp" #include "api/test_base.hpp" @@ -92,7 +93,7 @@ void TestContext::send_stdin(std::string_view input) { TRY_OR_THROW(prog_.get_subproc().send_stdin(input), "failed to write to stdin"); } -RunResult TestContext::run() { +Result TestContext::run() { int exit_code{}; auto res = prog_.run_until([&exit_code](const SyscallRecord& syscall) { @@ -112,7 +113,29 @@ RunResult TestContext::run() { return RunResult::make_exited(exit_code); } - return TRY_OR_THROW(res, "failed to run program"); + return res; +} + +Result TestContext::run_until(u64 syscallnr) { + std::optional exit_code{}; + + auto res = prog_.run_until([&exit_code, syscallnr](const SyscallRecord& syscall) { + if (syscall.num == SYS_exit || syscall.num == SYS_exit_group) { + exit_code = std::get(syscall.args.at(0)); + return true; + } + return syscall.num == syscallnr; + }); + + if (exit_code.has_value()) { + return RunResult::make_exited(*exit_code); + } + + return res; +} + +ProcessStats TestContext::stats() { + return ProcessStats{prog_.get_subproc().get_pid()}; } void TestContext::restart_program() { From ec5db507b3175c7ac094293334112d1f1ac92d61 Mon Sep 17 00:00:00 2001 From: Matthew Reese Date: Wed, 22 Oct 2025 11:04:56 -0700 Subject: [PATCH 05/12] feat: add TempFile::remove --- include/asmgrader/api/tempfile.hpp | 5 +++++ src/api/tempfile.cpp | 13 +++++++++++++ 2 files changed, 18 insertions(+) diff --git a/include/asmgrader/api/tempfile.hpp b/include/asmgrader/api/tempfile.hpp index 52e309d..0cb2bca 100644 --- a/include/asmgrader/api/tempfile.hpp +++ b/include/asmgrader/api/tempfile.hpp @@ -4,6 +4,7 @@ #include #include +#include #include #include @@ -33,6 +34,10 @@ class TempFile : NonMovable [[nodiscard]] static std::filesystem::path unique_path(); + /// Remove (delete) a file. Intended for use with files created based on + /// \ref unique_path, but could be used to safely remove any arbitrary file. + static Expected<> remove(const std::filesystem::path& path); + private: struct FileInfo { diff --git a/src/api/tempfile.cpp b/src/api/tempfile.cpp index 0b5d1e5..abae0ac 100644 --- a/src/api/tempfile.cpp +++ b/src/api/tempfile.cpp @@ -1,6 +1,7 @@ #include "api/tempfile.hpp" #include "common/aliases.hpp" +#include "common/expected.hpp" #include "logging.hpp" #include @@ -156,4 +157,16 @@ TempFile::FileInfo TempFile::generate_unique_file() { return ret; } +Expected<> TempFile::remove(const fs::path& path) { + std::error_code err; + fs::remove(path, err); + + if (err != std::error_code{}) { + LOG_ERROR("Failed remove file {} : {}", path, err); + return err; + } + + return {}; +} + } // namespace asmgrader From a3c7ee3e8c7fa455edd37de5b488e1f2b7580018 Mon Sep 17 00:00:00 2001 From: Matthew Reese Date: Wed, 22 Oct 2025 11:28:17 -0700 Subject: [PATCH 06/12] add str impl for RunResult --- include/asmgrader/subprocess/run_result.hpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/include/asmgrader/subprocess/run_result.hpp b/include/asmgrader/subprocess/run_result.hpp index 354ccf2..db7177e 100644 --- a/include/asmgrader/subprocess/run_result.hpp +++ b/include/asmgrader/subprocess/run_result.hpp @@ -21,6 +21,8 @@ class RunResult constexpr bool operator==(const RunResult&) const = default; + std::string str() const; + private: constexpr RunResult(Kind kind, int code); @@ -71,4 +73,8 @@ inline std::string format_as(const RunResult& from) { return fmt::format("{}({})", from.get_kind(), from.get_code()); } +inline std::string RunResult::str() const { + return fmt::to_string(*this); +} + } // namespace asmgrader From 3b7bc09621a7681b20f267176c40baca96f0f9e0 Mon Sep 17 00:00:00 2001 From: Matthew Reese Date: Wed, 22 Oct 2025 16:49:28 -0700 Subject: [PATCH 07/12] add more functionality to tempfile api; various fixes and logging improvements --- include/asmgrader/api/tempfile.hpp | 6 ++- include/asmgrader/subprocess/run_result.hpp | 4 ++ src/api/process_statistics.cpp | 6 +++ src/api/tempfile.cpp | 44 +++++++++++++++------ src/api/test_context.cpp | 1 + src/output/plaintext_serializer.cpp | 4 ++ src/subprocess/traced_subprocess.cpp | 3 ++ 7 files changed, 56 insertions(+), 12 deletions(-) diff --git a/include/asmgrader/api/tempfile.hpp b/include/asmgrader/api/tempfile.hpp index 0cb2bca..5a61bfb 100644 --- a/include/asmgrader/api/tempfile.hpp +++ b/include/asmgrader/api/tempfile.hpp @@ -20,7 +20,7 @@ namespace asmgrader { class TempFile : NonMovable { public: - TempFile(); + explicit TempFile(bool create = true); explicit TempFile(u16 perms); ~TempFile() noexcept; @@ -28,6 +28,8 @@ class TempFile : NonMovable void write(std::string_view str); void truncate(); + bool exists() const; + std::filesystem::path path() const { return file_info_.path; } std::string path_str() const { return file_info_.path.string(); } @@ -47,6 +49,8 @@ class TempFile : NonMovable [[nodiscard]] static FileInfo generate_unique_file(); + bool ensure_created(); + FileInfo file_info_; }; diff --git a/include/asmgrader/subprocess/run_result.hpp b/include/asmgrader/subprocess/run_result.hpp index db7177e..47eee48 100644 --- a/include/asmgrader/subprocess/run_result.hpp +++ b/include/asmgrader/subprocess/run_result.hpp @@ -73,6 +73,10 @@ inline std::string format_as(const RunResult& from) { return fmt::format("{}({})", from.get_kind(), from.get_code()); } +constexpr std::string_view str(const RunResult::Kind& from) { + return format_as(from); +} + inline std::string RunResult::str() const { return fmt::to_string(*this); } diff --git a/src/api/process_statistics.cpp b/src/api/process_statistics.cpp index 5025625..1bad891 100644 --- a/src/api/process_statistics.cpp +++ b/src/api/process_statistics.cpp @@ -38,6 +38,12 @@ ProcessStats::OpenFds ProcessStats::get_open_fds() const { std::string filename = dir_entry.path().filename().string(); LOG_DEBUG("Got filename: {:?}", filename); + + try { + res.fds.push_back(std::stoi(filename)); + } catch (...) { + res.fds.push_back(-1); + } } return res; diff --git a/src/api/tempfile.cpp b/src/api/tempfile.cpp index abae0ac..d264ac5 100644 --- a/src/api/tempfile.cpp +++ b/src/api/tempfile.cpp @@ -27,8 +27,12 @@ namespace asmgrader { namespace fs = std::filesystem; -TempFile::TempFile() - : file_info_{generate_unique_file()} {} +TempFile::TempFile(bool create) + : file_info_{.path = unique_path(), .handle = {}} { + if (create) { + ensure_created(); + } +} TempFile::TempFile(u16 perms) : TempFile() { @@ -53,7 +57,27 @@ TempFile::~TempFile() noexcept { } } +bool TempFile::exists() const { + return fs::exists(file_info_.path); +} + +bool TempFile::ensure_created() { + if (!file_info_.handle.is_open()) { + file_info_.handle.open(file_info_.path, std::ios::in | std::ios::out | std::ios::trunc); + + LOG_DEBUG("Creating a temporary file at {}", file_info_.path); + if (!file_info_.handle) { + LOG_FATAL("Failed to create temporary file {}", file_info_.path); + } + } + + return true; +} + std::string TempFile::read_all() { + if (!ensure_created()) { + return ""; + } // seek to the beginning of the file file_info_.handle.seekg(0); @@ -63,12 +87,19 @@ std::string TempFile::read_all() { } void TempFile::write(std::string_view str) { + if (!ensure_created()) { + return; + } // seek to the end of the file file_info_.handle.seekg(0, std::ios::end); file_info_.handle << str; + file_info_.handle.flush(); } void TempFile::truncate() { + if (!ensure_created()) { + return; + } std::error_code err; fs::resize_file(file_info_.path, 0, err); if (err != std::error_code{}) { @@ -145,15 +176,6 @@ TempFile::FileInfo TempFile::generate_unique_file() { // FIXME: technically could have a race condition auto path = unique_path(); - ret.handle.open(path, std::ios::in | std::ios::out | std::ios::trunc); - ret.path = std::move(path); - - LOG_DEBUG("Creating a temporary file at {}", ret.path); - if (!ret.handle) { - LOG_FATAL("Failed to create temporary file {}", ret.path); - throw std::runtime_error("Failed to create temporary file"); - } - return ret; } diff --git a/src/api/test_context.cpp b/src/api/test_context.cpp index 3a2cbd5..af73d1d 100644 --- a/src/api/test_context.cpp +++ b/src/api/test_context.cpp @@ -33,6 +33,7 @@ #include #include #include +#include #include #include diff --git a/src/output/plaintext_serializer.cpp b/src/output/plaintext_serializer.cpp index 7052b15..0d4b582 100644 --- a/src/output/plaintext_serializer.cpp +++ b/src/output/plaintext_serializer.cpp @@ -109,6 +109,10 @@ void PlainTextSerializer::on_test_result(const TestResult& data) { if (!should_output_test(verbosity_)) { return; } + if (data.error.has_value()) { + sink_.write(style_str("Internal test context error: ", ERROR_STYLE)); + sink_.write(fmt::to_string(*data.error) + '\n'); + } // Potentially output a msg for an empty test if (data.num_total == 0) { diff --git a/src/subprocess/traced_subprocess.cpp b/src/subprocess/traced_subprocess.cpp index ab0bbba..3b35e52 100644 --- a/src/subprocess/traced_subprocess.cpp +++ b/src/subprocess/traced_subprocess.cpp @@ -7,6 +7,8 @@ #include "subprocess/syscall_record.hpp" #include "subprocess/tracer.hpp" +#include + #include #include #include @@ -28,6 +30,7 @@ TracedSubprocess::~TracedSubprocess() { } LOG_DEBUG("Processed {} syscalls", tracer_.get_records().size()); + LOG_TRACE("Syscalls: {:?}", fmt::join(tracer_.get_records(), "\n")); if (is_alive()) { std::ignore = kill(); From c2692270f25ebc337542dd594f645df7eed0910b Mon Sep 17 00:00:00 2001 From: Matthew Reese Date: Fri, 24 Oct 2025 15:22:18 -0700 Subject: [PATCH 08/12] add Expected::transform specialization for when T is void --- include/asmgrader/common/expected.hpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/include/asmgrader/common/expected.hpp b/include/asmgrader/common/expected.hpp index cf6e852..d7561ab 100644 --- a/include/asmgrader/common/expected.hpp +++ b/include/asmgrader/common/expected.hpp @@ -160,6 +160,7 @@ class [[nodiscard]] Expected } template + requires(!std::is_void_v) constexpr Expected, E> transform(const Func& func) { if (!has_value()) { return error(); @@ -168,6 +169,16 @@ class [[nodiscard]] Expected return func(value()); } + template + requires(std::is_void_v) + constexpr Expected, E> transform(const Func& func) { + if (!has_value()) { + return error(); + } + + return func(); + } + private: template struct ExpectedData From f85a5383e827fc741eafbe7112908dda92a876f4 Mon Sep 17 00:00:00 2001 From: Matthew Reese Date: Fri, 24 Oct 2025 15:23:19 -0700 Subject: [PATCH 09/12] capture stderr with Subprocess; update API accordingly --- include/asmgrader/subprocess/subprocess.hpp | 86 +++++++++-- src/subprocess/subprocess.cpp | 151 ++++++++++++-------- tests/test_program.cpp | 9 +- tests/test_subprocess.cpp | 4 +- 4 files changed, 172 insertions(+), 78 deletions(-) diff --git a/include/asmgrader/subprocess/subprocess.hpp b/include/asmgrader/subprocess/subprocess.hpp index 69fdc63..e93e94d 100644 --- a/include/asmgrader/subprocess/subprocess.hpp +++ b/include/asmgrader/subprocess/subprocess.hpp @@ -1,10 +1,13 @@ #pragma once +#include #include #include #include #include +#include + #include #include #include @@ -28,17 +31,35 @@ class Subprocess : NonCopyable Subprocess(Subprocess&&) noexcept; Subprocess& operator=(Subprocess&&) noexcept; + /// Which output file descriptor to read from + enum class WhichOutput : u8 { None = 0, Stdout = 1, Stderr = 2, StdoutAndStderr = Stdout | Stderr }; + + /// stdout_str is only valid if WhichOutput::Stdout was included in the request + /// stderr_str is only valid if WhichOutput::Stderr was included in the request + struct OutputResult + { + std::string stdout_str; + std::string stderr_str; + }; + + /// Read buffered output since the last call to this function + OutputResult read_output(WhichOutput which = WhichOutput::StdoutAndStderr); + + /// Get all output since the program has launched + OutputResult read_full_output(WhichOutput which = WhichOutput::StdoutAndStderr); + template - Result read_stdout(const std::chrono::duration& timeout) { - return read_stdout_poll_impl(std::chrono::duration_cast(timeout).count()); + [[deprecated]] Result read_stdout(const std::chrono::duration& timeout) { + read_pipe_poll(stdout_, std::chrono::duration_cast(timeout).count()); + return new_output(WhichOutput::Stdout).stdout_str; } - Result read_stdout(); - - /// Get all stdout since the program has launched - const std::string& get_full_stdout(); + /// Updates output result based on cursor positions + /// If the cursor is located before the end of the string, then a substring is used + /// (starting at the cursor position) + void get_new_output(OutputResult& res); - Result send_stdin(std::string_view str); + Result send_stdin(std::string_view str) const; // Forks the current process to start a new subprocess as specified virtual Result start(); @@ -78,12 +99,16 @@ class Subprocess : NonCopyable /// pipes to communicate with subprocess' stdout and stdin respectively /// The parent process will only make use of the write end of stdin_pipe_, and the read end of stdout_pipe_ linux::Pipe stdin_pipe_{}; - linux::Pipe stdout_pipe_{}; - std::string stdout_buffer_; - std::size_t stdout_cursor_{}; + struct OutputPipe + { + linux::Pipe pipe; + std::string buffer; + std::size_t cursor; + }; - Result read_stdout_poll_impl(int timeout_ms); + OutputPipe stdout_{}; + OutputPipe stderr_{}; /// Marks all open fds (other than 0,1,2) as FD_CLOEXEC so that they get closed in the child proc /// Run in the PARENT process. @@ -92,10 +117,49 @@ class Subprocess : NonCopyable /// Reads any data on the stdout pipe to stdout_buffer_ Result read_stdout_impl(); + /// Reads any immediately available data available on the specified pipe's read end + /// Writes any new data to that OutputPipe's buffer + /// Throws a std::logic_error if any syscalls fail + static void read_pipe_nonblock(OutputPipe& pipe); + + /// Polls the pipe's read end for timeout_ms millis, or until available input arrives + /// Writes any new data to that OutputPipe's buffer + /// \returns true if a successful read occurred, false if timed out + static bool read_pipe_poll(OutputPipe& pipe, int timeout_ms); + + /// Obtain new output based on cursor positions and buffers, + /// and set cursors to the end of their resp. buffers + OutputResult new_output(WhichOutput which); + std::optional exit_code_; std::string exec_; std::vector args_; }; +// TODO: macro for bitfield enums + +constexpr std::string_view format_as(const Subprocess::WhichOutput& from) { + switch (from) { + case Subprocess::WhichOutput::None: + return "none"; + case Subprocess::WhichOutput::Stdout: + return "stdout"; + case Subprocess::WhichOutput::Stderr: + return "stderr"; + case Subprocess::WhichOutput::StdoutAndStderr: + return "stdout&stderr"; + default: + return ""; + } +} + +constexpr Subprocess::WhichOutput operator&(const Subprocess::WhichOutput& lhs, const Subprocess::WhichOutput& rhs) { + return static_cast(fmt::underlying(lhs) & fmt::underlying(rhs)); +} + +constexpr Subprocess::WhichOutput operator|(const Subprocess::WhichOutput& lhs, const Subprocess::WhichOutput& rhs) { + return static_cast(fmt::underlying(lhs) | fmt::underlying(rhs)); +} + } // namespace asmgrader diff --git a/src/subprocess/subprocess.cpp b/src/subprocess/subprocess.cpp index 260ded8..77f8b40 100644 --- a/src/subprocess/subprocess.cpp +++ b/src/subprocess/subprocess.cpp @@ -9,16 +9,16 @@ #include #include -#include #include -#include #include #include #include +#include #include #include #include #include +#include #include #include @@ -129,15 +129,20 @@ Result Subprocess::wait_for_exit(std::chrono::microseconds timeout) { Result Subprocess::close_pipes() { // Make sure all available data is read before pipes are closed - std::ignore = read_stdout_impl(); + read_pipe_nonblock(stdout_); + read_pipe_nonblock(stderr_); if (stdin_pipe_.write_fd != -1) { TRYE(linux::close(stdin_pipe_.write_fd), SyscallFailure); stdin_pipe_.write_fd = -1; } - if (stdout_pipe_.read_fd != -1) { - TRYE(linux::close(stdout_pipe_.read_fd), SyscallFailure); - stdout_pipe_.read_fd = -1; + if (stdout_.pipe.read_fd != -1) { + TRYE(linux::close(stdout_.pipe.read_fd), SyscallFailure); + stdout_.pipe.read_fd = -1; + } + if (stderr_.pipe.read_fd != -1) { + TRYE(linux::close(stderr_.pipe.read_fd), SyscallFailure); + stderr_.pipe.read_fd = -1; } return {}; @@ -146,16 +151,14 @@ Result Subprocess::close_pipes() { Subprocess::Subprocess(Subprocess&& other) noexcept : child_pid_{std::exchange(other.child_pid_, 0)} , stdin_pipe_{std::exchange(other.stdin_pipe_, {})} - , stdout_pipe_{std::exchange(other.stdout_pipe_, {})} - , stdout_buffer_{std::exchange(other.stdout_buffer_, {})} - , stdout_cursor_{std::exchange(other.stdout_cursor_, 0)} {} + , stdout_{std::exchange(other.stdout_, {})} + , stderr_{std::exchange(other.stderr_, {})} {} Subprocess& Subprocess::operator=(Subprocess&& rhs) noexcept { child_pid_ = std::exchange(rhs.child_pid_, 0); stdin_pipe_ = std::exchange(rhs.stdin_pipe_, {}); - stdout_pipe_ = std::exchange(rhs.stdout_pipe_, {}); - stdout_buffer_ = std::exchange(rhs.stdout_buffer_, {}); - stdout_cursor_ = std::exchange(rhs.stdout_cursor_, 0); + stdout_ = std::exchange(rhs.stdout_, {}); + stderr_ = std::exchange(rhs.stderr_, {}); return *this; } @@ -164,72 +167,89 @@ bool Subprocess::is_alive() const { return linux::kill(child_pid_, 0) != std::make_error_code(std::errc::no_such_process); } -Result Subprocess::read_stdout_poll_impl(int timeout_ms) { - // If the pipe is already closed, all we can do is try reading from the buffer - if (stdout_pipe_.read_fd == -1) { - return read_stdout(); +Subprocess::OutputResult Subprocess::read_output(WhichOutput which) { + if ((which & WhichOutput::Stdout) != WhichOutput::None) { + read_pipe_nonblock(stdout_); + } + if ((which & WhichOutput::Stderr) != WhichOutput::None) { + read_pipe_nonblock(stderr_); } - struct pollfd poll_struct = {.fd = stdout_pipe_.read_fd, .events = POLLIN, .revents = 0}; + return new_output(which); +} - // TODO: Create wrapper in linux.hpp - int res = poll(&poll_struct, 1, timeout_ms); - // Error - if (res == -1) { - LOG_WARN("Error polling for read from stdout pipe: '{}'", get_err_msg()); - return ErrorKind::SyscallFailure; +Subprocess::OutputResult Subprocess::read_full_output(WhichOutput which) { + if ((which & WhichOutput::Stdout) != WhichOutput::None) { + read_pipe_nonblock(stdout_); } - // Timeout occured - if (res == 0) { - return ""; + if ((which & WhichOutput::Stderr) != WhichOutput::None) { + read_pipe_nonblock(stderr_); } - return read_stdout(); + return OutputResult{.stdout_str = stdout_.buffer, .stderr_str = stderr_.buffer}; } -Result Subprocess::read_stdout() { - TRY(read_stdout_impl()); +void Subprocess::read_pipe_nonblock(OutputPipe& pipe) { + std::size_t num_bytes_avail = 0; - // Cursor is still at the end of the buffer -> no data was read - if (stdout_cursor_ == stdout_buffer_.size()) { - return ""; + if (pipe.pipe.read_fd == -1) { + LOG_TRACE("Attempted to read from a fd that's already closed ({})", pipe.pipe.read_fd); + return; } - auto res = stdout_buffer_.substr(stdout_cursor_); - stdout_cursor_ = stdout_buffer_.size(); + if (!linux::ioctl(pipe.pipe.read_fd, FIONREAD, &num_bytes_avail)) { + throw std::logic_error("ioctl for pipe failed"); + } - return res; -} + LOG_DEBUG("{} bytes available from fd ({})", num_bytes_avail, pipe.pipe.read_fd); -const std::string& Subprocess::get_full_stdout() { - std::ignore = read_stdout_impl(); + if (num_bytes_avail == 0) { + return; + } - return stdout_buffer_; + if (auto res = linux::read(pipe.pipe.read_fd, num_bytes_avail)) { + pipe.buffer += res.value(); + } else { + throw std::logic_error("read from pipe failed"); + } } -Result Subprocess::read_stdout_impl() { - std::size_t num_bytes_avail = 0; +bool Subprocess::read_pipe_poll(Subprocess::OutputPipe& pipe, int timeout_ms) { + struct pollfd poll_struct = {.fd = pipe.pipe.read_fd, .events = POLLIN, .revents = 0}; - if (stdout_pipe_.read_fd == -1) { - return {}; + // TODO: Create wrapper in linux.hpp + int res = poll(&poll_struct, 1, timeout_ms); + // Syscall error + if (res == -1) { + throw std::logic_error("poll for pipe failed"); + } + // Timeout occured + if (res == 0) { + return false; } - TRYE(linux::ioctl(stdout_pipe_.read_fd, FIONREAD, &num_bytes_avail), SyscallFailure); - - LOG_DEBUG("{} bytes available from stdout_pipe", num_bytes_avail); + read_pipe_nonblock(pipe); - if (num_bytes_avail == 0) { - return {}; - } + return true; +} - std::string res = TRYE(linux::read(stdout_pipe_.read_fd, num_bytes_avail), SyscallFailure); +Subprocess::OutputResult Subprocess::new_output(WhichOutput which) { + OutputResult res; - stdout_buffer_ += res; + // Update res and cursor positions + if ((which & WhichOutput::Stdout) != WhichOutput::None && stdout_.cursor < stdout_.buffer.size()) { + res.stdout_str = stdout_.buffer.substr(stdout_.cursor); + stdout_.cursor = stdout_.buffer.size(); + } + if ((which & WhichOutput::Stderr) != WhichOutput::None && stderr_.cursor < stderr_.buffer.size()) { + res.stderr_str = stderr_.buffer.substr(stderr_.cursor); + stderr_.cursor = stderr_.buffer.size(); + } - return {}; + return res; } -Result Subprocess::send_stdin(std::string_view str) { +Result Subprocess::send_stdin(std::string_view str) const { // TODO: more abstract write wrapper that ensures all bytes were sent TRYE(linux::write(stdin_pipe_.write_fd, str), SyscallFailure); @@ -237,8 +257,9 @@ Result Subprocess::send_stdin(std::string_view str) { } Result Subprocess::create(const std::string& exec, const std::vector& args) { - stdout_pipe_ = TRYE(linux::pipe2(), SyscallFailure); stdin_pipe_ = TRYE(linux::pipe2(), SyscallFailure); + stdout_.pipe = TRYE(linux::pipe2(), SyscallFailure); + stderr_.pipe = TRYE(linux::pipe2(), SyscallFailure); if (!mark_cloexec_all()) { LOG_WARN("Failed to set flags for fds; some fds will likely remain open in child proc"); @@ -264,13 +285,15 @@ Result Subprocess::create(const std::string& exec, const std::vector Subprocess::init_child() { TRYE(linux::dup2(stdin_pipe_.read_fd, STDIN_FILENO), SyscallFailure); - TRYE(linux::dup2(stdout_pipe_.write_fd, STDOUT_FILENO), SyscallFailure); + TRYE(linux::dup2(stdout_.pipe.write_fd, STDOUT_FILENO), SyscallFailure); + TRYE(linux::dup2(stderr_.pipe.write_fd, STDERR_FILENO), SyscallFailure); // Close the pipe ends not being used in the child proc // - read end for stdout - // - write end for stdin + // - write end for stdin and stderr TRYE(linux::close(stdin_pipe_.write_fd), SyscallFailure); - TRYE(linux::close(stdout_pipe_.read_fd), SyscallFailure); + TRYE(linux::close(stdout_.pipe.read_fd), SyscallFailure); + TRYE(linux::close(stderr_.pipe.read_fd), SyscallFailure); namespace fs = std::filesystem; @@ -296,16 +319,20 @@ Result Subprocess::init_child() { Result Subprocess::init_parent() { // Close the pipe ends being used in the parent proc // - write end for stdout - // - read end for stdin + // - read end for stdin and stderr TRYE(linux::close(stdin_pipe_.read_fd), SyscallFailure); - TRYE(linux::close(stdout_pipe_.write_fd), SyscallFailure); + TRYE(linux::close(stdout_.pipe.write_fd), SyscallFailure); + TRYE(linux::close(stderr_.pipe.write_fd), SyscallFailure); // stdin_pipefd_ = stdin_pipe.write_fd; // write end of stdin pipe // stdout_pipefd_ = stdout_pipe.read_fd; // read end of stdout pipe - // Make reading from stdout non-blocking - int pre_flags = TRYE(linux::fcntl(stdout_pipe_.read_fd, F_GETFL), SyscallFailure); + // Make reading from stdout and stderr non-blocking + int pre_flags_stdout = TRYE(linux::fcntl(stdout_.pipe.read_fd, F_GETFL), SyscallFailure); + int pre_flags_stderr = TRYE(linux::fcntl(stderr_.pipe.read_fd, F_GETFL), SyscallFailure); - TRYE(linux::fcntl(stdout_pipe_.read_fd, F_SETFL, pre_flags | O_NONBLOCK), // NOLINT + TRYE(linux::fcntl(stdout_.pipe.read_fd, F_SETFL, pre_flags_stdout | O_NONBLOCK), // NOLINT + SyscallFailure); + TRYE(linux::fcntl(stderr_.pipe.read_fd, F_SETFL, pre_flags_stderr | O_NONBLOCK), // NOLINT SyscallFailure); return {}; diff --git a/tests/test_program.cpp b/tests/test_program.cpp index d27d884..8eebcb7 100644 --- a/tests/test_program.cpp +++ b/tests/test_program.cpp @@ -3,6 +3,7 @@ #include "common/aliases.hpp" #include "common/error_types.hpp" #include "program/program.hpp" +#include "subprocess/subprocess.hpp" #include #include @@ -46,16 +47,18 @@ TEST_CASE("Call sum_and_write function") { asmgrader::Program prog(ASM_TESTS_EXEC, {}); REQUIRE(prog.call_function("sum_and_write", 0, 0)); - REQUIRE(prog.get_subproc().read_stdout() == std::string{"\0\0\0\0\0\0\0\0", 8}); + REQUIRE(prog.get_subproc().read_output(asmgrader::Subprocess::WhichOutput::Stdout).stdout_str == + std::string{"\0\0\0\0\0\0\0\0", 8}); REQUIRE(prog.call_function("sum_and_write", 'a', 5)); // 'a' + 5 = 'f' - REQUIRE(prog.get_subproc().read_stdout() == std::string{"f\0\0\0\0\0\0\0", 8}); + REQUIRE(prog.get_subproc().read_output(asmgrader::Subprocess::WhichOutput::Stdout).stdout_str == + std::string{"f\0\0\0\0\0\0\0", 8}); REQUIRE(prog.call_function("sum_and_write", 0x1010101010101010, 0x1010101010101010)); static_assert(' ' == 0x10 + 0x10, "Somehow not ASCII encoded???"); // 0x10 + 0x10 = 0x20 (space ' ') - REQUIRE(prog.get_subproc().read_stdout() == " "); + REQUIRE(prog.get_subproc().read_output(asmgrader::Subprocess::WhichOutput::Stdout).stdout_str == " "); } TEST_CASE("Test that timeouts are handled properly with timeout_fn") { diff --git a/tests/test_subprocess.cpp b/tests/test_subprocess.cpp index b8fdf7d..22e0086 100644 --- a/tests/test_subprocess.cpp +++ b/tests/test_subprocess.cpp @@ -40,7 +40,7 @@ TEST_CASE("Read /bin/echo stdout") { proc.wait_for_exit(); - REQUIRE(proc.read_stdout() == "Hello world!"); + REQUIRE(proc.read_output(asmgrader::Subprocess::WhichOutput::Stdout).stdout_str == "Hello world!"); } TEST_CASE("Interact with /bin/cat") { @@ -68,7 +68,7 @@ TEST_CASE("Get results of asm program") { REQUIRE(run_res->get_code() == 42); REQUIRE(proc.get_exit_code() == 42); - REQUIRE(proc.read_stdout() == "Hello, from assembly!\n"); + REQUIRE(proc.read_output(asmgrader::Subprocess::WhichOutput::Stdout).stdout_str == "Hello, from assembly!\n"); auto syscall_records = proc.get_tracer().get_records(); From 61f151f19c6885505b053328cf951b49a1fc1bd6 Mon Sep 17 00:00:00 2001 From: Matthew Reese Date: Fri, 24 Oct 2025 15:39:10 -0700 Subject: [PATCH 10/12] add test for reading from captured stderr --- tests/resources/simple_asm_aarch64.s | 42 ++++++++++++++++++++-------- tests/resources/simple_asm_x86_64.s | 21 +++++++++++++- tests/test_program.cpp | 19 +++++++++++++ 3 files changed, 70 insertions(+), 12 deletions(-) diff --git a/tests/resources/simple_asm_aarch64.s b/tests/resources/simple_asm_aarch64.s index 29d1c80..86812cb 100644 --- a/tests/resources/simple_asm_aarch64.s +++ b/tests/resources/simple_asm_aarch64.s @@ -23,24 +23,44 @@ sum: add x0, x0, x1 // x0 += x1 ret -/// sum_and_write +/// write_to /// sums two numbers and writes the result to stdout. Overflow may occur. /// Parameters: -/// x0 (u64) - the first number -/// x1 (u64) - the second number +/// x0 (const char*) - string to write +/// x1 (int) - the fd to write to +/// x2 (size_t) - length of the string /// Result: -/// x0 + x1 written to stdout as 8 bytes. -sum_and_write: - add x0, x0, x1 // x0 += x1 - STR x0, [sp, -8]! +/// length bytes of string written to fd +write_to: + mov x3, x0 # save x0 (str) into x3 mov x8, 64 // SYS_write - mov x0, 1 // fd param = stdout - mov x1, sp // str param = stack addr (sum result) - mov x2, 8 // len param = 8 + mov x0, x1 // fd param = fd + mov x1, x3 // str param = string + // len param [already set by param] svc 0 // SYS_write - add sp, sp, 8 // pop x0 off of the stack + ret + +/// sum_and_write_fd +/// sums two numbers and writes the result to specified fd. Overflow may occur. +/// Parameters: +/// rdi (u64) - the first number +/// rsi (u64) - the second number +/// rdx (int) - the fd to write to +/// Result: +/// rdi + rsi written to fd as 8 bytes. +sum_and_write_fd: + add rsi, rdi # rsi += rdi + push rsi + + mov rax, 1 # SYS_write + mov rdi, rdx # fd + lea rsi, [rsp] # rsi = stack addr - 8 (sum result) + mov rdx, 8 # rdx (len) = 8 + syscall # SYS_write + + pop rsi # pop rsi off the stack ret /// This subroutine will timeout in an infinitely recurring loop diff --git a/tests/resources/simple_asm_x86_64.s b/tests/resources/simple_asm_x86_64.s index 7f3eb61..8473bcd 100644 --- a/tests/resources/simple_asm_x86_64.s +++ b/tests/resources/simple_asm_x86_64.s @@ -13,7 +13,6 @@ _start: mov rdi, 42 # retcode syscall - /// sum /// sums two numbers and returns the result /// Parameters: @@ -46,6 +45,26 @@ sum_and_write: pop rsi # pop rsi off the stack ret +/// write_to +/// sums two numbers and writes the result to specified fd. Overflow may occur. +/// Parameters: +/// rdi (const char*) - string to write +/// rsi (int) - the fd to write to +/// rdx (size_t) - length of the string +/// Result: +/// length bytes of string written to fd +write_to: + mov r10, rdi # save rdi (str) into r10 + + mov rax, 1 # SYS_write + mov rdi, rsi # fd + mov rsi, r10 # rsi = str + mov rdx, rdx # rdx (len) = length + syscall # SYS_write + + ret + + /// This subroutine will timeout in an infinitely recurring loop timeout_fn: jmp timeout_fn diff --git a/tests/test_program.cpp b/tests/test_program.cpp index 8eebcb7..e85d5cb 100644 --- a/tests/test_program.cpp +++ b/tests/test_program.cpp @@ -8,10 +8,13 @@ #include #include +#include + using namespace asmgrader::aliases; using sum = u64(std::uint64_t, std::uint64_t); using sum_and_write = void(u64, std::uint64_t); +using write_to = void(const char*, int, size_t); using timeout_fn = void(); using segfaulting_fn = void(); using exiting_fn = void(u64); @@ -61,6 +64,22 @@ TEST_CASE("Call sum_and_write function") { REQUIRE(prog.get_subproc().read_output(asmgrader::Subprocess::WhichOutput::Stdout).stdout_str == " "); } +TEST_CASE("Call write_to function") { + asmgrader::Program prog(ASM_TESTS_EXEC, {}); + + std::string test_str = "I am a test string\nNOPE\n123897g51%%~"; + + REQUIRE(prog.call_function("write_to", test_str, STDOUT_FILENO, test_str.size())); + REQUIRE(prog.get_subproc().read_output().stdout_str == test_str); + + REQUIRE(prog.call_function("write_to", test_str, STDERR_FILENO, test_str.size())); + REQUIRE(prog.get_subproc().read_output().stderr_str == test_str); + + REQUIRE(prog.call_function("write_to", test_str, 0, test_str.size())); + REQUIRE(prog.get_subproc().read_output().stdout_str == ""); + REQUIRE(prog.get_subproc().read_output().stderr_str == ""); +} + TEST_CASE("Test that timeouts are handled properly with timeout_fn") { asmgrader::Program prog(ASM_TESTS_EXEC, {}); From 90f5ba5d2da750d3412ea2c2bc054d3877c79433 Mon Sep 17 00:00:00 2001 From: Matthew Reese Date: Fri, 24 Oct 2025 15:39:30 -0700 Subject: [PATCH 11/12] add api for reading from stderr to test_context --- include/asmgrader/api/test_context.hpp | 4 ++++ src/api/test_context.cpp | 13 +++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/include/asmgrader/api/test_context.hpp b/include/asmgrader/api/test_context.hpp index d07a5af..260266a 100644 --- a/include/asmgrader/api/test_context.hpp +++ b/include/asmgrader/api/test_context.hpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -81,6 +82,9 @@ class TestContext /// Get all stdout from since the beginning of the test invokation std::string get_full_stdout(); + Subprocess::OutputResult get_output(Subprocess::WhichOutput which = Subprocess::WhichOutput::StdoutAndStderr); + Subprocess::OutputResult get_full_output(Subprocess::WhichOutput which = Subprocess::WhichOutput::StdoutAndStderr); + /// Flushes any reamaining unread data in the stdin buffer /// Returns: number of bytes flushed, or error kind if failure occured std::size_t flush_stdin(); diff --git a/src/api/test_context.cpp b/src/api/test_context.cpp index af73d1d..7404412 100644 --- a/src/api/test_context.cpp +++ b/src/api/test_context.cpp @@ -17,6 +17,7 @@ #include "logging.hpp" #include "program/program.hpp" #include "subprocess/run_result.hpp" +#include "subprocess/subprocess.hpp" #include "subprocess/syscall_record.hpp" #include @@ -83,11 +84,19 @@ std::string_view TestContext::get_name() const { } std::string TestContext::get_stdout() { - return TRY_OR_THROW(prog_.get_subproc().read_stdout(), "failed to read stdout"); + return get_output(Subprocess::WhichOutput::Stdout).stdout_str; } std::string TestContext::get_full_stdout() { - return prog_.get_subproc().get_full_stdout(); + return get_full_output(Subprocess::WhichOutput::Stdout).stdout_str; +} + +Subprocess::OutputResult TestContext::get_output(Subprocess::WhichOutput which) { + return prog_.get_subproc().read_output(which); +} + +Subprocess::OutputResult TestContext::get_full_output(Subprocess::WhichOutput which) { + return prog_.get_subproc().read_full_output(which); } void TestContext::send_stdin(std::string_view input) { From b29c940d9d263676cc23da80bda22bbf6b80d6b1 Mon Sep 17 00:00:00 2001 From: Matthew Reese Date: Fri, 24 Oct 2025 15:57:34 -0700 Subject: [PATCH 12/12] fix aarch64 test asm program --- tests/resources/simple_asm_aarch64.s | 44 ++++++++++++++-------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/tests/resources/simple_asm_aarch64.s b/tests/resources/simple_asm_aarch64.s index 86812cb..12c191d 100644 --- a/tests/resources/simple_asm_aarch64.s +++ b/tests/resources/simple_asm_aarch64.s @@ -23,6 +23,27 @@ sum: add x0, x0, x1 // x0 += x1 ret +/// sum_and_write +/// sums two numbers and writes the result to stdout. Overflow may occur. +/// Parameters: +/// x0 (u64) - the first number +/// x1 (u64) - the second number +/// Result: +/// x0 + x1 written to stdout as 8 bytes. +sum_and_write: + add x0, x0, x1 // x0 += x1 + str x0, [sp, -8]! // save x0 onto the stack + + mov x8, 64 // SYS_write + mov x0, 1 // fd param = stdout + mov x1, sp // str param = stack addr (sum result) + mov x2, 8 // len param = 8 + svc 0 // SYS_write + + add sp, sp, 8 // pop x0 off of the stack + ret + + /// write_to /// sums two numbers and writes the result to stdout. Overflow may occur. /// Parameters: @@ -32,7 +53,7 @@ sum: /// Result: /// length bytes of string written to fd write_to: - mov x3, x0 # save x0 (str) into x3 + mov x3, x0 // save x0 (str) into x3 mov x8, 64 // SYS_write mov x0, x1 // fd param = fd @@ -42,27 +63,6 @@ write_to: ret -/// sum_and_write_fd -/// sums two numbers and writes the result to specified fd. Overflow may occur. -/// Parameters: -/// rdi (u64) - the first number -/// rsi (u64) - the second number -/// rdx (int) - the fd to write to -/// Result: -/// rdi + rsi written to fd as 8 bytes. -sum_and_write_fd: - add rsi, rdi # rsi += rdi - push rsi - - mov rax, 1 # SYS_write - mov rdi, rdx # fd - lea rsi, [rsp] # rsi = stack addr - 8 (sum result) - mov rdx, 8 # rdx (len) = 8 - syscall # SYS_write - - pop rsi # pop rsi off the stack - ret - /// This subroutine will timeout in an infinitely recurring loop timeout_fn: b timeout_fn