From 94df2efe2d2221bf0c4d77510142c95283d76f2b Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Wed, 10 May 2023 15:19:49 +0200 Subject: Clean-up error message and test Refactoring error messages: - standardize error messages and functions - return on error instead of exit - test error when target doesn't exist - include ability to capture stderr and stdout in the tests --- common/util.h | 12 +++++++ operations.c | 93 +++++++++++++++++++----------------------------- tests/unit/test_errors.c | 50 ++++++++++++++++++++++++++ tests/unit/testutil.h | 7 +++- tests/unit/util.c | 18 ++++++++++ 5 files changed, 122 insertions(+), 58 deletions(-) diff --git a/common/util.h b/common/util.h index 102b55b..ad2192b 100644 --- a/common/util.h +++ b/common/util.h @@ -8,6 +8,7 @@ #include #include +#include #define BIT(n) (1UL << (n)) @@ -33,6 +34,17 @@ void debug(const char *format, ...); exit(EXIT_FAILURE); \ } while (0) +#define ret_err(e, ...) \ + do { \ + if (errno == 0) { \ + err(__VA_ARGS__); \ + } else { \ + fprintf(stderr, __VA_ARGS__); \ + fprintf(stderr, ": %s\n", strerror(errno)); \ + } \ + return (e); \ + } while (0) + /** * From the Linux kernel, include/linux/log2.h: * diff --git a/operations.c b/operations.c index d6a9245..47a9a9f 100644 --- a/operations.c +++ b/operations.c @@ -11,7 +11,6 @@ #include #include #include -#include #include #include #include @@ -24,6 +23,7 @@ #include #include "common/gluten.h" +#include "common/util.h" #include "operations.h" static bool is_cookie_valid(int notifyFd, uint64_t id) @@ -33,34 +33,22 @@ static bool is_cookie_valid(int notifyFd, uint64_t id) static int send_target(const struct seccomp_notif_resp *resp, int notifier) { - if (!is_cookie_valid(notifier, resp->id)) { - fprintf(stderr, - "the response id isn't valid\ncheck if the targets has already terminated\n"); - exit(0); - } - if (ioctl(notifier, SECCOMP_IOCTL_NOTIF_SEND, resp) < 0) { - if (errno != EINPROGRESS) { - perror("sending the response"); - return -1; - } - } + if (!is_cookie_valid(notifier, resp->id)) + ret_err(-1, "the response id isn't valid"); + if (ioctl(notifier, SECCOMP_IOCTL_NOTIF_SEND, resp) < 0) + if (errno != EINPROGRESS) + ret_err(-1, "sending the response"); return 0; } static int send_inject_target(const struct seccomp_notif_addfd *resp, int notifier) { - if (!is_cookie_valid(notifier, resp->id)) { - fprintf(stderr, - "the response id isn't valid\ncheck if the targets has already terminated\n"); - return -1; - } - if (ioctl(notifier, SECCOMP_IOCTL_NOTIF_ADDFD, resp) < 0) { - if (errno != EINPROGRESS) { - perror("sending the response"); - return -1; - } - } + if (!is_cookie_valid(notifier, resp->id)) + ret_err(-1, "the response id isn't valid"); + if (ioctl(notifier, SECCOMP_IOCTL_NOTIF_ADDFD, resp) < 0) + if (errno != EINPROGRESS) + ret_err(-1, "sending the response"); return 0; } @@ -92,7 +80,7 @@ static void proc_ns_name(unsigned i, char *ns) snprintf(ns, PATH_MAX + 1, "time"); break; default: - fprintf(stderr, "unrecognized namespace index %d\n", i); + err("unrecognized namespace index %d\n", i); } } @@ -123,16 +111,11 @@ static int set_namespaces(const struct op_call *a, int tpid) break; } - if ((fd = open(path, O_CLOEXEC)) < 0) { - fprintf(stderr, "open for file %s: %s", path, - strerror(errno)); - return -1; - } + if ((fd = open(path, O_CLOEXEC)) < 0) + ret_err(-1, "open for file %s", path); - if (setns(fd, 0) != 0) { - perror("setns"); - return -1; - } + if (setns(fd, 0) != 0) + ret_err(-1, "setns"); } return 0; } @@ -164,16 +147,14 @@ int op_load(const struct seccomp_notif *req, int notifier, struct gluten *g, int fd, ret = 0; snprintf(path, sizeof(path), "/proc/%d/mem", req->pid); - if ((fd = open(path, O_RDONLY | O_CLOEXEC)) < 0) { - perror("open mem"); - return -1; - } + if ((fd = open(path, O_RDONLY | O_CLOEXEC)) < 0) + ret_err(-1, "error opening mem for %d", req->pid); /* * Avoid the TOCTOU and check if the read mappings are still valid */ if (!is_cookie_valid(notifier, req->id)) { - fprintf(stderr, "the seccomp request isn't valid anymore\n"); + err("the seccomp request isn't valid anymore"); ret = -1; goto out; } @@ -182,8 +163,9 @@ int op_load(const struct seccomp_notif *req, int notifier, struct gluten *g, goto out; } if (pread(fd, gluten_write_ptr(g, load->dst), load->size, *src) < 0) { - perror("pread"); - return -1; + err("pread"); + ret = -1; + goto out; } out: @@ -199,10 +181,8 @@ int do_call(struct arg_clone *c) /* Create a process that will be moved to the namespace */ child = clone(execute_syscall, stack + sizeof(stack), CLONE_FILES | CLONE_VM | SIGCHLD, (void *)c); - if (child == -1) { - perror("clone"); - return -1; - } + if (child == -1) + ret_err(-1, "clone"); wait(NULL); return 0; } @@ -267,7 +247,8 @@ int op_return(const struct seccomp_notif *req, int notifier, struct gluten *g, resp.flags = 0; resp.error = 0; - if (gluten_read(&req->data, g, &resp.val, op->val, sizeof(resp.val)) == -1) + if (gluten_read(&req->data, g, &resp.val, op->val, sizeof(resp.val)) == + -1) return -1; if (send_target(&resp, notifier) == -1) @@ -304,9 +285,11 @@ static int do_inject(const struct seccomp_notif *req, int notifier, resp.newfd_flags = 0; resp.id = req->id; - if(gluten_read(&req->data, g, &resp.newfd, op->new_fd, sizeof(resp.newfd)) == -1) + if (gluten_read(&req->data, g, &resp.newfd, op->new_fd, + sizeof(resp.newfd)) == -1) return -1; - if(gluten_read(&req->data, g, &resp.srcfd, op->old_fd, sizeof(resp.srcfd)) == -1) + if (gluten_read(&req->data, g, &resp.srcfd, op->old_fd, + sizeof(resp.srcfd)) == -1) return -1; if (atomic) @@ -363,18 +346,14 @@ int op_resolve_fd(const struct seccomp_notif *req, int notifier, (void)notifier; - - if(gluten_read(NULL, g, &path, op->path, sizeof(op->path_size)) == -1) + if (gluten_read(NULL, g, &path, op->path, sizeof(op->path_size)) == -1) return -1; - if(gluten_read(&req->data, g, &fd, op->fd, sizeof(fd)) == -1) + if (gluten_read(&req->data, g, &fd, op->fd, sizeof(fd)) == -1) return -1; snprintf(fdpath, PATH_MAX, "/proc/%d/fd/%d", req->pid, fd); - if ((nbytes = readlink(fdpath, buf, op->path_size)) < 0) { - fprintf(stderr, "error reading %s\n", fdpath); - perror("readlink"); - return -1; - } + if ((nbytes = readlink(fdpath, buf, op->path_size)) < 0) + ret_err(-1, "error reading %s", fdpath); if (strcmp(path, buf) == 0) return op->jmp; @@ -382,7 +361,7 @@ int op_resolve_fd(const struct seccomp_notif *req, int notifier, } int eval(struct gluten *g, struct op *ops, const struct seccomp_notif *req, - int notifier) + int notifier) { struct op *op = ops; @@ -398,7 +377,7 @@ int eval(struct gluten *g, struct op *ops, const struct seccomp_notif *req, HANDLE_OP(OP_CMP, op_cmp, cmp); HANDLE_OP(OP_RESOLVEDFD, op_resolve_fd, resfd); default: - fprintf(stderr, "unknown operation %d \n", op->type); + ret_err(-1, "unknown operation %d", op->type); } } return 0; diff --git a/tests/unit/test_errors.c b/tests/unit/test_errors.c index 06bae12..51fc3db 100644 --- a/tests/unit/test_errors.c +++ b/tests/unit/test_errors.c @@ -19,6 +19,11 @@ static void setup_error_check() setup(); } +static void setup_stderr() +{ + ck_stderr(); +} + struct gluten_offset test_max_size_data[] = { { OFFSET_DATA, DATA_SIZE }, { OFFSET_RO_DATA, RO_DATA_SIZE }, @@ -105,11 +110,50 @@ START_TEST(test_op_cmp) ck_assert_int_eq(eval(&gluten, ops, &req, notifyfd), -1); } +static struct ttargetnoexisting_data { + struct op op; + char err_msg[BUFSIZ]; +}; + +struct ttargetnoexisting_data test_target_noexisting_data[] = { + { { OP_CONT, { { 0 } } }, "the response id isn't valid" }, + { { OP_BLOCK, { { 0 } } }, "the response id isn't valid" }, + { { OP_RETURN, { { 0 } } }, "the response id isn't valid" }, + { { OP_INJECT, + { .inject = { { OFFSET_DATA, 0 }, { OFFSET_DATA, 0 } } } }, + "the response id isn't valid" }, + { { OP_INJECT_A, + { .inject = { { OFFSET_DATA, 0 }, { OFFSET_DATA, 0 } } } }, + "the response id isn't valid" }, + { { OP_CALL, { .call = { __NR_getpid, false } } }, + "the response id isn't valid" }, + { { OP_LOAD, + { .load = { { OFFSET_SECCOMP_DATA, 1 }, { OFFSET_DATA, 0 }, 0 } } }, + "error opening mem for" }, + { { OP_RESOLVEDFD, + { .resfd = { { OFFSET_SECCOMP_DATA, 1 }, + { OFFSET_DATA, 0 }, + 0, + 0 } } }, + "error reading /proc" }, +}; + +START_TEST(test_target_noexisting) +{ + struct op ops[2]; + + ops[0] = test_target_noexisting_data[_i].op; + ops[1].type = OP_END; + + ck_assert_int_eq(eval(&gluten, ops, &req, notifyfd), -1); + ck_error_msg(test_target_noexisting_data[_i].err_msg); +} Suite *error_suite(void) { Suite *s; TCase *bounds, *gwrite, *gread, *gcmp; + TCase *tnotexist; s = suite_create("Error handling"); @@ -138,6 +182,12 @@ Suite *error_suite(void) sizeof(test_cmp_data) / sizeof(test_cmp_data[0])); suite_add_tcase(s, gcmp); + tnotexist = tcase_create("target not existing"); + tcase_add_checked_fixture(tnotexist, setup_stderr, NULL); + tcase_add_loop_test(tnotexist, test_target_noexisting, 0, + ARRAY_SIZE(test_target_noexisting_data)); + suite_add_tcase(s, tnotexist); + return s; } diff --git a/tests/unit/testutil.h b/tests/unit/testutil.h index ec881c7..861efb0 100644 --- a/tests/unit/testutil.h +++ b/tests/unit/testutil.h @@ -1,6 +1,6 @@ #ifndef TESTUTIL_H #define TESTUTIL_H - +#include #include #include #include @@ -67,6 +67,8 @@ extern struct args_target *at; extern int pipefd[2]; extern pid_t pid; extern char path[PATH_MAX]; +extern char stderr_buff[BUFSIZ]; +extern char stdout_buff[BUFSIZ]; extern struct gluten gluten; @@ -85,4 +87,7 @@ void continue_target(); void mock_syscall_target(); void set_args_no_check(struct args_target *at); void check_target_result_nonegative(); +void ck_error_msg(char *s); +void ck_stderr(); +void ck_stdout(); #endif /* TESTUTIL_H */ diff --git a/tests/unit/util.c b/tests/unit/util.c index 45171f2..5c36c54 100644 --- a/tests/unit/util.c +++ b/tests/unit/util.c @@ -30,6 +30,8 @@ int pipefd[2]; pid_t pid; char path[PATH_MAX] = "/tmp/test-seitan"; struct gluten gluten; +char stderr_buff[BUFSIZ]; +char stdout_buff[BUFSIZ]; int install_notification_filter(struct args_target *at) { @@ -231,3 +233,19 @@ void teardown() munmap(at, sizeof(struct args_target)); unlink(path); } + +void ck_stderr() +{ + setbuf(stderr, stderr_buff); +} + +void ck_stdout() +{ + setbuf(stdout, stdout_buff); +} + +void ck_error_msg(char *s) +{ + ck_assert_msg(strstr(stderr_buff, s) != NULL, "err=\"%s\" doesn't contain \"%s\" ", + stderr_buff, s); +} -- cgit v1.2.3