system_path(): always return free'able memory to the caller

The function sometimes returns a newly allocated string and
sometimes returns a borrowed string, the latter of which the callers
must not free().  The existing callers all assume that the return
value belongs to the callee and most of them copy it with strdup()
when they want to keep it around.  They end up leaking the returned
copy when the callee returned a new string because they cannot tell
if they should free it.

Change the contract between the callers and system_path() to make
the returned string owned by the callers; they are responsible for
freeing it when done, but they do not have to make their own copy to
store it away.

Adjust the callers to make sure they do not leak the returned string
once they are done, but do not bother freeing it just before dying,
exiting or exec'ing other program to avoid unnecessary churn.

Reported-by: Alexander Kuleshov <kuleshovmail@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/help.c b/builtin/help.c
index 1fdefeb6867cdd37beaf41c5fa9e113576b19f17..76fbfe8b48467132347695efb4f2c1935198dc0a 100644 (file)
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -322,16 +322,18 @@ static void setup_man_path(void)
 {
        struct strbuf new_path = STRBUF_INIT;
        const char *old_path = getenv("MANPATH");
 {
        struct strbuf new_path = STRBUF_INIT;
        const char *old_path = getenv("MANPATH");

+       char *git_man_path = system_path(GIT_MAN_PATH);

 
        /* We should always put ':' after our path. If there is no
         * old_path, the ':' at the end will let 'man' to try
         * system-wide paths after ours to find the manual page. If
         * there is old_path, we need ':' as delimiter. */
 
        /* We should always put ':' after our path. If there is no
         * old_path, the ':' at the end will let 'man' to try
         * system-wide paths after ours to find the manual page. If
         * there is old_path, we need ':' as delimiter. */

-       strbuf_addstr(&new_path, system_path(GIT_MAN_PATH));
+       strbuf_addstr(&new_path, git_man_path);

        strbuf_addch(&new_path, ':');
        if (old_path)
                strbuf_addstr(&new_path, old_path);
 
        strbuf_addch(&new_path, ':');
        if (old_path)
                strbuf_addstr(&new_path, old_path);
 

+       free(git_man_path);

        setenv("MANPATH", new_path.buf, 1);
 
        strbuf_release(&new_path);
        setenv("MANPATH", new_path.buf, 1);
 
        strbuf_release(&new_path);


@@ -381,8 +383,10 @@ static void show_info_page(const char *git_cmd)
 static void get_html_page_path(struct strbuf *page_path, const char *page)
 {
        struct stat st;
 static void get_html_page_path(struct strbuf *page_path, const char *page)
 {
        struct stat st;

+       char *to_free = NULL;
+

        if (!html_path)
        if (!html_path)

-               html_path = system_path(GIT_HTML_PATH);
+               html_path = to_free = system_path(GIT_HTML_PATH);

 
        /* Check that we have a git documentation directory. */
        if (!strstr(html_path, "://")) {
 
        /* Check that we have a git documentation directory. */
        if (!strstr(html_path, "://")) {


@@ -393,6 +397,7 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)
 
        strbuf_init(page_path, 0);
        strbuf_addf(page_path, "%s/%s.html", html_path, page);
 
        strbuf_init(page_path, 0);
        strbuf_addf(page_path, "%s/%s.html", html_path, page);

+       free(to_free);

 }
 
 /*
 }
 
 /*

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 56f85e239ae0d2607c38e3c46013007ba9e71b12..86c8a30a3125d18ef8cf609ac6670bc5f10f5e51 100644 (file)
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -119,15 +119,18 @@ static void copy_templates(const char *template_dir)
        DIR *dir;
        const char *git_dir = get_git_dir();
        int len = strlen(git_dir);
        DIR *dir;
        const char *git_dir = get_git_dir();
        int len = strlen(git_dir);

+       char *to_free = NULL;

 
        if (!template_dir)
                template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT);
        if (!template_dir)
                template_dir = init_db_template_dir;
        if (!template_dir)
 
        if (!template_dir)
                template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT);
        if (!template_dir)
                template_dir = init_db_template_dir;
        if (!template_dir)

-               template_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);
-       if (!template_dir[0])
+               template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR);
+       if (!template_dir[0]) {
+               free(to_free);

                return;
                return;

+       }

        template_len = strlen(template_dir);
        if (PATH_MAX <= (template_len+strlen("/config")))
                die(_("insanely long template path %s"), template_dir);
        template_len = strlen(template_dir);
        if (PATH_MAX <= (template_len+strlen("/config")))
                die(_("insanely long template path %s"), template_dir);


@@ -139,7 +142,7 @@ static void copy_templates(const char *template_dir)
        dir = opendir(template_path);
        if (!dir) {
                warning(_("templates not found %s"), template_dir);
        dir = opendir(template_path);
        if (!dir) {
                warning(_("templates not found %s"), template_dir);

-               return;
+               goto free_return;

        }
 
        /* Make sure that template is from the correct vintage */
        }
 
        /* Make sure that template is from the correct vintage */


@@ -155,8 +158,7 @@ static void copy_templates(const char *template_dir)
                        "a wrong format version %d from '%s'"),
                        repository_format_version,
                        template_dir);
                        "a wrong format version %d from '%s'"),
                        repository_format_version,
                        template_dir);

