chip/host: Make persistence's path handling more robust

Originally I only touched the file because gcc was complaining that
snprintf might end up creating a non-terminated string even though
the code already care about that.

But there was another failure mode in there: persistence is used to
provide some semi-permanent temporary storage whose naming rules are
robust against multiple invocations of the same executable while also
avoiding collisions between multiple processes trying to create such
files at the same time (such as many parallel test suite runs).
If the executable's path is close to the limit given in PATH_MAX
(no matter if that is the true limit of the OS) filenames would be
truncated silently. Since a single executable can create multiple
such files only differentiated by their "tag", added as a suffix,
we'd end up with a test using the same file for multiple purposes
which is not the kind of issue I'd want to debug.

Instead, define some reasonably-looking limits and bail out explicitly
using ASSERT: If things go south, at least there's a constraint
violation message with a file name and source line to hunt for, like
this:
ASSERTION FAIL: chip/host/persistence.c:55:get_storage_path - sz <= max_len

While at it, also explain the purpose of these routines and why mkstemp
isn't a suitable replacement so the next hapless soul doesn't have
to do the same kind of digging I did.

BUG=b:185477664
BRANCH=none
TEST=New toolchain doesn't fail on chip/host/persistence.c anymore.
The tests still pass. Also tested that the assert triggers properly
by locally changing it to some ridiculously small value and running
the tests.

Signed-off-by: Patrick Georgi <pgeorgi@google.com>
Change-Id: I4869bb58861220401d58755489e896bbcb2102d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/2846333
Reviewed-by: Patrick Georgi <pgeorgi@chromium.org>
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>
Commit-Queue: Patrick Georgi <pgeorgi@chromium.org>
Tested-by: Patrick Georgi <pgeorgi@chromium.org>

1 files changed, 49 insertions, 3 deletions

diff --git a/chip/host/persistence.c b/chip/host/persistence.c
index 90856b3bdf..44d60f1bb8 100644
--- a/chip/host/persistence.c
+++ b/chip/host/persistence.c
@@ -5,11 +5,43 @@
 
 /* Persistence module for emulator */
 
+/* This provides storage that can be opened, closed and reopened by the
+ * current process at will, whose naming even remains stable across multiple
+ * invocations of the same executable, while providing a unique name for
+ * each executable (as determined by path) that uses these routines.
+ *
+ * Useful when semi-permanent storage is required even with many
+ * similar processes running in parallel (e.g. in a highly parallel
+ * test suite run.
+ *
+ * mkstemp and friends don't provide these properties which is why we have
+ * this homegrown implementation of something similar-yet-different.
+ */
+
 #include <linux/limits.h>
 #include <unistd.h>
 #include <stdio.h>
 #include <string.h>
 
+#include "util.h"
+
+/* The longest path in a chroot seems to be about 280 characters (as of
+ * April 2021) so define a cut-off instead of just hoping for the best:
+ * If we were to run into a path that is nearly PATH_MAX bytes long,
+ * file names could end up being reused inadvertedly because the various
+ * snprintf calls would cut off the trailing characters, so the "tag" (and
+ * maybe more) is gone even though it only exists for differentiation.
+ *
+ * Instead bail out if we encounter a path (to an executable using these
+ * routines) that is longer than we expect.
+ *
+ * Round up for some spare room because why not?
+ */
+static const int max_len = 300;
+
+/* This must be at least the size of the prefix added in get_storage_path */
+static const int max_prefix_len = 25;
+
 static void get_storage_path(char *out)
 {
 	char buf[PATH_MAX];
@@ -19,6 +51,8 @@ static void get_storage_path(char *out)
 	sz = readlink("/proc/self/exe", buf, PATH_MAX - 1);
 	buf[sz] = '\0';
 
+	ASSERT(sz <= max_len);
+
 	/* replace / by underscores in the path to get the shared memory name */
 	current = strchr(buf, '/');
 	while (current) {
@@ -26,8 +60,12 @@ static void get_storage_path(char *out)
 		current = strchr(current, '/');
 	}
 
-	snprintf(out, PATH_MAX - 1, "/dev/shm/EC_persist_%s", buf);
+
+	sz = snprintf(out, PATH_MAX - 1, "/dev/shm/EC_persist_%.*s",
+		max_len, buf);
 	out[PATH_MAX - 1] = '\0';
+
+	ASSERT(sz <= max_len + max_prefix_len);
 }
 
 FILE *get_persistent_storage(const char *tag, const char *mode)
@@ -35,12 +73,16 @@ FILE *get_persistent_storage(const char *tag, const char *mode)
 	char buf[PATH_MAX];
 	char path[PATH_MAX];
 
+	/* There's no longer tag in use right now, and there shouldn't be. */
+	ASSERT(strlen(tag) < 32);
+
 	/*
 	 * The persistent storage with tag 'foo' for test 'bar' would
 	 * be named 'bar_persist_foo'
 	 */
 	get_storage_path(buf);
-	snprintf(path, PATH_MAX - 1, "%s_%s", buf, tag);
+	snprintf(path, PATH_MAX - 1, "%.*s_%32s",
+		max_len + max_prefix_len, buf, tag);
 	path[PATH_MAX - 1] = '\0';
 
 	return fopen(path, mode);
@@ -56,8 +98,12 @@ void remove_persistent_storage(const char *tag)
 	char buf[PATH_MAX];
 	char path[PATH_MAX];
 
+	/* There's no longer tag in use right now, and there shouldn't be. */
+	ASSERT(strlen(tag) < 32);
+
 	get_storage_path(buf);
-	snprintf(path, PATH_MAX - 1, "%s_%s", buf, tag);
+	snprintf(path, PATH_MAX - 1, "%.*s_%32s",
+		max_len + max_prefix_len, buf, tag);
 	path[PATH_MAX - 1] = '\0';
 
 	unlink(path);