Memory leak in git_repository_set_workdir(for unhappy paths and when resetting the same working dir) #6746
Description
Reproduction steps
- Create an empty folder with an empty git repository (in my case I had simply mkdir /tmp/luca123 && cd /tmp/luca123 && git init)
- Build the following program:
I am using:clang++ -g -I /usr/local/include -L /Users/lstoppa/dev/libgitdevelop/build -lgit2 -fsanitize=leak leak.cpp
lstoppa@C02DL3AAMD6R libgitdevelop % cat build/leak.cpp
#include "git2.h"
#include <iostream>
void log_error() {
const git_error* err=::git_error_last();
std::cout <<err->klass <<": " <<err->message <<'\n';
}
int main(int argc, char** argv) {
::git_libgit2_init();
git_repository* r{nullptr};
if (::git_repository_open(&r, argv[1]) != GIT_OK) {
log_error();
return -1;
}
if (::git_repository_set_workdir(r, "/tmp", 1) != GIT_OK)
log_error();
::git_repository_free(r);
::git_libgit2_shutdown();
return 0;
}
- Run it and you'll see a leak:
=================================================================
==38332==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 24 byte(s) in 1 object(s) allocated from:
#0 0x10b453cca in realloc+0x5a (libclang_rt.lsan_osx_dynamic.dylib:x86_64h+0x33cca)
#1 0x10b2358b6 in stdalloc__realloc stdalloc.c:33
#2 0x10b2487d0 in git__realloc alloc.h:29
#3 0x10b2486a2 in git_str_try_grow str.c:86
#4 0x10b248501 in git_str_grow str.c:110
#5 0x10b248a16 in git_str_set str.c:157
#6 0x10b248af8 in git_str_sets str.c:171
#7 0x10b23aca1 in git_fs_path_prettify fs_path.c:403
#8 0x10b23ad14 in git_fs_path_prettify_dir fs_path.c:408
#9 0x10b325c3a in git_repository_set_workdir repository.c:3246
#10 0x10aa14373 in main leak.cpp:16
#11 0x7ff8039cf385 in start+0x795 (dyld:x86_64+0xfffffffffff5c385)
Expected behavior
No memory leak reported.
Leak 1:
Actual behavior
There are several problems in git_repository_set_workdir().
If you try to call this function with the same path as the current working directory, namely when this code is true:
if (repo->workdir && strcmp(repo->workdir, path.ptr) == 0)
return 0;
you never free path (in my example program you would only need call `::git_repository_set_workdir(r, argv[1]),1).
Leak 2:
When this code fails, again we don't free path
if (git_repository_config__weakptr(&config, repo) < 0)
return -1;
Leak 3:
Whenever we have the variable error set
if (!error) {
char *old_workdir = repo->workdir;
repo->workdir = git_str_detach(&path);
repo->is_bare = 0;
git__free(old_workdir);
}
is never executed, but still we never really free path.
Leak 4:
when you call twice git_repository_set_workdir() with the same path. This happens because we allocate the new string, and then we check that the current working dir and the one we want to set are the same (with strcmp) and we return true.
Possible Leak 5:
I am not sure what happens to path when git_fs_path_prettify_dir fails. Do we have a leak? Couldn't really reproduce it.
Anyway, the way I trigger this issue was by chance, and it seems to be related to permission issues: /private/tmp is owned by root.
lstoppa@C02DL3AAMD6R build % ./a.out /tmp/luca123
6: cannot overwrite gitlink file into path '/private/tmp/'
I don't know exactly how to create a patch, but this is the diff file that works fine for all test cases with update_link=true (false doesn't have any problem).
lstoppa@C02DL3AAMD6R libgitdevelop % git diff
diff --git a/src/libgit2/repository.c b/src/libgit2/repository.c
index 4eb344913..7d5444f91 100644
--- a/src/libgit2/repository.c
+++ b/src/libgit2/repository.c
@@ -3246,14 +3246,18 @@ int git_repository_set_workdir(
if (git_fs_path_prettify_dir(&path, workdir, NULL) < 0)
return -1;
- if (repo->workdir && strcmp(repo->workdir, path.ptr) == 0)
+ if (repo->workdir && strcmp(repo->workdir, path.ptr) == 0) {
+ git_str_dispose(&path);
return 0;
+ }
if (update_gitlink) {
git_config *config;
- if (git_repository_config__weakptr(&config, repo) < 0)
+ if (git_repository_config__weakptr(&config, repo) < 0) {
+ git_str_dispose(&path);
return -1;
+ }
error = repo_write_gitlink(path.ptr, git_repository_path(repo), false);
@@ -3274,7 +3278,9 @@ int git_repository_set_workdir(
repo->is_bare = 0;
git__free(old_workdir);
- }
+ } else {
+ git_str_dispose(&path);
+ }
return error;
}
Version of libgit2 (release number or SHA1)
Develop
Operating system(s) tested
macos/linux