-               closedir(dir);
-               return;
+               goto close_free_return;

        }
 
        memcpy(path, git_dir, len);
        }
 
        memcpy(path, git_dir, len);


@@ -166,7 +168,10 @@ static void copy_templates(const char *template_dir)
        copy_templates_1(path, len,
                         template_path, template_len,
                         dir);
        copy_templates_1(path, len,
                         template_path, template_len,
                         dir);

+close_free_return:

        closedir(dir);
        closedir(dir);

+free_return:
+       free(to_free);

 }
 
 static int git_init_db_config(const char *k, const char *v, void *cb)
 }
 
 static int git_init_db_config(const char *k, const char *v, void *cb)

diff --git a/exec_cmd.c b/exec_cmd.c
index 125fa6fabf503d29cb82b2ccbc92359abf95b0e8..26ebef6686bec26ff28bbc1492f2af5362910703 100644 (file)
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -6,7 +6,7 @@
 static const char *argv_exec_path;
 static const char *argv0_path;
 
 static const char *argv_exec_path;
 static const char *argv0_path;
 

-const char *system_path(const char *path)
+char *system_path(const char *path)

 {
 #ifdef RUNTIME_PREFIX
        static const char *prefix;
 {
 #ifdef RUNTIME_PREFIX
        static const char *prefix;


@@ -16,7 +16,7 @@ const char *system_path(const char *path)
        struct strbuf d = STRBUF_INIT;
 
        if (is_absolute_path(path))
        struct strbuf d = STRBUF_INIT;
 
        if (is_absolute_path(path))

-               return path;
+               return xstrdup(path);

 
 #ifdef RUNTIME_PREFIX
        assert(argv0_path);
 
 #ifdef RUNTIME_PREFIX
        assert(argv0_path);


@@ -34,8 +34,7 @@ const char *system_path(const char *path)
 #endif
 
        strbuf_addf(&d, "%s/%s", prefix, path);
 #endif
 
        strbuf_addf(&d, "%s/%s", prefix, path);

-       path = strbuf_detach(&d, NULL);
-       return path;
+       return strbuf_detach(&d, NULL);

 }
 
 const char *git_extract_argv0_path(const char *argv0)
 }
 
 const char *git_extract_argv0_path(const char *argv0)

diff --git a/exec_cmd.h b/exec_cmd.h
index e4c9702f02858973096f6ef0f41e5a572ff4d0db..93b0c02529a854af4ccb648ea0d0571c5495b63a 100644 (file)
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -9,6 +9,6 @@ extern const char **prepare_git_cmd(const char **argv);
 extern int execv_git_cmd(const char **argv); /* NULL terminated */
 LAST_ARG_MUST_BE_NULL
 extern int execl_git_cmd(const char *cmd, ...);
 extern int execv_git_cmd(const char **argv); /* NULL terminated */
 LAST_ARG_MUST_BE_NULL
 extern int execl_git_cmd(const char *cmd, ...);

-extern const char *system_path(const char *path);
+extern char *system_path(const char *path);

 
 #endif /* GIT_EXEC_CMD_H */
 
 #endif /* GIT_EXEC_CMD_H */