Refactoring of gluten_read/write

Refactor includes:
  - use static inline instead of macro
  - return -1 if there is an error and don't exit
  - eval return 0 or -1
  - adjust code and tests

5 files changed, 86 insertions, 60 deletions

diff --git a/common/gluten.h b/common/gluten.h
index d1d3c61..078c6fa 100644
--- a/common/gluten.h
+++ b/common/gluten.h
@@ -12,6 +12,8 @@
 #include <stdbool.h>
 #include <linux/seccomp.h>
 
+#include <stdio.h>
+
 #include "util.h"
 
 extern struct seccomp_data anonymous_seccomp_data;
@@ -26,30 +28,6 @@ extern struct seccomp_data anonymous_seccomp_data;
 	MAX(MAX(MAX(DATA_SIZE, RO_DATA_SIZE), INST_MAX), \
 	    ARRAY_SIZE(anonymous_seccomp_data.args))
 
-#define check_gluten_limits(g, v, size)                                   \
-	do {                                                              \
-		struct gluten_offset off = { v.type, v.offset + (size) }; \
-		if (!is_offset_valid(off))                                \
-			die(" invalid offset: %d", off.offset);           \
-	} while (0)
-
-#define gluten_write(g, dst, src)                               \
-	do {                                                    \
-		void *p = gluten_write_ptr((g), (dst));         \
-		check_gluten_limits((g), (dst), sizeof((src))); \
-		if (p == NULL)                                  \
-			die(" invalid type of offset");         \
-		memcpy(p, &(src), sizeof(src));                 \
-	} while (0)
-
-#define gluten_read(s, g, dst, src, size)                    \
-	do {                                                 \
-		const void *p = gluten_ptr((s), (g), (src)); \
-		check_gluten_limits((g), (src), (size));     \
-		if (p == NULL)                               \
-			die(" invalid type of offset");      \
-		memcpy(&(dst), p, (size));      \
-	} while (0)
 
 enum gluten_offset_type {
 	OFFSET_RO_DATA = 0,
@@ -242,7 +220,7 @@ static inline void *gluten_write_ptr(struct gluten *g,
 #endif
 {
 	if (!is_offset_valid(x))
-		die(" invalid offset: %d", x.offset);
+		return NULL;
 
 	switch (x.type) {
 	case OFFSET_DATA:
@@ -264,7 +242,7 @@ static inline const void *gluten_ptr(const struct seccomp_data *s,
 				     const struct gluten_offset x)
 {
 	if (!is_offset_valid(x))
-		die(" invalid offset: %d", x.offset);
+		return NULL;
 
 	switch (x.type) {
 	case OFFSET_DATA:
@@ -279,5 +257,35 @@ static inline const void *gluten_ptr(const struct seccomp_data *s,
 		return NULL;
 	}
 }
+
+static inline bool check_gluten_limits(struct gluten_offset v, size_t size)
+{
+	struct gluten_offset off = { v.type, v.offset + size };
+	return is_offset_valid(off);
+}
+
+static inline int gluten_write(struct gluten *g, struct gluten_offset dst,
+			       const void *src, size_t size)
+{
+	void *p = gluten_write_ptr(g, dst);
+	if (p == NULL || !check_gluten_limits(dst, size))
+		return -1;
+	memcpy(p, src, size);
+
+	return 0;
+}
+
+static inline int gluten_read(const struct seccomp_data *s, struct gluten *g,
+			      void *dst, const struct gluten_offset src,
+			      size_t size)
+{
+	const void *p = gluten_ptr(s, g, src);
+	if (p == NULL || !check_gluten_limits(src, size))
+		return -1;
+	memcpy(dst, p, size);
+
+	return 0;
+}
+
 #endif
 #endif /* COMMON_GLUTEN_H */
diff --git a/operations.c b/operations.c
index bf03ab8..af86568 100644
--- a/operations.c
+++ b/operations.c
@@ -177,7 +177,10 @@ int op_load(const struct seccomp_notif *req, int notifier, struct gluten *g,
 		ret = -1;
 		goto out;
 	}
-	check_gluten_limits(&g, load->dst, load->size);
+	if (!check_gluten_limits(load->dst, load->size)) {
+		ret = -1;
+		goto out;
+	}
 	if (pread(fd, gluten_write_ptr(g, load->dst), load->size, *src) < 0) {
 		perror("pread");
 		return -1;
@@ -233,7 +236,7 @@ int op_call(const struct seccomp_notif *req, int notifier, struct gluten *g,
 	 * reference
 	 */
 	if (op->has_ret)
-		gluten_write(g, op->ret, c.ret);
+		return gluten_write(g, op->ret, &c.ret, sizeof(c.ret));
 
 	return 0;
 }
@@ -264,7 +267,8 @@ int op_return(const struct seccomp_notif *req, int notifier, struct gluten *g,
 	resp.flags = 0;
 	resp.error = 0;
 
-	gluten_read(NULL, g, resp.val, op->val, sizeof(resp.val));
+	if (gluten_read(&req->data, g, &resp.val, op->val, sizeof(resp.val)) == -1)
+		return -1;
 
 	if (send_target(&resp, notifier) == -1)
 		return -1;
@@ -300,8 +304,10 @@ static int do_inject(const struct seccomp_notif *req, int notifier,
 	resp.newfd_flags = 0;
 	resp.id = req->id;
 
-	gluten_read(NULL, g, resp.newfd, op->new_fd, sizeof(resp.newfd));
-	gluten_read(NULL, g, resp.srcfd, op->old_fd, sizeof(resp.srcfd));
+	if(gluten_read(NULL, g, &resp.newfd, op->new_fd, sizeof(resp.newfd)) == -1)
+		return -1;
+	if(gluten_read(NULL, g, &resp.srcfd, op->old_fd, sizeof(resp.srcfd)) == -1)
+		return -1;
 
 	if (atomic)
 		resp.flags |= SECCOMP_ADDFD_FLAG_SEND;
@@ -351,8 +357,10 @@ int op_resolve_fd(const struct seccomp_notif *req, int notifier,
 	(void)notifier;
 
 
-	gluten_read(NULL, g, path, op->path, sizeof(op->path_size));
-	gluten_read(NULL, g, fd, op->fd, sizeof(fd));
+	if(gluten_read(NULL, g, &path, op->path, sizeof(op->path_size)) == -1)
+		return -1;
+	if(gluten_read(NULL, 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) {
@@ -366,12 +374,12 @@ int op_resolve_fd(const struct seccomp_notif *req, int notifier,
 	return 0;
 }
 
-void eval(struct gluten *g, struct op *ops, const struct seccomp_notif *req,
+int eval(struct gluten *g, struct op *ops, const struct seccomp_notif *req,
 	  int notifier)
 {
 	struct op *op = ops;
 
-	while (op->type != OP_END && op != NULL) {
+	while (op->type != OP_END) {
 		switch (op->type) {
 			HANDLE_OP(OP_CALL, op_call, call);
 			HANDLE_OP(OP_BLOCK, op_block, block);
@@ -384,7 +392,7 @@ void eval(struct gluten *g, struct op *ops, const struct seccomp_notif *req,
 			HANDLE_OP(OP_RESOLVEDFD, op_resolve_fd, resfd);
 		default:
 			fprintf(stderr, "unknown operation %d \n", op->type);
-			return;
 		}
 	}
+	return 0;
 }
diff --git a/operations.h b/operations.h
index 011369a..ccc42c2 100644
--- a/operations.h
+++ b/operations.h
@@ -21,7 +21,7 @@
 			if (res == 0)                                   \
 				(op)++;                                 \
 			else if (res == -1)                             \
-				(op) = NULL;                            \
+				return -1;                            \
 			else                                            \
 				(op) += res;                            \
 		} while (0);                                            \
@@ -35,7 +35,7 @@ struct arg_clone {
 };
 
 int do_call(struct arg_clone *c);
-void eval(struct gluten *g, struct op *ops, const struct seccomp_notif *req,
+int eval(struct gluten *g, struct op *ops, const struct seccomp_notif *req,
 	  int notifier);
 int op_call(const struct seccomp_notif *req, int notifier, struct gluten *g,
 	    struct op_call *op);
diff --git a/tests/unit/test_errors.c b/tests/unit/test_errors.c
index d00d42e..ca6fcb0 100644
--- a/tests/unit/test_errors.c
+++ b/tests/unit/test_errors.c
@@ -1,4 +1,3 @@
-#include <stdio.h>
 #include <stdlib.h>
 #include <sys/mman.h>
 #include <sys/syscall.h>
@@ -31,6 +30,7 @@ START_TEST(test_bound_check)
 {
 	struct op ops[] = {
 		{ OP_RETURN, { { 0 } } },
+		{ OP_END, { { 0 } } },
 	};
 	ops[0].op.ret.val.offset = test_max_size_data[_i].offset;
 	ops[0].op.ret.val.type = test_max_size_data[_i].type;
@@ -45,9 +45,10 @@ START_TEST(test_write_op_return)
 		  { .call = { .nr = __NR_getpid,
 			      .has_ret = true,
 			      .ret = { OFFSET_DATA, DATA_SIZE - 1 } } } },
+		{ OP_END, { { 0 } } },
 	};
 
-	eval(&gluten, ops, &req, notifyfd);
+	ck_assert_int_eq(eval(&gluten, ops, &req, notifyfd), -1);
 }
 
 START_TEST(test_write_op_load)
@@ -58,20 +59,28 @@ START_TEST(test_write_op_load)
 		  { .load = { { OFFSET_SECCOMP_DATA, 1 },
 			      { OFFSET_DATA, DATA_SIZE - 1 },
 			      sizeof(a) } } },
+		{ OP_END, { { 0 } } },
 	};
 
-	eval(&gluten, ops, &req, notifyfd);
+	ck_assert_int_eq(eval(&gluten, ops, &req, notifyfd), -1);
 }
 
+struct gluten_offset test_max_size_read_data[] = {
+	{ OFFSET_DATA, DATA_SIZE },
+	{ OFFSET_RO_DATA, RO_DATA_SIZE },
+	{ OFFSET_SECCOMP_DATA, 6 },
+};
+
 START_TEST(test_read_op_return)
 {
 	struct op ops[] = {
 		{ OP_RETURN, { { 0 } } },
+		{ OP_END, { { 0 } } },
 	};
-	ops[0].op.ret.val.offset = test_max_size_data[_i].offset - 1;
-	ops[0].op.ret.val.type = test_max_size_data[_i].type;
+	ops[0].op.ret.val.offset = test_max_size_read_data[_i].offset - 1;
+	ops[0].op.ret.val.type = test_max_size_read_data[_i].type;
 
-	eval(&gluten, ops, &req, notifyfd);
+	ck_assert_int_eq(eval(&gluten, ops, &req, notifyfd), -1);
 }
 
 Suite *error_suite(void)
@@ -82,21 +91,22 @@ Suite *error_suite(void)
 	s = suite_create("Error handling");
 
 	bounds = tcase_create("bound checks");
-	tcase_add_loop_exit_test(bounds, test_bound_check, EXIT_FAILURE, 0,
-				 sizeof(test_max_size_data) /
-					 sizeof(test_max_size_data[0]));
+	tcase_add_loop_test(bounds, test_bound_check, 0,
+			    sizeof(test_max_size_data) /
+				    sizeof(test_max_size_data[0]));
 	suite_add_tcase(s, bounds);
 
 	gwrite = tcase_create("write gluten");
 	tcase_add_checked_fixture(gwrite, setup_error_check, teardown);
-	tcase_add_exit_test(gwrite, test_write_op_return, EXIT_FAILURE);
-	tcase_add_exit_test(gwrite, test_write_op_load, EXIT_FAILURE);
+	tcase_add_test(gwrite, test_write_op_return);
+	tcase_add_test(gwrite, test_write_op_load);
 	suite_add_tcase(s, gwrite);
 
 	gread = tcase_create("read gluten");
-	tcase_add_loop_exit_test(gread, test_read_op_return, EXIT_FAILURE, 0,
-				 sizeof(test_max_size_data) /
-					 sizeof(test_max_size_data[0]));
+	tcase_add_checked_fixture(gread, setup_error_check, teardown);
+	tcase_add_loop_test(gread, test_read_op_return, 0,
+			    sizeof(test_max_size_read_data) /
+				    sizeof(test_max_size_read_data[0]));
 	suite_add_tcase(s, gread);
 
 	return s;
diff --git a/tests/unit/test_operations.c b/tests/unit/test_operations.c
index 9c3cf94..0e22754 100644
--- a/tests/unit/test_operations.c
+++ b/tests/unit/test_operations.c
@@ -136,7 +136,7 @@ START_TEST(test_op_call)
 		{ 0 },
 	};
 
-	eval(&gluten, operations, &req, notifyfd);
+	ck_assert_int_eq(eval(&gluten, operations, &req, notifyfd), 0);
 	check_target_result(1, 0, true);
 }
 END_TEST
@@ -153,7 +153,7 @@ START_TEST(test_op_call_ret)
 		{ 0 },
 	};
 
-	eval(&gluten, operations, &req, notifyfd);
+	ck_assert_int_eq(eval(&gluten, operations, &req, notifyfd), 0);
 	check_target_result(1, 0, true);
 	ck_read_gluten(gluten, operations[0].op.call.ret, r);
 	ck_assert(r == getpid());
@@ -218,7 +218,7 @@ START_TEST(test_op_load)
 	int v = 2;
 
 	ck_write_gluten(gluten, operations[1].op.ret.val, v);
-	eval(&gluten, operations, &req, notifyfd);
+	ck_assert_int_eq(eval(&gluten, operations, &req, notifyfd), 0);
 	check_target_result(v, 0, false);
 
 	ck_read_gluten(gluten, operations[0].op.load.dst, addr);
@@ -246,7 +246,7 @@ static void test_op_cmp_int(int a, int b, enum op_cmp_type cmp)
 	ck_write_gluten(gluten, operations[0].op.cmp.x, a);
 	ck_write_gluten(gluten, operations[0].op.cmp.y, b);
 
-	eval(&gluten, operations, &req, notifyfd);
+	ck_assert_int_eq(eval(&gluten, operations, &req, notifyfd), 0);
 	check_target_result_nonegative();
 }
 
@@ -306,7 +306,7 @@ START_TEST(test_op_cmp_string_eq)
 	ck_write_gluten(gluten, operations[0].op.cmp.x, s1);
 	ck_write_gluten(gluten, operations[0].op.cmp.y, s2);
 
-	eval(&gluten, operations, &req, notifyfd);
+	ck_assert_int_eq(eval(&gluten, operations, &req, notifyfd), 0);
 	check_target_result_nonegative();
 }
 END_TEST
@@ -332,7 +332,7 @@ START_TEST(test_op_cmp_string_false)
 	ck_write_gluten(gluten, operations[0].op.cmp.x, s1);
 	ck_write_gluten(gluten, operations[0].op.cmp.y, s2);
 
-	eval(&gluten, operations, &req, notifyfd);
+	ck_assert_int_eq(eval(&gluten, operations, &req, notifyfd), 0);
 	check_target_result_nonegative();
 }
 END_TEST
@@ -355,7 +355,7 @@ START_TEST(test_op_resolvedfd_eq)
 	ck_write_gluten(gluten, operations[0].op.resfd.fd, at->fd);
 	ck_write_gluten(gluten, operations[0].op.resfd.path, path);
 
-	eval(&gluten, operations, &req, notifyfd);
+	ck_assert_int_eq(eval(&gluten, operations, &req, notifyfd), 0);
 	check_target_result(-1, 1, false);
 }
 END_TEST
@@ -379,7 +379,7 @@ START_TEST(test_op_resolvedfd_neq)
 	ck_write_gluten(gluten, operations[0].op.resfd.fd, at->fd);
 	ck_write_gluten(gluten, operations[0].op.resfd.path, path2);
 
-	eval(&gluten, operations, &req, notifyfd);
+	ck_assert_int_eq(eval(&gluten, operations, &req, notifyfd), 0);
 	check_target_result(-1, 1, false);
 }
 END_TEST