DAOS-18323 ddb: Incorrect parsing of vos path with DDB

Fix ddb vos file path parsing.

Features: recovery
Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

Author: kanard38

src/utils/ddb/ddb_parse.c

@@ -8,6 +8,9 @@
 
 #include <wordexp.h>
 #include <getopt.h>
+#include <regex.h>
+#include <errno.h>
+#include <string.h>
 
 #include <daos_errno.h>
 #include <daos_srv/bio.h>
@@ -23,60 +26,123 @@ safe_strcat(char *dst, const char *src, size_t dst_size)
 	strncat(dst, src, remaining_space);
 }
 
+static void
+print_regx_error(int rc, regex_t *preg, const char *regex_buf)
+{
+	char  *buf;
+	size_t buf_size;
+
+	buf_size = regerror(rc, preg, NULL, 0);
+	D_ALLOC_ARRAY(buf, buf_size);
+	D_ASSERT(buf != NULL);
+	regerror(rc, preg, buf, buf_size);
+	D_CRIT("Invalid regex '%s': %s", regex_buf, buf);
+	D_FREE(buf);
+}
+
 int
 vos_path_parse(const char *path, struct vos_file_parts *vos_file_parts)
 {
-	uint32_t	 path_len = strlen(path) + 1;
-	char		*path_copy;
-	char		*tok;
-	int		 rc = -DER_INVAL;
+	enum {
+		DB_PATH_IDX       = 1,
+		POOL_UUID_IDX     = 3,
+		VOS_FILE_NAME_IDX = 5,
+		TARGET_IDX_IDX    = 6,
+		MATCH_SIZE        = 7,
+		POOL_UUID_LEN     = 36
+	};
+	const char *regex_buf =
+	    "^(/?([^/]+/)*)([0-9a-fA-F]{8}(-[0-9a-fA-F]{4}){3}-[0-9a-fA-F]{12})/"
+	    "((vos-([0-9]|([1-9][0-9]+)))|(rdb-pool)reviewers comments:)$";
+	char      *endptr;
+	uint64_t   target_idx;
+	regex_t    preg;
+	char       pool_uuid[POOL_UUID_LEN + 1];
+	regmatch_t match[MATCH_SIZE + 1];
+	size_t     vos_file_name_len;
+	int        rc;
 
 	D_ASSERT(path != NULL && vos_file_parts != NULL);
 
-	D_ALLOC(path_copy, path_len);
-	if (path_copy == NULL)
-		return -DER_NOMEM;
-	strcpy(path_copy, path);
-
-	tok = strtok(path_copy, "/");
-	while (tok != NULL && rc != 0) {
-		rc = uuid_parse(tok, vos_file_parts->vf_pool_uuid);
-		if (!SUCCESS(rc)) {
-			safe_strcat(vos_file_parts->vf_db_path, "/", DB_PATH_LEN);
-			safe_strcat(vos_file_parts->vf_db_path, tok, DB_PATH_LEN);
-		}
-		tok = strtok(NULL, "/");
+	rc = regcomp(&preg, regex_buf, REG_EXTENDED);
+	if (rc != 0) {
+		print_regx_error(rc, &preg, regex_buf);
+		rc = -DER_INVAL;
+		goto out;
 	}
 
-	if (rc != 0 || tok == NULL) {
-		D_ERROR("Incomplete path: %s\n", path);
-		D_GOTO(done, rc = -DER_INVAL);
+	rc = regexec(&preg, path, MATCH_SIZE, match, 0);
+	if (rc == REG_NOMATCH) {
+		D_ERROR("Innvalid VOS path: '%s'\n", path);
+		rc = -DER_INVAL;
+		goto out_preg;
+	}
+	D_ASSERT(SUCCESS(rc));
+
+	vos_file_parts->vf_db_path[0] = '\0';
+	if ((match[DB_PATH_IDX].rm_eo - match[DB_PATH_IDX].rm_so) != 0) {
+		D_ASSERT(match[DB_PATH_IDX].rm_so == 0);
+		if (match[DB_PATH_IDX].rm_eo > DB_PATH_SIZE) {
+			D_ERROR("DB path '%.*s' too long in VOS path '%s': get=%i, max=%i\n",
+				match[DB_PATH_IDX].rm_eo - 1, &path[match[DB_PATH_IDX].rm_so], path,
+				match[DB_PATH_IDX].rm_eo - 1, DB_PATH_SIZE - 1);
+			rc = -DER_INVAL;
+			goto out_preg;
+		}
+		memcpy(vos_file_parts->vf_db_path, path, match[DB_PATH_IDX].rm_eo - 1);
+		vos_file_parts->vf_db_path[match[DB_PATH_IDX].rm_eo - 1] = '\0';
 	}
 
-	strncpy(vos_file_parts->vf_vos_file, tok, ARRAY_SIZE(vos_file_parts->vf_vos_file) - 1);
-
-	if (strcmp(vos_file_parts->vf_vos_file, "rdb-pool") == 0) {
-		vos_file_parts->vf_target_idx = BIO_SYS_TGT_ID;
-		goto done;
+	D_ASSERT(match[POOL_UUID_IDX].rm_so != (regoff_t)-1);
+	D_ASSERT(match[POOL_UUID_IDX].rm_eo - match[POOL_UUID_IDX].rm_so == POOL_UUID_LEN);
+	memcpy(pool_uuid, &path[match[POOL_UUID_IDX].rm_so], POOL_UUID_LEN);
+	pool_uuid[POOL_UUID_LEN] = '\0';
+	rc                       = uuid_parse(pool_uuid, vos_file_parts->vf_pool_uuid);
+	if (!SUCCESS(rc)) {
+		D_CRIT("Invalid Pool UUID '%s' in VOS path '%s'\n", pool_uuid, path);
+		rc = -DER_INVAL;
+		goto out_preg;
 	}
 
-	/*
-	 * file name should be vos-N ... split on "-"
-	 * If not, might be test, just assume target of 0
-	 */
-	strtok(tok, "-");
-	tok = strtok(NULL, "-");
-	if (tok != NULL) {
-		D_WARN("vos file name not in correct format: %s\n", vos_file_parts->vf_vos_file);
-		vos_file_parts->vf_target_idx = atoi(tok);
+	D_ASSERT(match[VOS_FILE_NAME_IDX].rm_so != (regoff_t)-1);
+	vos_file_name_len = match[VOS_FILE_NAME_IDX].rm_eo - match[VOS_FILE_NAME_IDX].rm_so;
+	if (vos_file_name_len + 1 > VOS_FILE_NAME_SIZE) {
+		D_ERROR("VOS file name '%.*s' too long in VOS path '%s': get=%zu, max=%i\n",
+			(int)vos_file_name_len, &path[match[VOS_FILE_NAME_IDX].rm_so], path,
+			vos_file_name_len, VOS_FILE_NAME_SIZE - 1);
+		rc = -DER_INVAL;
+		goto out_preg;
+	}
+	memcpy(vos_file_parts->vf_vos_file_name, &path[match[VOS_FILE_NAME_IDX].rm_so],
+	       vos_file_name_len);
+	vos_file_parts->vf_vos_file_name[vos_file_name_len] = '\0';
+
+	D_ASSERT(match[TARGET_IDX_IDX].rm_so != (regoff_t)-1);
+	errno      = 0;
+	target_idx = strtoull(&path[match[TARGET_IDX_IDX].rm_so], &endptr, 10);
+	if (errno != 0 || endptr == &path[match[TARGET_IDX_IDX].rm_so] || *endptr != '\0') {
+		D_CRIT("Invalid target index '%s' in VOS path '%s': %s\n",
+		       &path[match[TARGET_IDX_IDX].rm_so], path, strerror(errno));
+		rc = -DER_INVAL;
+		goto out_preg;
 	}
+	if (target_idx > UINT32_MAX) {
+		D_ERROR("Target index " DF_U64
+			"' out of range in VOS path '%s': min=0 , max=%" PRIu32 "\n",
+			target_idx, path, UINT32_MAX);
+		rc = -DER_INVAL;
+		goto out_preg;
+	}
+	vos_file_parts->vf_target_idx = target_idx;
 
-done:
-	if (!SUCCESS(rc)) {
-		/* Reset to if not valid */
+	rc = -DER_SUCCESS;
+
+out_preg:
+	regfree(&preg);
+out:
+	/* Reset to zero if not valid */
+	if (!SUCCESS(rc))
 		memset(vos_file_parts, 0, sizeof(*vos_file_parts));
-	}
-	D_FREE(path_copy);
 	return rc;
 }
 

src/utils/ddb/ddb_parse.h

@@ -15,12 +15,12 @@
 #include <daos_types.h>
 #include "ddb_common.h"
 
-#define DB_PATH_LEN 256
+enum { DB_PATH_SIZE = 256, VOS_FILE_NAME_SIZE = 16 };
 struct vos_file_parts {
-	char            vf_db_path[DB_PATH_LEN];
-	uuid_t		vf_pool_uuid;
-	char		vf_vos_file[16];
-	uint32_t	vf_target_idx;
+	char     vf_db_path[DB_PATH_SIZE];
+	uuid_t   vf_pool_uuid;
+	char     vf_vos_file_name[VOS_FILE_NAME_SIZE];
+	uint32_t vf_target_idx;
 };
 
 /* Parse a path to a VOS file to get needed parts for initializing vos */

src/utils/ddb/ddb_vos.c

@@ -85,7 +85,7 @@ dv_pool_destroy(const char *path, const char *db_path)
 		return rc;
 	}
 
-	if (strncmp(path_parts.vf_vos_file, "rdb", 3) == 0)
+	if (strncmp(path_parts.vf_vos_file_name, "rdb", 3) == 0)
 		flags |= VOS_POF_RDB;
 
 	rc = vos_pool_destroy_ex(path, path_parts.vf_pool_uuid, flags);

src/utils/ddb/tests/SConscript

@@ -50,10 +50,11 @@ def scons():
     prereqs.require(tenv, 'argobots', 'spdk', 'pmdk')
     # Required for dtx_act_discard_invalid tests.
     # This function is validated by its respective unit tests.
-    tenv.AppendUnique(LINKFLAGS=['-Wl,--wrap=vos_dtx_discard_invalid'])
+    tenv.AppendUnique(LINKFLAGS=['-Wl,--wrap=vos_dtx_discard_invalid', '-Wl,--wrap=regcomp',
+                                 '-Wl,--wrap=uuid_parse', '-Wl,--wrap=strtoull'])
 
     libs = ['uuid', 'daos_common_pmem', 'gurt', 'vea', 'abt', 'bio', 'cmocka', 'pthread', 'pmemobj']
-    src = ['ddb_ut.c', 'ddb_vos_ut.c']
+    src = ['ddb_ut.c', 'ddb_vos_ut.c', 'ddb_parse_ut.c']
     # vos and mock object files to wrap vos symbols
     vos_obj = tenv.Object(Glob('../../../vos/*.c'))
     mock_obj = tenv.Object(Glob('../../../dtx/tests/*_mock.c'))

src/utils/ddb/tests/ddb_parse_tests.c

@@ -13,6 +13,14 @@
 #include "ddb_cmocka.h"
 #include "ddb_test_driver.h"
 
+/*
+ * -----------------------------------------------
+ * Mock implementations
+ * -----------------------------------------------
+ */
+
+#define MOCKED_POOL_UUID_STR "12345678-1234-1234-1234-123456789012"
+
 static int
 fake_print(const char *fmt, ...)
 {
@@ -25,27 +33,89 @@ fake_print(const char *fmt, ...)
  * -----------------------------------------------
  */
 
-#define assert_invalid_f_path(path, parts) assert_invalid(vos_path_parse(path, &parts))
-#define assert_f_path(path, parts) assert_success(vos_path_parse(path, &parts))
-
 static void
-vos_file_parts_tests(void **state)
+vos_file_parse_test_errors(void **state)
 {
+	uuid_t                pool_uuid;
 	struct vos_file_parts parts = {0};
-	uuid_t expected_uuid;
+	char                 *buf;
+	int                   rc;
 
-	uuid_parse("12345678-1234-1234-1234-123456789012", expected_uuid);
+	rc = uuid_parse(MOCKED_POOL_UUID_STR, pool_uuid);
+	assert_rc_equal(rc, 0);
 
-	assert_invalid_f_path("", parts);
-	assert_invalid_f_path("/mnt/daos", parts);
-	assert_invalid_f_path("/mnt/daos/12345678-1234-1234-1234-123456789012", parts);
+	/* Test invalid vos paths not respecting regex */
+	rc = vos_path_parse("", &parts);
+	assert_rc_equal(rc, -DER_INVAL);
 
-	assert_f_path("/mnt/daos/12345678-1234-1234-1234-123456789012/vos-1", parts);
+	rc = vos_path_parse("/mnt/daos", &parts);
+	assert_rc_equal(rc, -DER_INVAL);
+
+	rc = vos_path_parse("/mnt/daos/" MOCKED_POOL_UUID_STR, &parts);
+	assert_rc_equal(rc, -DER_INVAL);
+
+	rc = vos_path_parse("//mnt/daos/" MOCKED_POOL_UUID_STR "/vos-1", &parts);
+	assert_rc_equal(rc, -DER_INVAL);
+
+	rc = vos_path_parse("/mnt/daos/g2345678-1234-1234-1234-123456789012/vos-1", &parts);
+	assert_rc_equal(rc, -DER_INVAL);
 
+	/* Test invalid vos paths with too long db path */
+	D_ALLOC(buf, DB_PATH_SIZE + 64);
+	assert_non_null(buf);
+	memset(buf, 'a', DB_PATH_SIZE + 64);
+	buf[0] = '/';
+	memcpy(&buf[DB_PATH_SIZE], "/" MOCKED_POOL_UUID_STR "/vos-0",
+	       sizeof("/" MOCKED_POOL_UUID_STR "/vos-0"));
+	rc = vos_path_parse(buf, &parts);
+	D_FREE(buf);
+	assert_rc_equal(rc, -DER_INVAL);
+	D_FREE(buf);
+
+	/* Test invalid vos paths with too long vos file name */
+	rc = vos_path_parse("/mnt/daos/" MOCKED_POOL_UUID_STR "/vos-999999999999", &parts);
+	assert_rc_equal(rc, -DER_INVAL);
+
+	/* Test invalid vos paths with invalid target idx */
+	rc = vos_path_parse("/mnt/daos/" MOCKED_POOL_UUID_STR "/vos-99999999999", &parts);
+	assert_rc_equal(rc, -DER_INVAL);
+}
+
+static void
+vos_file_parse_test_success(void **state)
+{
+	uuid_t                expected_uuid;
+	struct vos_file_parts parts = {0};
+	int                   rc;
+
+	rc = uuid_parse(MOCKED_POOL_UUID_STR, expected_uuid);
+	assert_rc_equal(rc, 0);
+
+	/* Test with absolute path */
+	rc = vos_path_parse("/mnt/daos/" MOCKED_POOL_UUID_STR "/vos-0", &parts);
+	assert_rc_equal(rc, 0);
 	assert_string_equal("/mnt/daos", parts.vf_db_path);
 	assert_uuid_equal(expected_uuid, parts.vf_pool_uuid);
-	assert_string_equal("vos-1", parts.vf_vos_file);
-	assert_int_equal(1, parts.vf_target_idx);
+	assert_string_equal("vos-0", parts.vf_vos_file_name);
+	assert_int_equal(0, parts.vf_target_idx);
+
+	/* Test with relative path */
+	memset(&parts, 0, sizeof(parts));
+	rc = vos_path_parse("mnt/daos/" MOCKED_POOL_UUID_STR "/vos-42", &parts);
+	assert_rc_equal(rc, 0);
+	assert_string_equal("mnt/daos", parts.vf_db_path);
+	assert_uuid_equal(expected_uuid, parts.vf_pool_uuid);
+	assert_string_equal("vos-42", parts.vf_vos_file_name);
+	assert_int_equal(42, parts.vf_target_idx);
+
+	/* Test with null db path */
+	memset(&parts, 1, sizeof(parts));
+	rc = vos_path_parse(MOCKED_POOL_UUID_STR "/vos-666", &parts);
+	assert_rc_equal(rc, 0);
+	assert_string_equal("", parts.vf_db_path);
+	assert_uuid_equal(expected_uuid, parts.vf_pool_uuid);
+	assert_string_equal("vos-666", parts.vf_vos_file_name);
+	assert_int_equal(666, parts.vf_target_idx);
 }
 
 #define assert_vtp_eq(a, b) \
@@ -293,8 +363,9 @@ int
 ddb_parse_tests_run()
 {
 	static const struct CMUnitTest tests[] = {
-	    TEST(vos_file_parts_tests), TEST(parse_dtx_id_tests),  TEST(keys_are_parsed_correctly),
-	    TEST(pool_flags_tests),     TEST(date2cmt_time_tests),
+	    TEST(vos_file_parse_test_errors), TEST(vos_file_parse_test_success),
+	    TEST(parse_dtx_id_tests),         TEST(keys_are_parsed_correctly),
+	    TEST(pool_flags_tests),           TEST(date2cmt_time_tests),
 	};
 	return cmocka_run_group_tests_name("DDB helper parsing function tests", tests,
 					   NULL, NULL);