fastrpc: add iobuffer unit test

Despite the modularity of the FastRPC implementation, there are no unit
tests. This makes it unpredictable when the implementation encounters an
unlikely scenario, such as when reading uninitialized memory that is
non-zero, or when allocating zero bytes of memory. Add a unit test for
the I/O buffer encoder and decoder so there can be more confidence in
its expected operation.

This does not test edge cases in the malloc() function, such as
allocating zero bytes or failures.

README.md

@@ -48,8 +48,7 @@ with your terminal emulator:
 You can send 3 messages: lookup, attr, and watch. The lookup command returns
 SUIDs for the passed data type. The attr command returns attributes for the
 sensor. Finally, the watch command tells the sensor core to send events from a
-sensor whenever the sensor's value changes. The watch command is only valid if
-the stream type is 1 or 2.
+sensor whenever the sensor's value changes.
 
 An EOF (normally Ctrl+D) is enough to exit the shell unless the sensor core
 died.

fastrpc/hexagonrpcd/apps_std.c

@@ -172,12 +172,6 @@ static uint32_t apps_std_fopen_with_env(void *data,
 		return AEE_EUNSUPPORTED;
 	}
 
-	if (ctx->rootfd < 0) {
-		fprintf(stderr, "Could not open virtual root directory: %s\n",
-				strerror(-ctx->rootfd));
-		return AEE_EFAILED;
-	}
-
 	if (!strcmp(inbufs[1].p, "ADSP_LIBRARY_PATH")) {
 		dirfd = ctx->adsp_library_dirfd;
 	} else if (!strcmp(inbufs[1].p, "ADSP_AVS_CFG_PATH")) {
@@ -225,12 +219,6 @@ static uint32_t apps_std_opendir(void *data,
 	if (((const char *) inbufs[1].p)[inbufs[1].s - 1] != 0)
 		return AEE_EBADPARM;
 
-	if (ctx->rootfd < 0) {
-		fprintf(stderr, "Could not open virtual root directory: %s\n",
-				strerror(-ctx->rootfd));
-		return AEE_EFAILED;
-	}
-
 	ret = hexagonfs_openat(ctx->fds, ctx->rootfd, ctx->rootfd, inbufs[1].p);
 	if (ret < 0) {
 		fprintf(stderr, "Could not open %s: %s\n",
@@ -302,7 +290,6 @@ static uint32_t apps_std_stat(void *data,
 			      struct fastrpc_io_buffer *outbufs)
 {
 	struct apps_std_ctx *ctx = data;
-	char *pathname;
 	struct {
 		uint64_t tsz; // Unknown purpose
 		uint64_t dev;
@@ -318,31 +305,25 @@ static uint32_t apps_std_stat(void *data,
 		int64_t ctime;
 		int64_t ctimensec;
 	} *first_out = outbufs[0].p;
+	const char *pathname = inbufs[1].p;
 	struct stat stats;
 	int fd, ret;
 
-	pathname = malloc(inbufs[1].s + 1);
-	memcpy(pathname, inbufs[1].p, inbufs[1].s);
-	pathname[inbufs[1].s] = 0;
-
-	if (ctx->rootfd < 0) {
-		fprintf(stderr, "Could not open virtual root directory: %s\n",
-				strerror(-ctx->rootfd));
-		return AEE_EFAILED;
-	}
+	if (((const char *) inbufs[1].p)[inbufs[1].s - 1] != 0)
+		return AEE_EBADPARM;
 
 	fd = hexagonfs_openat(ctx->fds, ctx->rootfd, ctx->rootfd, pathname);
 	if (fd < 0) {
 		fprintf(stderr, "Could not open %s: %s\n",
 				pathname, strerror(-fd));
-		goto err_free_pathname;
+		return AEE_EFAILED;
 	}
 
 	ret = hexagonfs_fstat(ctx->fds, fd, &stats);
 	if (ret) {
 		fprintf(stderr, "Could not stat %s: %s\n",
 				pathname, strerror(-fd));
-		goto err_free_pathname;
+		return AEE_EFAILED;
 	}
 
 	hexagonfs_close(ctx->fds, fd);
@@ -366,13 +347,7 @@ static uint32_t apps_std_stat(void *data,
 	first_out->ctime = (int64_t) stats.st_ctim.tv_nsec;
 	first_out->ctimensec = (int64_t) stats.st_ctim.tv_nsec;
 
-	free(pathname);
-
 	return 0;
-
-err_free_pathname:
-	free(pathname);
-	return AEE_EFAILED;
 }
 
 struct fastrpc_interface *fastrpc_apps_std_init(struct hexagonfs_dirent *root)
@@ -386,28 +361,30 @@ struct fastrpc_interface *fastrpc_apps_std_init(struct hexagonfs_dirent *root)
 
 	ctx = calloc(1, sizeof(struct apps_std_ctx));
 	if (ctx == NULL)
-		goto err;
+		goto err_free_iface;
 
 	memcpy(iface, &apps_std_interface, sizeof(struct fastrpc_interface));
 
 	ctx->rootfd = hexagonfs_open_root(ctx->fds, root);
+	if (ctx->rootfd < 0)
+		goto err_free_ctx;
 
-	if (ctx->rootfd >= 0) {
-		ctx->adsp_avs_cfg_dirfd = hexagonfs_openat(ctx->fds,
-							   ctx->rootfd,
-							   ctx->rootfd,
-							   "/vendor/etc/acdbdata/");
-		ctx->adsp_library_dirfd = hexagonfs_openat(ctx->fds,
-							   ctx->rootfd,
-							   ctx->rootfd,
-							   "/usr/lib/qcom/adsp/");
-	}
+	ctx->adsp_avs_cfg_dirfd = hexagonfs_openat(ctx->fds,
+						   ctx->rootfd,
+						   ctx->rootfd,
+						   "/vendor/etc/acdbdata/");
+	ctx->adsp_library_dirfd = hexagonfs_openat(ctx->fds,
+						   ctx->rootfd,
+						   ctx->rootfd,
+						   "/usr/lib/qcom/adsp/");
 
 	iface->data = ctx;
 
 	return iface;
 
-err:
+err_free_ctx:
+	free(ctx);
+err_free_iface:
 	free(iface);
 
 	return NULL;

fastrpc/hexagonrpcd/hexagonfs.c

@@ -69,8 +69,11 @@ static struct hexagonfs_fd *pop_dir(struct hexagonfs_fd *dir,
 
 	if (dir != root && dir->up != NULL) {
 		up = dir->up;
-		dir->ops->close(dir->data);
-		free(dir);
+
+		if (!dir->is_assigned) {
+			dir->ops->close(dir->data);
+			free(dir);
+		}
 	} else {
 		up = dir;
 	}

fastrpc/hexagonrpcd/iobuffer.c

@@ -34,7 +34,7 @@ static size_t consume_size(struct fastrpc_decoder_context *ctx,
 {
 	size_t segment;
 
-	segment = MIN(len, (size_t) 4 - ctx->size_off);
+	segment = MIN(len, 4 - ctx->size_off);
 	memcpy(&((char *) &ctx->size)[ctx->size_off], buf, segment);
 	ctx->size_off = (ctx->size_off + segment) % 4;
 	ctx->align = (ctx->align + segment) & 0x7;
@@ -42,12 +42,22 @@ static size_t consume_size(struct fastrpc_decoder_context *ctx,
 	return segment;
 }
 
-static void try_populate_inbuf(struct fastrpc_decoder_context *ctx)
+static int try_populate_inbuf(struct fastrpc_decoder_context *ctx)
 {
-	if (!ctx->size_off) {
-		ctx->inbufs[ctx->idx].s = ctx->size;
-		ctx->inbufs[ctx->idx].p = malloc(ctx->size);
-	}
+	void *buf;
+
+	// If we still need to process the size, don't do anything yet.
+	if (ctx->size_off)
+		return 0;
+
+	buf = malloc(ctx->size);
+	if (buf == NULL)
+		return -1;
+
+	ctx->inbufs[ctx->idx].s = ctx->size;
+	ctx->inbufs[ctx->idx].p = buf;
+
+	return 0;
 }
 
 static size_t consume_alignment(struct fastrpc_decoder_context *ctx, size_t len)
@@ -55,7 +65,7 @@ static size_t consume_alignment(struct fastrpc_decoder_context *ctx, size_t len)
 	size_t segment = 0;
 
 	if (ctx->align) {
-		segment = MIN(len, (size_t) 8 - ctx->align);
+		segment = MIN(len, 8 - ctx->align);
 		ctx->align = (ctx->align + segment) & 0x7;
 	}
 
@@ -130,7 +140,7 @@ struct fastrpc_decoder_context *inbuf_decode_start(uint32_t sc)
 	ctx->align = 0;
 
 	ctx->inbufs = malloc(sizeof(*ctx->inbufs) * ctx->n_inbufs);
-	if (ctx->inbufs == NULL)
+	if (ctx->inbufs == NULL && ctx->n_inbufs != 0)
 		goto err;
 
 	return ctx;
@@ -154,20 +164,27 @@ int inbuf_decode_is_complete(struct fastrpc_decoder_context *ctx)
 	return ctx->idx >= ctx->n_inbufs;
 }
 
-void inbuf_decode(struct fastrpc_decoder_context *ctx, size_t len, const void *src)
+int inbuf_decode(struct fastrpc_decoder_context *ctx, size_t len, const void *src)
 {
 	const char *buf = src;
 	size_t off = 0;
+	int ret = 0;
 
 	while (off < len && ctx->idx < ctx->n_inbufs) {
 		if (!ctx->size || ctx->size_off) {
 			off += consume_size(ctx, len - off, &buf[off]);
-			try_populate_inbuf(ctx);
-		} else {
+			ret = try_populate_inbuf(ctx);
+		} else if (ctx->align && !ctx->buf_off) {
 			off += consume_alignment(ctx, len - off);
+		} else {
 			off += consume_buf(ctx, len - off, &buf[off]);
 		}
+
+		if (ret == -1)
+			return ret;
 	}
+
+	return 0;
 }
 
 size_t outbufs_calculate_size(size_t n_outbufs, const struct fastrpc_io_buffer *outbufs)

fastrpc/hexagonrpcd/iobuffer.h

@@ -22,23 +22,25 @@
 #ifndef IOBUFFER_H
 #define IOBUFFER_H
 
+#include <stdint.h>
 #include <stddef.h>
 #include <sys/types.h>
 
 struct fastrpc_io_buffer {
-	size_t s;
+	uint32_t s;
 	void *p;
 };
 
 struct fastrpc_decoder_context {
 	struct fastrpc_io_buffer *inbufs;
-	int idx;
-	size_t size;
-	int n_inbufs;
+	unsigned int n_inbufs;
+	unsigned int idx;
 
-	off_t size_off;
-	off_t buf_off;
-	off_t align;
+	uint32_t size;
+	uint32_t buf_off;
+
+	unsigned int size_off;
+	unsigned int align;
 };
 
 void iobuf_free(size_t n_iobufs, struct fastrpc_io_buffer *iobufs);
@@ -46,7 +48,7 @@ void iobuf_free(size_t n_iobufs, struct fastrpc_io_buffer *iobufs);
 struct fastrpc_decoder_context *inbuf_decode_start(uint32_t sc);
 struct fastrpc_io_buffer *inbuf_decode_finish(struct fastrpc_decoder_context *ctx);
 int inbuf_decode_is_complete(struct fastrpc_decoder_context *ctx);
-void inbuf_decode(struct fastrpc_decoder_context *ctx, size_t len, const void *src);
+int inbuf_decode(struct fastrpc_decoder_context *ctx, size_t len, const void *src);
 
 size_t outbufs_calculate_size(size_t n_outbufs, const struct fastrpc_io_buffer *outbufs);
 void outbufs_encode(size_t n_outbufs, const struct fastrpc_io_buffer *outbufs,

fastrpc/hexagonrpcd/listener.c

@@ -19,6 +19,7 @@
  * along with this program.  If not, see <https://www.gnu.org/licenses/>.
  */
 
+#include <inttypes.h>
 #include <libhexagonrpc/fastrpc.h>
 #include <stddef.h>
 #include <stdio.h>
@@ -29,15 +30,6 @@
 #include "iobuffer.h"
 #include "listener.h"
 
-const struct fastrpc_interface *fastrpc_listener_interfaces[] = {
-	[REMOTECTL_HANDLE] = &localctl_interface,
-
-	&apps_std_interface,
-};
-
-size_t fastrpc_listener_n_interfaces = sizeof(fastrpc_listener_interfaces)
-				     / sizeof(*fastrpc_listener_interfaces);
-
 static int adsp_listener_init2(int fd)
 {
 	return fastrpc2(&adsp_listener_init2_def, fd, ADSP_LISTENER_HANDLE);
@@ -73,6 +65,7 @@ static struct fastrpc_io_buffer *allocate_outbufs(const struct fastrpc_function_
 	off_t off;
 	uint32_t *sizes;
 
+	out_count = def->out_bufs + (def->out_nums && 1);
 	/*
 	 * POSIX allows malloc to return a non-NULL pointer to a zero-size area
 	 * in memory. Since the code below assumes non-zero size if the pointer
@@ -81,7 +74,6 @@ static struct fastrpc_io_buffer *allocate_outbufs(const struct fastrpc_function_
 	if (out_count == 0)
 		return NULL;
 
-	out_count = def->out_bufs + (def->out_nums && 1);
 	out = malloc(sizeof(struct fastrpc_io_buffer) * out_count);
 	if (out == NULL)
 		return NULL;
@@ -123,7 +115,7 @@ static int check_inbuf_sizes(const struct fastrpc_function_def_interp2 *def,
 	if (inbufs[0].s != 4 * (def->in_nums
 			      + def->in_bufs
 			      + def->out_bufs)) {
-		fprintf(stderr, "Invalid number of input numbers: %lu (expected %u)\n",
+		fprintf(stderr, "Invalid number of input numbers: %" PRIu32 " (expected %u)\n",
 				inbufs[0].s,
 				4 * (def->in_nums
 				   + def->in_bufs
@@ -190,12 +182,16 @@ static int return_for_next_invoke(int fd,
 
 	ctx = inbuf_decode_start(*sc);
 	if (!ctx) {
-		perror("Could not start decoding\n");
+		perror("Could not start decoding");
 		ret = -1;
 		goto err_free_outbufs;
 	}
 
-	inbuf_decode(ctx, inbufs_len, inbufs);
+	ret = inbuf_decode(ctx, inbufs_len, inbufs);
+	if (ret) {
+		perror("Could not decode");
+		goto err_free_outbufs;
+	}
 
 	if (!inbuf_decode_is_complete(ctx)) {
 		fprintf(stderr, "Expected more input buffers\n");

fastrpc/hexagonrpcd/listener.h

@@ -46,9 +46,6 @@ extern const struct fastrpc_interface localctl_interface;
 
 extern const struct fastrpc_interface apps_std_interface;
 
-extern const struct fastrpc_interface *fastrpc_listener_interfaces[];
-extern size_t fastrpc_listener_n_interfaces;
-
 int run_fastrpc_listener(int fd,
 			 size_t n_ifaces,
 			 struct fastrpc_interface **ifaces);

fastrpc/hexagonrpcd/rpcd.c

@@ -223,7 +223,7 @@ int main(int argc, char* argv[])
 	listener_args = malloc(sizeof(struct listener_thread_args));
 	if (listener_args == NULL) {
 		perror("Could not create listener arguments");
-		goto err_close_dev;
+		return 1;
 	}
 
 	listener_args->device_dir = "/usr/share/qcom/";

fastrpc/meson.build

@@ -5,3 +5,5 @@ include = include_directories('include')
 subdir('libhexagonrpc')
 
 subdir('hexagonrpcd')
+
+subdir('tests')

fastrpc/tests/meson.build

@@ -0,0 +1,13 @@
+valgrind = find_program('valgrind', required : false)
+
+if valgrind.found()
+  add_test_setup('valgrind', exe_wrapper : valgrind)
+endif
+
+test_iobuffer = executable('test_iobuffer',
+  'test_iobuffer.c',
+  '../hexagonrpcd/iobuffer.c',
+  include_directories : include,
+)
+
+test('iobuffer', test_iobuffer)

fastrpc/tests/test_iobuffer.c

@@ -0,0 +1,245 @@
+/*
+ * FastRPC reverse tunnel - tests for argument encoder/decoder
+ *
+ * Copyright (C) 2023 The Sensor Shell Contributors
+ *
+ * This file is part of sensh.
+ *
+ * Sensh is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
+
+#include <libhexagonrpc/fastrpc.h>
+#include <string.h>
+
+#include "../hexagonrpcd/iobuffer.h"
+
+static const char misaligned_iobufs[] = {
+	/* inbuf 0 (4 bytes misaligned) */
+	0x01, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x12,
+	/* inbuf 1 (5 bytes misaligned) */
+	0x0A, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00,
+	0x02, 0x46, 0x8A, 0xCF, 0x13, 0x57, 0x9B, 0xDF, 0x04, 0x8C,
+	/* inbuf 2 (6 bytes misaligned) */
+	0x03, 0x00, 0x00, 0x00,
+	0x00, 0x00,
+	'A', 'B', 'C',
+	/* inbuf 3 (7 bytes misaligned) */
+	0x04, 0x00, 0x00, 0x00,
+	0x00,
+	'F', 'a', 's', 't',
+	/* inbuf 4 (aligned) */
+	0x05, 0x00, 0x00, 0x00,
+	'S', 'l', 'o', 'w', '\0',
+	/* inbuf 5 (1 byte misaligned) */
+	0x06, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	'f', 'a', 's', 't', 'e', 'r',
+	/* inbuf 6 (2 bytes misaligned) */
+	0x07, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	'F', 'a', 's', 't', 'R', 'P', 'C',
+	/* inbuf 7 (3 bytes misaligned) */
+	0x02, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00,
+	':', 'D',
+};
+
+static char misaligned_decoded0[] = { 0x12, };
+static char misaligned_decoded1[] = {
+	0x02, 0x46, 0x8A, 0xCF, 0x13, 0x57, 0x9B, 0xDF, 0x04, 0x8C,
+};
+static char misaligned_decoded2[] = { 'A', 'B', 'C', };
+static char misaligned_decoded3[] = { 'F', 'a', 's', 't', };
+static char misaligned_decoded4[] = { 'S', 'l', 'o', 'w', '\0', };
+static char misaligned_decoded5[] = { 'f', 'a', 's', 't', 'e', 'r', };
+static char misaligned_decoded6[] = { 'F', 'a', 's', 't', 'R', 'P', 'C', };
+static char misaligned_decoded7[] = { ':', 'D', };
+
+static const struct fastrpc_io_buffer misaligned_decoded[] = {
+	{ .s =  1, .p = misaligned_decoded0, },
+	{ .s = 10, .p = misaligned_decoded1, },
+	{ .s =  3, .p = misaligned_decoded2, },
+	{ .s =  4, .p = misaligned_decoded3, },
+	{ .s =  5, .p = misaligned_decoded4, },
+	{ .s =  6, .p = misaligned_decoded5, },
+	{ .s =  7, .p = misaligned_decoded6, },
+	{ .s =  2, .p = misaligned_decoded7, },
+};
+
+static int test_in_empty(void)
+{
+	struct fastrpc_decoder_context *ctx;
+	struct fastrpc_io_buffer *bufs;
+	int complete;
+
+	ctx = inbuf_decode_start(REMOTE_SCALARS_MAKE(1, 0, 2));
+	if (ctx == NULL)
+		return 1;
+
+	inbuf_decode(ctx, 0, NULL);
+
+	complete = inbuf_decode_is_complete(ctx);
+	if (!complete)
+		return 1;
+
+	bufs = inbuf_decode_finish(ctx);
+
+	iobuf_free(0, bufs);
+
+	return 0;
+}
+
+static int test_in_normal(void)
+{
+	struct fastrpc_decoder_context *ctx;
+	struct fastrpc_io_buffer *bufs;
+	int complete;
+	size_t i;
+	int ret;
+
+	ctx = inbuf_decode_start(REMOTE_SCALARS_MAKE(1, 8, 2));
+	if (ctx == NULL)
+		return 1;
+
+	complete = inbuf_decode_is_complete(ctx);
+	if (complete)
+		return 1;
+
+	inbuf_decode(ctx, sizeof(misaligned_iobufs), misaligned_iobufs);
+
+	complete = inbuf_decode_is_complete(ctx);
+	if (!complete)
+		return 1;
+
+	bufs = inbuf_decode_finish(ctx);
+
+	for (i = 0; i < 8; i++) {
+		if (bufs[i].s != misaligned_decoded[i].s)
+			return 1;
+
+		ret = memcmp(bufs[i].p, misaligned_decoded[i].p, misaligned_decoded[i].s);
+		if (ret)
+			return 1;
+	}
+
+	iobuf_free(8, bufs);
+
+	return 0;
+}
+
+static int test_in_misaligned(void)
+{
+	struct fastrpc_decoder_context *ctx;
+	struct fastrpc_io_buffer *bufs;
+	int complete;
+	size_t i;
+	int ret;
+
+	ctx = inbuf_decode_start(REMOTE_SCALARS_MAKE(1, 8, 2));
+	if (ctx == NULL)
+		return 1;
+
+	for (i = 0; i < sizeof(misaligned_iobufs); i++) {
+		complete = inbuf_decode_is_complete(ctx);
+		if (complete)
+			return 1;
+
+		inbuf_decode(ctx, 1, &misaligned_iobufs[i]);
+	}
+
+	complete = inbuf_decode_is_complete(ctx);
+	if (!complete)
+		return 1;
+
+	bufs = inbuf_decode_finish(ctx);
+
+	for (i = 0; i < 8; i++) {
+		if (bufs[i].s != misaligned_decoded[i].s)
+			return 1;
+
+		ret = memcmp(bufs[i].p, misaligned_decoded[i].p, misaligned_decoded[i].s);
+		if (ret)
+			return 1;
+	}
+
+	iobuf_free(8, bufs);
+
+	return 0;
+}
+
+static int test_out_empty(void)
+{
+	size_t size;
+
+	size = outbufs_calculate_size(0, NULL);
+	if (size != 0)
+		return 1;
+
+	outbufs_encode(0, NULL, NULL);
+
+	return 0;
+}
+
+static int test_out_misaligned(void)
+{
+	size_t size;
+	void *buf;
+	int ret;
+
+	size = outbufs_calculate_size(8, misaligned_decoded);
+	if (size != sizeof(misaligned_iobufs))
+		return 1;
+
+	buf = malloc(size);
+	if (buf == NULL)
+		return 1;
+
+	outbufs_encode(8, misaligned_decoded, buf);
+
+	ret = memcmp(buf, misaligned_iobufs, sizeof(misaligned_iobufs));
+	if (ret)
+		return 1;
+
+	free(buf);
+
+	return 0;
+}
+
+int main(int argc, const char **argv)
+{
+	int ret;
+
+	ret = test_in_empty();
+	if (ret)
+		return ret;
+
+	ret = test_in_normal();
+	if (ret)
+		return ret;
+
+	ret = test_in_misaligned();
+	if (ret)
+		return ret;
+
+	ret = test_out_empty();
+	if (ret)
+		return ret;
+
+	ret = test_out_misaligned();
+	if (ret)
+		return ret;
+}