Merge pull request #5950 from boretrk/posixtest

open: input validation for empty segments in path

13 files changed, 44 insertions, 21 deletions

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index b58f57758..572f02544 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -91,7 +91,7 @@ jobs:
           env:
             CC: gcc
             CMAKE_GENERATOR: Ninja
-            CMAKE_OPTIONS: -DUSE_HTTPS=OpenSSL -DREGEX_BACKEND=builtin -DDEPRECATE_HARD=ON -DUSE_LEAK_CHECKER=valgrind -DUSE_GSSAPI=ON -DDEBUG_STRICT_ALLOC=ON
+            CMAKE_OPTIONS: -DUSE_HTTPS=OpenSSL -DREGEX_BACKEND=builtin -DDEPRECATE_HARD=ON -DUSE_LEAK_CHECKER=valgrind -DUSE_GSSAPI=ON -DDEBUG_STRICT_ALLOC=ON -DDEBUG_STRICT_OPEN=ON
           os: ubuntu-latest
         - # Xenial, GCC, mbedTLS
           container:
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 96bdb4cf0..a9f544174 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -51,6 +51,7 @@ OPTION(USE_STANDALONE_FUZZERS		"Enable standalone fuzzers (compatible with gcc)"
 OPTION(USE_LEAK_CHECKER			"Run tests with leak checker"				OFF)
 OPTION(DEBUG_POOL			"Enable debug pool allocator"				OFF)
 OPTION(DEBUG_STRICT_ALLOC		"Enable strict allocator behavior"			OFF)
+OPTION(DEBUG_STRICT_OPEN		"Enable path validation in open"			OFF)
 OPTION(ENABLE_WERROR			"Enable compilation with -Werror"			OFF)
 OPTION(USE_BUNDLED_ZLIB    		"Use the bundled version of zlib. Can be set to one of Bundled(ON)/Chromium. The Chromium option requires a x86_64 processor with SSE4.2 and CLMUL"			OFF)
    SET(USE_HTTP_PARSER			"" CACHE STRING "Specifies the HTTP Parser implementation; either system or builtin.")
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 28ef258f3..45dec2796 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -11,6 +11,11 @@ IF(DEBUG_STRICT_ALLOC)
 ENDIF()
 ADD_FEATURE_INFO(debugalloc GIT_DEBUG_STRICT_ALLOC "debug strict allocators")
 
+IF(DEBUG_STRICT_OPEN)
+	SET(GIT_DEBUG_STRICT_OPEN 1)
+ENDIF()
+ADD_FEATURE_INFO(debugopen GIT_DEBUG_STRICT_OPEN "path validation in open")
+
 INCLUDE(PkgBuildConfig)
 INCLUDE(SanitizeBool)
 
diff --git a/src/features.h.in b/src/features.h.in
index 41f2734ac..202cef49e 100644
--- a/src/features.h.in
+++ b/src/features.h.in
@@ -3,6 +3,7 @@
 
 #cmakedefine GIT_DEBUG_POOL 1
 #cmakedefine GIT_DEBUG_STRICT_ALLOC 1
+#cmakedefine GIT_DEBUG_STRICT_OPEN 1
 
 #cmakedefine GIT_TRACE 1
 #cmakedefine GIT_THREADS 1
diff --git a/src/posix.c b/src/posix.c
index bf764ae6b..c40134824 100644
--- a/src/posix.c
+++ b/src/posix.c
@@ -109,6 +109,13 @@ int p_open(const char *path, volatile int flags, ...)
 {
 	mode_t mode = 0;
 
+	#ifdef GIT_DEBUG_STRICT_OPEN
+	if (strstr(path, "//") != NULL) {
+		errno = EACCES;
+		return -1;
+	}
+	#endif
+
 	if (flags & O_CREAT) {
 		va_list arg_list;
 
diff --git a/src/refdb_fs.c b/src/refdb_fs.c
index 0b8e103c2..7cf48b13d 100644
--- a/src/refdb_fs.c
+++ b/src/refdb_fs.c
@@ -578,7 +578,7 @@ static int iter_load_loose_paths(refdb_fs_backend *backend, refdb_fs_iter *iter)
 		}
 	}
 
-	if ((error = git_buf_printf(&path, "%s/", backend->commonpath)) < 0 ||
+	if ((error = git_buf_puts(&path, backend->commonpath)) < 0 ||
 		(error = git_buf_put(&path, ref_prefix, ref_prefix_len)) < 0) {
 		git_buf_dispose(&path);
 		return error;
@@ -1609,8 +1609,9 @@ static char *setup_namespace(git_repository *repo, const char *in)
 			GIT_MKDIR_PATH, NULL) < 0)
 		goto done;
 
-	/* Return root of the namespaced gitpath, i.e. without the trailing '/refs' */
+	/* Return root of the namespaced gitpath, i.e. without the trailing 'refs' */
 	git_buf_rtruncate_at_char(&path, '/');
+	git_buf_putc(&path, '/');
 	out = git_buf_detach(&path);
 
 done:
diff --git a/src/win32/posix_w32.c b/src/win32/posix_w32.c
index 0a8f2bee0..7fcc472e9 100644
--- a/src/win32/posix_w32.c
+++ b/src/win32/posix_w32.c
@@ -543,6 +543,13 @@ int p_open(const char *path, int flags, ...)
 	mode_t mode = 0;
 	struct open_opts opts = {0};
 
+	#ifdef GIT_DEBUG_STRICT_OPEN
+	if (strstr(path, "//") != NULL) {
+		errno = EACCES;
+		return -1;
+	}
+	#endif
+
 	if (git_win32_path_from_utf8(wpath, path) < 0)
 		return -1;
 
diff --git a/src/worktree.c b/src/worktree.c
index 0bced6d4a..fe8db7743 100644
--- a/src/worktree.c
+++ b/src/worktree.c
@@ -43,7 +43,7 @@ int git_worktree_list(git_strarray *wts, git_repository *repo)
 	wts->count = 0;
 	wts->strings = NULL;
 
-	if ((error = git_buf_printf(&path, "%s/worktrees/", repo->commondir)) < 0)
+	if ((error = git_buf_joinpath(&path, repo->commondir, "worktrees/")) < 0)
 		goto exit;
 	if (!git_path_exists(path.ptr) || git_path_is_empty_dir(path.ptr))
 		goto exit;
@@ -182,7 +182,7 @@ int git_worktree_lookup(git_worktree **out, git_repository *repo, const char *na
 
 	*out = NULL;
 
-	if ((error = git_buf_printf(&path, "%s/worktrees/%s", repo->commondir, name)) < 0)
+	if ((error = git_buf_join3(&path, '/', repo->commondir, "worktrees", name)) < 0)
 		goto out;
 
 	if ((error = (open_worktree_dir(out, git_repository_workdir(repo), path.ptr, name))) < 0)
@@ -592,7 +592,7 @@ int git_worktree_prune(git_worktree *wt,
 	}
 
 	/* Delete gitdir in parent repository */
-	if ((err = git_buf_printf(&path, "%s/worktrees/%s", wt->commondir_path, wt->name)) < 0)
+	if ((err = git_buf_join3(&path, '/', wt->commondir_path, "worktrees", wt->name)) < 0)
 		goto out;
 	if (!git_path_exists(path.ptr))
 	{
diff --git a/tests/iterator/workdir.c b/tests/iterator/workdir.c
index 547fb7d95..82ee363f9 100644
--- a/tests/iterator/workdir.c
+++ b/tests/iterator/workdir.c
@@ -1024,7 +1024,7 @@ static void create_paths(const char *root, int depth)
 	int i;
 
 	cl_git_pass(git_buf_puts(&fullpath, root));
-	cl_git_pass(git_buf_putc(&fullpath, '/'));
+	cl_git_pass(git_path_to_dir(&fullpath));
 
 	root_len = fullpath.size;
 
diff --git a/tests/merge/workdir/setup.c b/tests/merge/workdir/setup.c
index ad29fcd94..0b85f2712 100644
--- a/tests/merge/workdir/setup.c
+++ b/tests/merge/workdir/setup.c
@@ -49,7 +49,7 @@ static bool test_file_contents(const char *filename, const char *expected)
 	git_buf file_path_buf = GIT_BUF_INIT, file_buf = GIT_BUF_INIT;
 	bool equals;
 	
-	git_buf_printf(&file_path_buf, "%s/%s", git_repository_path(repo), filename);
+	git_buf_joinpath(&file_path_buf, git_repository_path(repo), filename);
 	
 	cl_git_pass(git_futils_readbuffer(&file_buf, file_path_buf.ptr));
 	equals = (strcmp(file_buf.ptr, expected) == 0);
@@ -64,7 +64,7 @@ static void write_file_contents(const char *filename, const char *output)
 {
 	git_buf file_path_buf = GIT_BUF_INIT;
 
-	git_buf_printf(&file_path_buf, "%s/%s", git_repository_path(repo),
+	git_buf_joinpath(&file_path_buf, git_repository_path(repo),
 		filename);
 	cl_git_rewritefile(file_path_buf.ptr, output);
 
diff --git a/tests/odb/foreach.c b/tests/odb/foreach.c
index 7a45a57ae..02112380b 100644
--- a/tests/odb/foreach.c
+++ b/tests/odb/foreach.c
@@ -127,7 +127,7 @@ void test_odb_foreach__files_in_objects_dir(void)
 	cl_fixture_sandbox("testrepo.git");
 	cl_git_pass(git_repository_open(&repo, "testrepo.git"));
 
-	cl_git_pass(git_buf_printf(&buf, "%s/objects/somefile", git_repository_path(repo)));
+	cl_git_pass(git_buf_joinpath(&buf, git_repository_path(repo), "objects/somefile"));
 	cl_git_mkfile(buf.ptr, "");
 	git_buf_dispose(&buf);
 
diff --git a/tests/worktree/merge.c b/tests/worktree/merge.c
index 4b743829c..2a1206032 100644
--- a/tests/worktree/merge.c
+++ b/tests/worktree/merge.c
@@ -70,9 +70,9 @@ void test_worktree_merge__merge_setup(void)
 		    ours, (const git_annotated_commit **)&theirs, 1));
 
 	for (i = 0; i < ARRAY_SIZE(merge_files); i++) {
-		git_buf_clear(&path);
-		cl_git_pass(git_buf_printf(&path, "%s/%s",
-			    fixture.worktree->gitdir, merge_files[i]));
+		cl_git_pass(git_buf_joinpath(&path,
+		            fixture.worktree->gitdir,
+		            merge_files[i]));
 		cl_assert(git_path_exists(path.ptr));
 	}
 
diff --git a/tests/worktree/worktree.c b/tests/worktree/worktree.c
index f2078a3f9..9b87bfae6 100644
--- a/tests/worktree/worktree.c
+++ b/tests/worktree/worktree.c
@@ -44,8 +44,9 @@ void test_worktree_worktree__list_with_invalid_worktree_dirs(void)
 	git_strarray wts;
 	size_t i, j, len;
 
-	cl_git_pass(git_buf_printf(&path, "%s/worktrees/invalid",
-		    fixture.repo->commondir));
+	cl_git_pass(git_buf_joinpath(&path,
+	            fixture.repo->commondir,
+	            "worktrees/invalid"));
 	cl_git_pass(p_mkdir(path.ptr, 0755));
 
 	len = path.size;
@@ -145,9 +146,9 @@ void test_worktree_worktree__open_invalid_commondir(void)
 	git_buf buf = GIT_BUF_INIT, path = GIT_BUF_INIT;
 
 	cl_git_pass(git_buf_sets(&buf, "/path/to/nonexistent/commondir"));
-	cl_git_pass(git_buf_printf(&path,
-		    "%s/worktrees/testrepo-worktree/commondir",
-		    fixture.repo->commondir));
+	cl_git_pass(git_buf_joinpath(&path,
+	            fixture.repo->commondir,
+	            "worktrees/testrepo-worktree/commondir"));
 	cl_git_pass(git_futils_writebuffer(&buf, path.ptr, O_RDWR, 0644));
 
 	cl_git_pass(git_worktree_lookup(&wt, fixture.repo, "testrepo-worktree"));
@@ -165,9 +166,9 @@ void test_worktree_worktree__open_invalid_gitdir(void)
 	git_buf buf = GIT_BUF_INIT, path = GIT_BUF_INIT;
 
 	cl_git_pass(git_buf_sets(&buf, "/path/to/nonexistent/gitdir"));
-	cl_git_pass(git_buf_printf(&path,
-		    "%s/worktrees/testrepo-worktree/gitdir",
-		    fixture.repo->commondir));
+	cl_git_pass(git_buf_joinpath(&path,
+	            fixture.repo->commondir,
+	            "worktrees/testrepo-worktree/gitdir"));
 	cl_git_pass(git_futils_writebuffer(&buf, path.ptr, O_RDWR, 0644));
 
 	cl_git_pass(git_worktree_lookup(&wt, fixture.repo, "testrepo-worktree"));