Merge pull request #2210 from libgit2/rb/submodule-api-with-no-submodules

Fix segfault if gitmodules is invalid

4 files changed, 139 insertions, 7 deletions

diff --git a/include/git2/submodule.h b/include/git2/submodule.h
index 789f2c045..28e235725 100644
--- a/include/git2/submodule.h
+++ b/include/git2/submodule.h
@@ -115,8 +115,8 @@ typedef enum {
  *
  * - The submodule is not mentioned in the HEAD, the index, and the config,
  *   but does "exist" in the working directory (i.e. there is a subdirectory
- *   that is a valid self-contained git repo).  In this case, this function
- *   returns GIT_EEXISTS to indicate the the submodule exists but not in a
+ *   that appears to be a Git repository).  In this case, this function
+ *   returns GIT_EEXISTS to indicate a sub-repository exists but not in a
  *   state where a git_submodule can be instantiated.
  * - The submodule is not mentioned in the HEAD, index, or config and the
  *   working directory doesn't contain a value git repo at that path.
@@ -129,7 +129,7 @@ typedef enum {
  * @param repo The parent repository
  * @param name The name of or path to the submodule; trailing slashes okay
  * @return 0 on success, GIT_ENOTFOUND if submodule does not exist,
- *         GIT_EEXISTS if submodule exists in working directory only,
+ *         GIT_EEXISTS if a repository is found in working directory only,
  *         -1 on other errors.
  */
 GIT_EXTERN(int) git_submodule_lookup(
diff --git a/src/submodule.c b/src/submodule.c
index 769b092a0..b07c9d917 100644
--- a/src/submodule.c
+++ b/src/submodule.c
@@ -156,7 +156,7 @@ int git_submodule_lookup(
 			if (git_buf_joinpath(&path, git_repository_workdir(repo), name) < 0)
 				return -1;
 
-			if (git_path_contains_dir(&path, DOT_GIT))
+			if (git_path_contains(&path, DOT_GIT))
 				error = GIT_EEXISTS;
 
 			git_buf_free(&path);
@@ -846,7 +846,8 @@ int git_submodule_reload_all(git_repository *repo, int force)
 	if (repo->submodules)
 		git_strmap_foreach_value(repo->submodules, sm, { sm->flags = 0; });
 
-	error = load_submodule_config(repo, true);
+	if ((error = load_submodule_config(repo, true)) < 0)
+		return error;
 
 	git_strmap_foreach_value(repo->submodules, sm, {
 		git_strmap *cache = repo->submodules;
@@ -1278,14 +1279,17 @@ static int submodule_load_from_config(
 		}
 	}
 
+	/* Found a alternate key for the submodule */
 	if (alternate) {
 		void *old_sm = NULL;
 		git_strmap_insert2(smcfg, alternate, sm, old_sm, error);
 
 		if (error < 0)
 			goto done;
-		if (error >= 0)
-			GIT_REFCOUNT_INC(sm); /* inserted under a new key */
+		if (error > 0)
+			error = 0;
+
+		GIT_REFCOUNT_INC(sm); /* increase refcount for new key */
 
 		/* if we replaced an old module under this key, release the old one */
 		if (old_sm && ((git_submodule *)old_sm) != sm) {
diff --git a/tests/submodule/lookup.c b/tests/submodule/lookup.c
index cc29b11b2..36bde4f6e 100644
--- a/tests/submodule/lookup.c
+++ b/tests/submodule/lookup.c
@@ -141,3 +141,36 @@ void test_submodule_lookup__lookup_even_with_missing_index(void)
 	refute_submodule_exists(g_repo, "just_a_file", GIT_ENOTFOUND);
 	refute_submodule_exists(g_repo, "no_such_file", GIT_ENOTFOUND);
 }
+
+void test_submodule_lookup__just_added(void)
+{
+	git_submodule *sm;
+
+	cl_git_pass(git_submodule_add_setup(&sm, g_repo, "https://github.com/libgit2/libgit2.git", "sm_just_added", 1));
+	git_submodule_free(sm);
+	assert_submodule_exists(g_repo, "sm_just_added");
+
+	cl_git_pass(git_submodule_add_setup(&sm, g_repo, "https://github.com/libgit2/libgit2.git", "sm_just_added_2", 1));
+	assert_submodule_exists(g_repo, "sm_just_added_2");
+	git_submodule_free(sm);
+
+	cl_git_append2file("submod2/.gitmodules", "\n[submodule \"mismatch_name\"]\n\tpath = mismatch_path\n\turl = https://example.com/example.git\n\n");
+
+	cl_git_pass(git_submodule_reload_all(g_repo, 1));
+
+	assert_submodule_exists(g_repo, "mismatch_name");
+	assert_submodule_exists(g_repo, "mismatch_path");
+
+	assert_submodule_exists(g_repo, "sm_just_added");
+	assert_submodule_exists(g_repo, "sm_just_added_2");
+
+	/* all the regular ones should still be working right, too */
+
+	assert_submodule_exists(g_repo, "sm_unchanged");
+	assert_submodule_exists(g_repo, "sm_added_and_uncommited");
+	assert_submodule_exists(g_repo, "sm_gitmodules_only");
+	refute_submodule_exists(g_repo, "not-submodule", GIT_EEXISTS);
+	refute_submodule_exists(g_repo, "just_a_dir", GIT_ENOTFOUND);
+	refute_submodule_exists(g_repo, "just_a_file", GIT_ENOTFOUND);
+	refute_submodule_exists(g_repo, "no_such_file", GIT_ENOTFOUND);
+}
diff --git a/tests/submodule/nosubs.c b/tests/submodule/nosubs.c
new file mode 100644
index 000000000..5ef4f42ab
--- /dev/null
+++ b/tests/submodule/nosubs.c
@@ -0,0 +1,95 @@
+/* test the submodule APIs on repositories where there are no submodules */
+
+#include "clar_libgit2.h"
+#include "posix.h"
+
+void test_submodule_nosubs__cleanup(void)
+{
+	cl_git_sandbox_cleanup();
+}
+
+void test_submodule_nosubs__lookup(void)
+{
+	git_repository *repo = cl_git_sandbox_init("status");
+	git_submodule *sm = NULL;
+
+	p_mkdir("status/subrepo", 0777);
+	cl_git_mkfile("status/subrepo/.git", "gitdir: ../.git");
+
+	cl_assert_equal_i(GIT_ENOTFOUND, git_submodule_lookup(&sm, repo, "subdir"));
+
+	cl_assert_equal_i(GIT_EEXISTS, git_submodule_lookup(&sm, repo, "subrepo"));
+
+	cl_git_pass(git_submodule_reload_all(repo, 0));
+
+	cl_assert_equal_i(GIT_ENOTFOUND, git_submodule_lookup(&sm, repo, "subdir"));
+
+	cl_assert_equal_i(GIT_EEXISTS, git_submodule_lookup(&sm, repo, "subrepo"));
+}
+
+void test_submodule_nosubs__immediate_reload(void)
+{
+	git_repository *repo = cl_git_sandbox_init("status");
+	cl_git_pass(git_submodule_reload_all(repo, 0));
+}
+
+static int fake_submod_cb(git_submodule *sm, const char *n, void *p)
+{
+	GIT_UNUSED(sm); GIT_UNUSED(n); GIT_UNUSED(p);
+	return 0;
+}
+
+void test_submodule_nosubs__foreach(void)
+{
+	git_repository *repo = cl_git_sandbox_init("status");
+	cl_git_pass(git_submodule_foreach(repo, fake_submod_cb, NULL));
+}
+
+void test_submodule_nosubs__add(void)
+{
+	git_repository *repo = cl_git_sandbox_init("status");
+	git_submodule *sm, *sm2;
+
+	cl_git_pass(git_submodule_add_setup(&sm, repo, "https://github.com/libgit2/libgit2.git", "submodules/libgit2", 1));
+
+	cl_git_pass(git_submodule_lookup(&sm2, repo, "submodules/libgit2"));
+	git_submodule_free(sm2);
+
+	cl_git_pass(git_submodule_foreach(repo, fake_submod_cb, NULL));
+	cl_git_pass(git_submodule_reload_all(repo, 0));
+
+	git_submodule_free(sm);
+}
+
+void test_submodule_nosubs__reload_add_reload(void)
+{
+	git_repository *repo = cl_git_sandbox_init("status");
+	git_submodule *sm;
+
+	cl_git_pass(git_submodule_reload_all(repo, 0));
+
+	cl_git_pass(git_submodule_add_setup(&sm, repo, "https://github.com/libgit2/libgit2.git", "submodules/libgit2", 1));
+
+	cl_git_pass(git_submodule_reload_all(repo, 0));
+
+	cl_assert_equal_s("submodules/libgit2", git_submodule_name(sm));
+	git_submodule_free(sm);
+
+	cl_git_pass(git_submodule_lookup(&sm, repo, "submodules/libgit2"));
+	cl_assert_equal_s("submodules/libgit2", git_submodule_name(sm));
+	git_submodule_free(sm);
+}
+
+void test_submodule_nosubs__bad_gitmodules(void)
+{
+	git_repository *repo = cl_git_sandbox_init("status");
+
+	cl_git_mkfile("status/.gitmodules", "[submodule \"foobar\"]\tpath=blargle\n\turl=\n\tbranch=\n\tupdate=flooble\n\n");
+	cl_git_fail(git_submodule_reload_all(repo, 0));
+
+	cl_git_rewritefile("status/.gitmodules", "[submodule \"foobar\"]\tpath=blargle\n\turl=\n\tbranch=\n\tupdate=rebase\n\n");
+	cl_git_pass(git_submodule_reload_all(repo, 0));
+
+	cl_git_pass(git_submodule_lookup(NULL, repo, "foobar"));
+	cl_assert_equal_i(GIT_ENOTFOUND, git_submodule_lookup(NULL, repo, "subdir"));
+}