8000 Memory leak in git_repository_set_workdir(for unhappy paths and when resetting the same working dir) · Issue #6746 · libgit2/libgit2 · GitHub
[go: up one dir, main page]
Skip to content

Memory leak in git_repository_set_workdir(for unhappy paths and when resetting the same working dir) #6746

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
lucanus81 opened this issue Feb 23, 2024 · 0 comments · Fixed by #6748
Closed

Memory leak in git_repository_set_workdir(for unhappy paths and when resetting the same working dir) #6746

lucanus81 opened this issue Feb 23, 2024 · 0 comments · Fixed by #6748

Comments

@lucanus81
Copy link
lucanus81 commented Feb 23, 2024 •
edited
Loading

Reproduction steps

  1. Create an empty folder with an empty git repository (in my case I had simply mkdir /tmp/luca123 && cd /tmp/luca123 && git init)
  2. 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;
}
  1. 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
@lucanus81 lucanus81 changed the title Memory leak in git_repository_set_workdir(0 for unhappy paths Memory leak in git_repository_set_workdir(for unhappy paths) Feb 23, 2024
@lucanus81 lucanus81 changed the title Memory leak in git_repository_set_workdir(for unhappy paths) Memory leak in git_repository_set_workdir(for unhappy paths and when resetting the same working dir) 60F4 Feb 24, 2024
@csware csware mentioned this issue Feb 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix memory leaks csware/libgit2
1 participant
@lucanus81
0