Fix bugs related to real_path()
cover Fix bugs related to real_path()
The issue with real_path() seems to be long-standing, where multiple people solved parts of it over time. I'm adding another part here after I have discovered a crash related to it.
Even with this step, there are still problems remaining:
* read_gitfile_gently() still uses shared buffer.
* absolute_path() was not removed.
These issues remain because there're too many code references and I'd like to avoid submitting a single topic of a scary size.
Alexandr Miloslavskiy (4):
set_git_dir: fix crash when used with real_path()
real_path: remove unsafe API real_path_if_valid(): remove unsafe API get_superproject_working_tree(): return strbuf
abspath.c | 18 +----------------- builtin/clone.c | 7 ++++++- builtin/commit-graph.c | 6 +++++- builtin/init-db.c | 4 ++-- builtin/rev-parse.c | 12 ++++++++---- builtin/worktree.c | 10 +++++++--- cache.h | 4 +--- editor.c | 11 +++++++++-- environment.c | 18 ++++++++++++++++-- path.c | 4 ++-- setup.c | 37 ++++++++++++++++++++++++------------- sha1-file.c | 13 ++++--------- submodule.c | 22 ++++++++++++---------- submodule.h | 4 ++-- t/helper/test-path-utils.c | 6 +++++- worktree.c | 13 ++++++++++--- 16 files changed, 114 insertions(+), 75 deletions(-)
base-commit: 076cbdcd739aeb33c1be87b73aebae5e43d7bcc5
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-575%2FSyntevoAlex%2F%230205(git)_crash_real_path-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-575/SyntevoAlex/#0205(git)_crash_real_path-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/575
Signature · 1 line
gitgitgadget
PATCH 1/4 set_git_dir: fix crash when used with real_path()
From: Alexandr Miloslavskiy <[email protected]>
real_path() returns result from a shared buffer, inviting subtle reentrance bugs. One of these bugs occur when invoked this way:
set_git_dir(real_path(git_dir))
In this case, real_path() has reentrance:
real_path
read_gitfile_gently
repo_set_gitdir
setup_git_env
set_git_dir_1
set_git_dirLater, set_git_dir() uses its now-dead parameter:
!is_absolute_path(path)
Fix this by using a dedicated strbuf to hold strbuf_realpath().
Signed-off-by Alexandr Miloslavskiy <[email protected]>
--- builtin/init-db.c | 4 ++-- cache.h | 2 +- environment.c | 11 ++++++++++- path.c | 2 +- setup.c | 18 +++++++++--------- 5 files changed, 23 insertions(+), 14 deletions(-)
builtin/init-db.c +2 -2
@@ -356,12 +356,12 @@ int init_db(const char *git_dir, const char *real_git_dir,if (!exist_ok && !stat (real_git_dir, &st))die (_ ("%s already exists" ), real_git_dir); -set_git_dir (real_path (real_git_dir)); +set_git_dir (real_git_dir,1 ); git_dir =get_git_dir ();separate_git_dir (git_dir, original_git_dir); }else { -set_git_dir (real_path (git_dir)); +set_git_dir (git_dir,1 ); git_dir =get_git_dir (); } startup_info->have_repository =1 ;
cache.h +1 -1
@@ -543,7 +543,7 @@ const char *get_git_common_dir(void);char *get_object_directory (void );char *get_index_file (void );char *get_graft_file (struct repository *r); -void set_git_dir (const char *path); +void set_git_dir (const char *path,int make_realpath);int get_common_dir_noenv (struct strbuf *sb,const char *gitdir);int get_common_dir (struct strbuf *sb,const char *gitdir);const char *get_git_namespace (void );
environment.c +10 -1
@@ -345,11 +345,20 @@ static void update_relative_gitdir(const char *name,free (path); } -void set_git_dir (const char *path) +void set_git_dir (const char *path,int make_realpath) { +struct strbuf realpath =STRBUF_INIT ; + +if (make_realpath) { +strbuf_realpath (&realpath, path,1 ); + path = realpath.buf; + } +set_git_dir_1 (path);if (!is_absolute_path (path))chdir_notify_register (NULL , update_relative_gitdir,NULL ); + +strbuf_release (&realpath); }const char *get_log_output_encoding (void )
path.c +1 -1
@@ -850,7 +850,7 @@ const char *enter_repo(const char *path, int strict) }if (is_git_directory ("." )) { -set_git_dir ("." ); +set_git_dir ("." ,0 );check_repository_format ();return path; }
setup.c +9 -9
@@ -725,7 +725,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv, }/* #18, #26 */ -set_git_dir (gitdirenv); +set_git_dir (gitdirenv,0 );free (gitfile);return NULL ; }
@@ -747,7 +747,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv, }else if (!git_env_bool (GIT_IMPLICIT_WORK_TREE_ENVIRONMENT ,1 )) {/* #16d */ -set_git_dir (gitdirenv); +set_git_dir (gitdirenv,0 );free (gitfile);return NULL ; }
@@ -759,14 +759,14 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,/* both get_git_work_tree() and cwd are already normalized */ if (!strcmp (cwd->buf, worktree)) {/* cwd == worktree */ -set_git_dir (gitdirenv); +set_git_dir (gitdirenv,0 );free (gitfile);return NULL ; } offset =dir_inside_of (cwd->buf, worktree);if (offset >=0 ) {/* cwd inside worktree? */ -set_git_dir (real_path (gitdirenv)); +set_git_dir (gitdirenv,1 );if (chdir (worktree))die_errno (_ ("cannot chdir to '%s'" ), worktree);strbuf_addch (cwd,'/' );
@@ -775,7 +775,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv, }/* cwd outside worktree */ -set_git_dir (gitdirenv); +set_git_dir (gitdirenv,0 );free (gitfile);return NULL ; }
@@ -804,7 +804,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */ if (is_bare_repository_cfg >0 ) { -set_git_dir (offset == cwd->len ? gitdir :real_path (gitdir)); +set_git_dir (gitdir, (offset != cwd->len));if (chdir (cwd->buf))die_errno (_ ("cannot come back to cwd" ));return NULL ;
@@ -813,7 +813,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,/* #0, #1, #5, #8, #9, #12, #13 */ set_git_work_tree ("." );if (strcmp (gitdir,DEFAULT_GIT_DIR_ENVIRONMENT )) -set_git_dir (gitdir); +set_git_dir (gitdir,0 ); inside_git_dir =0 ; inside_work_tree =1 ;if (offset >= cwd->len)
@@ -856,10 +856,10 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,die_errno (_ ("cannot come back to cwd" )); root_len =offset_1st_component (cwd->buf);strbuf_setlen (cwd, offset > root_len ? offset : root_len); -set_git_dir (cwd->buf); +set_git_dir (cwd->buf,0 ); }else -set_git_dir ("." ); +set_git_dir ("." ,0 );return NULL ; }
Signature · 1 line
gitgitgadget
Re: [PATCH 1/4] set_git_dir: fix crash when used with real_path()
"Alexandr Miloslavskiy via GitGitGadget" <[email protected]>
Quoted 14 lines · writes:
From: Alexandr Miloslavskiy <[email protected]>
real_path()returns result from a shared buffer, inviting subtle reentrance bugs. One of these bugs occur when invoked this way:set_git_dir(real_path(git_dir))In this case,
real_path()has reentrance:real_path read_gitfile_gently repo_set_gitdir setup_git_env set_git_dir_1 set_git_dir
Quoted 4 lines
Later,
set_git_dir()uses its now-dead parameter:
!is_absolute_path(path)Fix this by using a dedicated
strbufto holdstrbuf_realpath().
With this detailed explanation, I expected to see a test or two that demonstrates a breakage, but reading a stale value may not reproducibly give the same wrong result or crash the program, perhaps?
Quoted 6 lines
environment.c +4
-1-void set_git_dir (const char *path) +void set_git_dir (const char *path,int make_realpath) { +struct strbuf realpath =STRBUF_INIT ; + +if (make_realpath) {
environment.c static void update_relative_gitdir(const char *name, · line 362
Quoted 10 lines
environment.c +6
-0+strbuf_realpath (&realpath, path,1 ); + path = realpath.buf; + } +set_git_dir_1 (path);if (!is_absolute_path (path))chdir_notify_register (NULL , update_relative_gitdir,NULL ); + +strbuf_release (&realpath); }
Makes sense. I looked at changes to the callers in this patch and it all made sense.
Re: [PATCH 1/4] set_git_dir: fix crash when used with real_path()
Quoted 4 lines · Junio C Hamano
With this detailed explanation, I expected to see a test or two that demonstrates a breakage, but reading a stale value may not reproducibly give the same wrong result or crash the program, perhaps?
Let's put it this way: one of the tests hits the bug every single time, yet still the bug has gone unnoticed for years. So yes, it's not super reliable. I think I could make a test that crashes often enough, but the effort will probably not be justified. The problem here is rather apparent when a finger is pointed to it.
PATCH 2/4 real_path: remove unsafe API
In reply to Alexandr Miloslavskiy via GitGitGadgetFrom: Alexandr Miloslavskiy <[email protected]>
Returning a shared buffer invites very subtle bugs due to reentrancy or multi-threading, as demonstrated by the previous patch.
There was an unfinished effort to abolish this [1].
Let's finally rid of real_path(), using strbuf_realpath() instead.
This patch uses a local strbuf for most places where real_path() was previously called.
However, two places return the value of real_path() to the caller. For them, a static local strbuf was added, effectively pushing the problem one level higher:
read_gitfile_gently()
get_superproject_working_tree()
[1] https://lore.kernel.org/git/[email protected]/
Signed-off-by Alexandr Miloslavskiy <[email protected]>
--- abspath.c | 8 +------- builtin/clone.c | 7 ++++++- builtin/commit-graph.c | 6 +++++- builtin/rev-parse.c | 5 ++++- builtin/worktree.c | 10 +++++++--- cache.h | 1 - editor.c | 11 +++++++++-- environment.c | 7 ++++++- path.c | 2 +- setup.c | 17 ++++++++++++++--- submodule.c | 4 +++- t/helper/test-path-utils.c | 6 +++++- worktree.c | 5 ++++- 13 files changed, 65 insertions(+), 24 deletions(-)
abspath.c +1 -7
@@ -206,12 +206,6 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,* Resolve `path` into an absolute, cleaned-up path. The return value * comes from a shared buffer. */ -const char *real_path (const char *path) -{ -static struct strbuf realpath =STRBUF_INIT ; -return strbuf_realpath (&realpath, path,1 ); -} -const char *real_path_if_valid (const char *path) {static struct strbuf realpath =STRBUF_INIT ;
@@ -233,7 +227,7 @@ char *real_pathdup(const char *path, int die_on_error)/* * Use this to get an absolute path from a relative one. If you want -* to resolve links, you should use real_path. +* to resolve links, you should use strbuf_realpath. */ const char *absolute_path (const char *path) {
builtin/clone.c +6 -1
@@ -420,6 +420,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,struct dir_iterator *iter;int iter_status;unsigned int flags; +struct strbuf realpath =STRBUF_INIT ;mkdir_if_missing (dest->buf,0777 );
@@ -454,7 +455,9 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,if (unlink (dest->buf) && errno !=ENOENT )die_errno (_ ("failed to unlink '%s'" ), dest->buf);if (!option_no_hardlinks) { -if (!link (real_path (src->buf), dest->buf)) +strbuf_reset (&realpath); +strbuf_realpath (&realpath, src->buf,1 ); +if (!link (realpath.buf, dest->buf))continue ;if (option_local >0 )die_errno (_ ("failed to create link '%s'" ), dest->buf);
@@ -468,6 +471,8 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,strbuf_setlen (src, src_len);die (_ ("failed to iterate over '%s'" ), src->buf); } + +strbuf_release (&realpath); }static void clone_local (const char *src_repo,const char *dest_repo)
builtin/commit-graph.c +5 -1
@@ -39,14 +39,18 @@ static struct object_directory *find_odb(struct repository *r, {struct object_directory *odb;char *obj_dir_real =real_pathdup (obj_dir,1 ); +struct strbuf odb_path_real =STRBUF_INIT ;prepare_alt_odb (r);for (odb = r->objects->odb; odb; odb = odb->next) { -if (!strcmp (obj_dir_real,real_path (odb->path))) +strbuf_reset (&odb_path_real); +strbuf_realpath (&odb_path_real, odb->path,1 ); +if (!strcmp (obj_dir_real, odb_path_real.buf))break ; }free (obj_dir_real); +strbuf_release (&odb_path_real);if (!odb)die (_ ("could not find object directory matching %s" ), obj_dir);
builtin/rev-parse.c +4 -1
@@ -857,7 +857,10 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)if (!gitdir && !prefix) gitdir =".git" ;if (gitdir) { -puts (real_path (gitdir)); +struct strbuf realpath =STRBUF_INIT ; +strbuf_realpath (&realpath, gitdir,1 ); +puts (realpath.buf); +strbuf_release (&realpath);continue ; } }
builtin/worktree.c +7 -3
@@ -258,7 +258,7 @@ static int add_worktree(const char *path, const char *refname, const struct add_opts *opts) {struct strbuf sb_git =STRBUF_INIT , sb_repo =STRBUF_INIT ; -struct strbuf sb =STRBUF_INIT ; +struct strbuf sb =STRBUF_INIT , realpath =STRBUF_INIT ;const char *name;struct child_process cp =CHILD_PROCESS_INIT ;struct argv_array child_env =ARGV_ARRAY_INIT ;
@@ -330,9 +330,12 @@ static int add_worktree(const char *path, const char *refname,strbuf_reset (&sb);strbuf_addf (&sb,"%s/gitdir" , sb_repo.buf); -write_file (sb.buf,"%s" ,real_path (sb_git.buf)); +strbuf_realpath (&realpath, sb_git.buf,1 ); +write_file (sb.buf,"%s" , realpath.buf); +strbuf_reset (&realpath); +strbuf_realpath (&realpath,get_git_common_dir (),1 );write_file (sb_git.buf,"gitdir: %s/worktrees/%s" , -real_path (get_git_common_dir ()), name); + realpath.buf, name);/* * This is to keep resolve_ref() happy. We need a valid HEAD * or is_git_directory() will reject the directory. Any value which
@@ -418,6 +421,7 @@ static int add_worktree(const char *path, const char *refname,strbuf_release (&sb_repo);strbuf_release (&sb_git);strbuf_release (&sb_name); +strbuf_release (&realpath);return ret; }
cache.h +0 -1
@@ -1314,7 +1314,6 @@ static inline int is_absolute_path(const char *path)int is_directory (const char *);char *strbuf_realpath (struct strbuf *resolved,const char *path,int die_on_error); -const char *real_path (const char *path);const char *real_path_if_valid (const char *path);char *real_pathdup (const char *path,int die_on_error);const char *absolute_path (const char *path);
editor.c +9 -2
@@ -54,7 +54,8 @@ static int launch_specified_editor(const char *editor, const char *path,return error ("Terminal is dumb, but EDITOR unset" );if (strcmp (editor,":" )) { -const char *args[] = { editor,real_path (path),NULL }; +struct strbuf realpath =STRBUF_INIT ; +const char *args[] = { editor,NULL ,NULL };struct child_process p =CHILD_PROCESS_INIT ;int ret, sig;int print_waiting_for_editor = advice_waiting_for_editor &&isatty (2 );
@@ -75,16 +76,22 @@ static int launch_specified_editor(const char *editor, const char *path,fflush (stderr); } +strbuf_realpath (&realpath, path,1 ); + args[1 ] = realpath.buf; + p.argv = args; p.env = env; p.use_shell =1 ; p.trace2_child_class ="editor" ; -if (start_command (&p) <0 ) +if (start_command (&p) <0 ) { +strbuf_release (&realpath);return error ("unable to start editor '%s'" , editor); + }sigchain_push (SIGINT ,SIG_IGN );sigchain_push (SIGQUIT ,SIG_IGN ); ret =finish_command (&p); +strbuf_release (&realpath); sig = ret -128 ;sigchain_pop (SIGINT );sigchain_pop (SIGQUIT );
environment.c +6 -1
@@ -254,8 +254,11 @@ static int git_work_tree_initialized;*/ void set_git_work_tree (const char *new_work_tree) { +struct strbuf realpath =STRBUF_INIT ; +if (git_work_tree_initialized) { - new_work_tree =real_path (new_work_tree); +strbuf_realpath (&realpath, new_work_tree,1 ); + new_work_tree = realpath.buf;if (strcmp (new_work_tree, the_repository->worktree)) die("internal error: work tree has already been set\n" "Current worktree: %s\nNew worktree: %s",
@@ -264,6 +267,8 @@ void set_git_work_tree(const char *new_work_tree) } git_work_tree_initialized =1 ;repo_set_worktree (the_repository, new_work_tree); + +strbuf_release (&realpath); }const char *get_git_work_tree (void )
path.c +1 -1
@@ -723,7 +723,7 @@ static struct passwd *getpw_str(const char *username, size_t len)* then it is a newly allocated string. Returns NULL on getpw failure or * if path is NULL. * -* If real_home is true, real_path($HOME) is used in the expansion. +* If real_home is true, strbuf_realpath($HOME) is used in the expansion. */ char *expand_user_path (const char *path,int real_home) {
setup.c +14 -3
@@ -32,6 +32,7 @@ static int abspath_part_inside_repo(char *path)char *path0;int off;const char *work_tree =get_git_work_tree (); +struct strbuf realpath =STRBUF_INIT ;if (!work_tree)return -1 ;
@@ -60,8 +61,11 @@ static int abspath_part_inside_repo(char *path) path++;if (*path =='/' ) { *path ='\0' ; -if (fspathcmp (real_path (path0), work_tree) ==0 ) { +strbuf_reset (&realpath); +strbuf_realpath (&realpath, path0,1 ); +if (fspathcmp (realpath.buf, work_tree) ==0 ) {memmove (path0, path +1 , len - (path - path0)); +strbuf_release (&realpath);return 0 ; } *path ='/' ;
@@ -69,11 +73,15 @@ static int abspath_part_inside_repo(char *path) }/* check whole path */ -if (fspathcmp (real_path (path0), work_tree) ==0 ) { +strbuf_reset (&realpath); +strbuf_realpath (&realpath, path0,1 ); +if (fspathcmp (realpath.buf, work_tree) ==0 ) { *path0 ='\0' ; +strbuf_release (&realpath);return 0 ; } +strbuf_release (&realpath);return -1 ; }
@@ -619,6 +627,7 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)struct stat st;int fd;ssize_t len; +static struct strbuf realpath =STRBUF_INIT ;if (stat (path, &st)) {/* NEEDSWORK: discern between ENOENT vs other errors */
@@ -669,7 +678,9 @@ const char *read_gitfile_gently(const char *path, int *return_error_code) error_code =READ_GITFILE_ERR_NOT_A_REPO ; goto cleanup_return; } - path =real_path (dir); + +strbuf_realpath (&realpath, dir,1 ); + path = realpath.buf; cleanup_return:if (return_error_code)
submodule.c +3 -1
@@ -2170,6 +2170,7 @@ void absorb_git_dir_into_superproject(const char *path,const char *get_superproject_working_tree (void ) { +static struct strbuf realpath =STRBUF_INIT ;struct child_process cp =CHILD_PROCESS_INIT ;struct strbuf sb =STRBUF_INIT ;const char *one_up =real_path_if_valid ("../" );
@@ -2231,7 +2232,8 @@ const char *get_superproject_working_tree(void) super_wt =xstrdup (cwd); super_wt[cwd_len - super_sub_len] ='\0' ; - ret =real_path (super_wt); +strbuf_realpath (&realpath, super_wt,1 ); + ret = realpath.buf;free (super_wt); }strbuf_release (&sb);
t/helper/test-path-utils.c +5 -1
@@ -290,11 +290,15 @@ int cmd__path_utils(int argc, const char **argv) }if (argc >=2 && !strcmp (argv[1 ],"real_path" )) { +struct strbuf realpath =STRBUF_INIT ;while (argc >2 ) { -puts (real_path (argv[2 ])); +strbuf_reset (&realpath); +strbuf_realpath (&realpath, argv[2 ],1 ); +puts (realpath.buf); argc--; argv++; } +strbuf_release (&realpath);return 0 ; }
worktree.c +4 -1
@@ -285,6 +285,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg, unsigned flags) {struct strbuf wt_path =STRBUF_INIT ; +struct strbuf realpath =STRBUF_INIT ;char *path =NULL ;int err, ret =-1 ;
@@ -336,7 +337,8 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg, goto done; } - ret =fspathcmp (path,real_path (git_common_path ("worktrees/%s" , wt->id))); +strbuf_realpath (&realpath,git_common_path ("worktrees/%s" , wt->id),1 ); + ret =fspathcmp (path, realpath.buf);if (ret) strbuf_addf_gently(errmsg, _("'%s' does not point back to '%s'"),
@@ -344,6 +346,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg, done:free (path);strbuf_release (&wt_path); +strbuf_release (&realpath);return ret; }
Signature · 1 line
gitgitgadget
Re: [PATCH 2/4] real_path: remove unsafe API
"Alexandr Miloslavskiy via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
However, two places return the value of
real_path()to the caller. For them, astaticlocalstrbufwas added, effectively pushing the problem one level higher:
read_gitfile_gently()
get_superproject_working_tree()
Yeah, I noticed that while reading the patch. It is not making it any worse, and other parts of the patch made tons of sense (except one small thing).
It was especially pleasing to see that care has been taken to avoid introducing strbuf leaks.
Quoted 13 lines
builtin/clone.c +1
-0@@ -420,6 +420,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,struct dir_iterator *iter;int iter_status;unsigned int flags; +struct strbuf realpath =STRBUF_INIT ;mkdir_if_missing (dest->buf,0777 );
builtin/clone.c static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest, · line 459
Quoted 6 lines
builtin/clone.c +2
-1@@ -454,7 +455,9 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,if (unlink (dest->buf) && errno !=ENOENT )die_errno (_ ("failed to unlink '%s'" ), dest->buf);if (!option_no_hardlinks) { -if (!link (real_path (src->buf), dest->buf)) +strbuf_reset (&realpath); +strbuf_realpath (&realpath, src->buf,1 );
This is inside a loop, so "struct strbuf realpath" here in the second or subsequent iteration may not be empty; it is true that strbuf_reset() is necessary _somewhere_ in the loop to discard the path that was created for the previous iteration.
If my reading of the code is correct, however, the first thing that is done by strbuf_realpath() is to empty the output buffer by using strbuf_reset() indirectly via get_root_part(). Calling strbuf_reset() here should not hurt, but it is unnecessary, I would think. An even worse effect such a redundant strbuf_reset() has is that by repeatedly seeing the "reset then call realpath" pattern, readers who do not read the implementation of strbuf_realpath() might mistakenly think that
strbuf_addf(&message, "the path '%s' is really ", path); strbuf_realpath(&message, path);
is how realpath() is expected to be used, i.e. keep the current contents in the buffer and append the resolved path to it.
Quoted 6 lines
builtin/clone.c +0
-0static void clone_local (const char *src_repo,const char *dest_repo)builtin/commit-graph.c +0
-0
builtin/commit-graph.c static struct object_directory *find_odb(struct repository *r, · line 47
Quoted 10 lines
builtin/commit-graph.c +3
-1@@ -39,14 +39,18 @@ static struct object_directory *find_odb(struct repository *r, {struct object_directory *odb;char *obj_dir_real =real_pathdup (obj_dir,1 ); +struct strbuf odb_path_real =STRBUF_INIT ;prepare_alt_odb (r);for (odb = r->objects->odb; odb; odb = odb->next) { -if (!strcmp (obj_dir_real,real_path (odb->path))) +strbuf_reset (&odb_path_real); +strbuf_realpath (&odb_path_real, odb->path,1 );
Likewise.
builtin/commit-graph.c static int abspath_part_inside_repo(char *path) · line 65
Quoted 7 lines
builtin/commit-graph.c +2
-1@@ -60,8 +61,11 @@ static int abspath_part_inside_repo(char *path) path++;if (*path =='/' ) { *path ='\0' ; -if (fspathcmp (real_path (path0), work_tree) ==0 ) { +strbuf_reset (&realpath); +strbuf_realpath (&realpath, path0,1 );
Likewise.
builtin/commit-graph.c static int abspath_part_inside_repo(char *path) · line 77
Quoted 7 lines
builtin/commit-graph.c +2
-1@@ -69,11 +73,15 @@ static int abspath_part_inside_repo(char *path) }/* check whole path */ -if (fspathcmp (real_path (path0), work_tree) ==0 ) { +strbuf_reset (&realpath); +strbuf_realpath (&realpath, path0,1 );
Likewise.
Quoted 5 lines
t/helper/test-path-utils.c +0
-0
t/helper/test-path-utils.c int cmd__path_utils(int argc, const char **argv) · line 297
Quoted 9 lines
t/helper/test-path-utils.c +4
-1@@ -290,11 +290,15 @@ int cmd__path_utils(int argc, const char **argv) }if (argc >=2 && !strcmp (argv[1 ],"real_path" )) { +struct strbuf realpath =STRBUF_INIT ;while (argc >2 ) { -puts (real_path (argv[2 ])); +strbuf_reset (&realpath); +strbuf_realpath (&realpath, argv[2 ],1 ); +puts (realpath.buf);
Likewise.
Thanks.
Re: [PATCH 2/4] real_path: remove unsafe API
Quoted 8 lines · Junio C Hamano
If my reading of the code is correct, however, the first thing that is done by
strbuf_realpath()is to empty the output buffer by usingstrbuf_reset()indirectly viaget_root_part(). Callingstrbuf_reset()here should not hurt, but it is unnecessary, I would think. An even worse effect such a redundantstrbuf_reset()has is that by repeatedly seeing the "reset then call realpath" pattern, readers who do not read the implementation ofstrbuf_realpath()might mistakenly think that
Quoted 5 lines
strbuf_addf(&message, "the path '%s' is really ", path); strbuf_realpath(&message, path);is how
realpath()is expected to be used, i.e. keep the current contents in the buffer and append the resolved path to it.
Thanks, will change in V2 next week.
PATCH 3/4 real_path_if_valid(): remove unsafe API
In reply to Alexandr Miloslavskiy via GitGitGadgetFrom: Alexandr Miloslavskiy <[email protected]>
This commit continues the work started with previous commit.
Signed-off-by Alexandr Miloslavskiy <[email protected]>
--- abspath.c | 10 ---------- cache.h | 1 - setup.c | 2 +- sha1-file.c | 13 ++++--------- submodule.c | 7 ++++--- worktree.c | 8 ++++++-- 6 files changed, 15 insertions(+), 26 deletions(-)
abspath.c +0 -10
@@ -202,16 +202,6 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,return retval; } -/* -* Resolve `path` into an absolute, cleaned-up path. The return value -* comes from a shared buffer. -*/ -const char *real_path_if_valid (const char *path) -{ -static struct strbuf realpath =STRBUF_INIT ; -return strbuf_realpath (&realpath, path,0 ); -} -char *real_pathdup (const char *path,int die_on_error) {struct strbuf realpath =STRBUF_INIT ;
cache.h +0 -1
@@ -1314,7 +1314,6 @@ static inline int is_absolute_path(const char *path)int is_directory (const char *);char *strbuf_realpath (struct strbuf *resolved,const char *path,int die_on_error); -const char *real_path_if_valid (const char *path);char *real_pathdup (const char *path,int die_on_error);const char *absolute_path (const char *path);char *absolute_pathdup (const char *path);
setup.c +1 -1
@@ -888,7 +888,7 @@ static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_/* * A "string_list_each_func_t" function that canonicalizes an entry -* from GIT_CEILING_DIRECTORIES using real_path_if_valid(), or +* from GIT_CEILING_DIRECTORIES using real_pathdup(), or * discards it if unusable. The presence of an empty entry in * GIT_CEILING_DIRECTORIES turns off canonicalization for all * subsequent entries.
sha1-file.c +4 -9
@@ -676,20 +676,15 @@ void add_to_alternates_memory(const char *reference)char *compute_alternate_path (const char *path,struct strbuf *err) {char *ref_git =NULL ; -const char *repo, *ref_git_s; +const char *repo;int seen_error =0 ; - ref_git_s =real_path_if_valid (path); -if (!ref_git_s) { + ref_git =real_pathdup (path,0 ); +if (!ref_git) { seen_error =1 ;strbuf_addf (err,_ ("path '%s' does not exist" ), path); goto out; - }else -/* -* Beware: read_gitfile(), real_path() and mkpath() -* return static buffer -*/ - ref_git =xstrdup (ref_git_s); + } repo =read_gitfile (ref_git);if (!repo)
submodule.c +4 -3
@@ -2173,7 +2173,7 @@ const char *get_superproject_working_tree(void)static struct strbuf realpath =STRBUF_INIT ;struct child_process cp =CHILD_PROCESS_INIT ;struct strbuf sb =STRBUF_INIT ; -const char *one_up =real_path_if_valid ("../" ); +struct strbuf one_up =STRBUF_INIT ;const char *cwd =xgetcwd ();const char *ret =NULL ;const char *subpath;
@@ -2188,10 +2188,11 @@ const char *get_superproject_working_tree(void)*/ return NULL ; -if (!one_up) +if (!strbuf_realpath (&one_up,"../" ,0 ))return NULL ; - subpath =relative_path (cwd, one_up, &sb); + subpath =relative_path (cwd, one_up.buf, &sb); +strbuf_release (&one_up);prepare_submodule_repo_env (&cp.env_array);argv_array_pop (&cp.env_array);
worktree.c +6 -2
@@ -226,17 +226,21 @@ struct worktree *find_worktree(struct worktree **list,struct worktree *find_worktree_by_path (struct worktree **list,const char *p) { +struct strbuf wt_path =STRBUF_INIT ;char *path =real_pathdup (p,0 );if (!path)return NULL ;for (; *list; list++) { -const char *wt_path =real_path_if_valid ((*list)->path); +strbuf_reset (&wt_path); +if (!strbuf_realpath (&wt_path, (*list)->path,0 )) +continue ; -if (wt_path && !fspathcmp (path, wt_path)) +if (!fspathcmp (path, wt_path.buf))break ; }free (path); +strbuf_release (&wt_path);return *list; }
Signature · 1 line
gitgitgadget
Re: [PATCH 3/4] real_path_if_valid(): remove unsafe API
"Alexandr Miloslavskiy via GitGitGadget" <[email protected]>
Quoted 16 lines · writes:
sha1-file.c +3
-3@@ -676,20 +676,15 @@ void add_to_alternates_memory(const char *reference)char *compute_alternate_path (const char *path,struct strbuf *err) {char *ref_git =NULL ; -const char *repo, *ref_git_s; +const char *repo;int seen_error =0 ; - ref_git_s =real_path_if_valid (path); -if (!ref_git_s) { + ref_git =real_pathdup (path,0 ); +if (!ref_git) {
sha1-file.c void add_to_alternates_memory(const char *reference) · line 687
Quoted 10 lines
sha1-file.c +1
-6seen_error =1 ;strbuf_addf (err,_ ("path '%s' does not exist" ), path); goto out; - }else -/* -* Beware: read_gitfile(), real_path() and mkpath() -* return static buffer -*/ - ref_git =xstrdup (ref_git_s); + }
It is amusing to see that rewriting not to use the unsafe function makes the code a lot easier to follow ;-)
PATCH 4/4 get_superproject_working_tree(): return strbuf
In reply to Alexandr Miloslavskiy via GitGitGadgetFrom: Alexandr Miloslavskiy <[email protected]>
Together with the previous commits, this commit fully fixes the problem of using shared buffer for real_path() in get_superproject_working_tree().
Signed-off-by Alexandr Miloslavskiy <[email protected]>
--- builtin/rev-parse.c | 7 ++++--- submodule.c | 17 ++++++++--------- submodule.h | 4 ++-- 3 files changed, 14 insertions(+), 14 deletions(-)
builtin/rev-parse.c +4 -3
@@ -808,9 +808,10 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)continue ; }if (!strcmp (arg,"--show-superproject-working-tree" )) { -const char *superproject =get_superproject_working_tree (); -if (superproject) -puts (superproject); +struct strbuf superproject =STRBUF_INIT ; +if (get_superproject_working_tree (&superproject)) +puts (superproject.buf); +strbuf_release (&superproject);continue ; }if (!strcmp (arg,"--show-prefix" )) {
submodule.c +8 -9
@@ -2168,14 +2168,13 @@ void absorb_git_dir_into_superproject(const char *path, } } -const char *get_superproject_working_tree (void ) +int get_superproject_working_tree (struct strbuf * buf) { -static struct strbuf realpath =STRBUF_INIT ;struct child_process cp =CHILD_PROCESS_INIT ;struct strbuf sb =STRBUF_INIT ;struct strbuf one_up =STRBUF_INIT ;const char *cwd =xgetcwd (); -const char *ret =NULL ; +int ret =0 ;const char *subpath;int code;ssize_t len;
@@ -2186,10 +2185,10 @@ const char *get_superproject_working_tree(void)* We might have a superproject, but it is harder * to determine. */ -return NULL ; +return 0 ;if (!strbuf_realpath (&one_up,"../" ,0 )) -return NULL ; +return 0 ; subpath =relative_path (cwd, one_up.buf, &sb);strbuf_release (&one_up);
@@ -2233,8 +2232,8 @@ const char *get_superproject_working_tree(void) super_wt =xstrdup (cwd); super_wt[cwd_len - super_sub_len] ='\0' ; -strbuf_realpath (&realpath, super_wt,1 ); - ret = realpath.buf; +strbuf_realpath (buf, super_wt,1 ); + ret =1 ;free (super_wt); }strbuf_release (&sb);
@@ -2243,10 +2242,10 @@ const char *get_superproject_working_tree(void)if (code ==128 )/* '../' is not a git repository */ -return NULL ; +return 0 ;if (code ==0 && len ==0 )/* There is an unrelated git repository at '../' */ -return NULL ; +return 0 ;if (code)die (_ ("ls-tree returned unexpected return code %d" ), code);
submodule.h +2 -2
@@ -152,8 +152,8 @@ void absorb_git_dir_into_superproject(const char *path,/* * Return the absolute path of the working tree of the superproject, which this * project is a submodule of. If this repository is not a submodule of -* another repository, return NULL. +* another repository, return 0. */ -const char *get_superproject_working_tree (void ); +int get_superproject_working_tree (struct strbuf * buf);#endif
Signature · 1 line
gitgitgadget
Re: [PATCH 4/4] get_superproject_working_tree(): return strbuf
"Alexandr Miloslavskiy via GitGitGadget" <[email protected]>
builtin/rev-parse.c int cmd_rev_parse(int argc, const char **argv, const char *prefix) · line 814
Quoted 8 lines · writes:
builtin/rev-parse.c +4
-3if (!strcmp (arg,"--show-superproject-working-tree" )) { -const char *superproject =get_superproject_working_tree (); -if (superproject) -puts (superproject); +struct strbuf superproject =STRBUF_INIT ; +if (get_superproject_working_tree (&superproject)) +puts (superproject.buf); +strbuf_release (&superproject);
The new calling convention makes sense here.
submodule.c void absorb_git_dir_into_superproject(const char *path, · line 2171
Quoted 13 lines
builtin/rev-parse.c +0
-0continue ; }if (!strcmp (arg,"--show-prefix" )) {submodule.c +1
-1@@ -2168,14 +2168,13 @@ void absorb_git_dir_into_superproject(const char *path, } } -const char *get_superproject_working_tree (void ) +int get_superproject_working_tree (struct strbuf * buf)
Micronit.
The asterisk sticks to the identifier, not type, in our codebase. I.e. "struct strbuf *buf".
submodule.h void absorb_git_dir_into_superproject(const char *path, · line 157
Quoted 13 lines
submodule.h +2
-2@@ -152,8 +152,8 @@ void absorb_git_dir_into_superproject(const char *path,/* * Return the absolute path of the working tree of the superproject, which this * project is a submodule of. If this repository is not a submodule of -* another repository, return NULL. +* another repository, return 0. */ -const char *get_superproject_working_tree (void ); +int get_superproject_working_tree (struct strbuf * buf);
Likewise.
The conversion of the function body looked quite sensible.
Thanks.
Re: [PATCH 4/4] get_superproject_working_tree(): return strbuf
Quoted 4 lines · Junio C Hamano
Micronit.
The asterisk sticks to the identifier, not type, in our codebase. I.e. "struct strbuf *buf".
Sorry, having difficulties switching between many different styles. Will fix in V2 next week.
Thank you very much for giving such a quick response! It is a great pleasure when my patches get movement instead of just gathering dust like in some other opensource projects :(
v2 cover Fix bugs related to real_path()
In reply to Alexandr Miloslavskiy via GitGitGadgetChanges since V1
-------------------
1) Removed strbuf_realpath() that weren't needed
2) Code style in declaration of get_superproject_working_tree()
Original description
-------------------
The issue with real_path() seems to be long-standing, where multiple people solved parts of it over time. I'm adding another part here after I have discovered a crash related to it.
Even with this step, there are still problems remaining:
* read_gitfile_gently() still uses shared buffer.
* absolute_path() was not removed.
These issues remain because there're too many code references and I'd like to avoid submitting a single topic of a scary size.
Alexandr Miloslavskiy (4):
set_git_dir: fix crash when used with real_path()
real_path: remove unsafe API real_path_if_valid(): remove unsafe API get_superproject_working_tree(): return strbuf
abspath.c | 18 +----------------- builtin/clone.c | 6 +++++- builtin/commit-graph.c | 5 ++++- builtin/init-db.c | 4 ++-- builtin/rev-parse.c | 12 ++++++++---- builtin/worktree.c | 9 ++++++--- cache.h | 4 +--- editor.c | 11 +++++++++-- environment.c | 18 ++++++++++++++++-- path.c | 4 ++-- setup.c | 35 ++++++++++++++++++++++------------- sha1-file.c | 13 ++++--------- submodule.c | 22 ++++++++++++---------- submodule.h | 4 ++-- t/helper/test-path-utils.c | 5 ++++- worktree.c | 12 +++++++++--- 16 files changed, 107 insertions(+), 75 deletions(-)
base-commit: 076cbdcd739aeb33c1be87b73aebae5e43d7bcc5
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-575%2FSyntevoAlex%2F%230205(git)_crash_real_path-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-575/SyntevoAlex/#0205(git)_crash_real_path-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/575
Range-diff vs v1:
1: f7afcb4cc83 = 1: f7afcb4cc83 set_git_dir: fix crash when used with real_path()
2: 039d3d36866 ! 2: 29e7133dcd9 real_path: remove unsafe API
@@ -64,7 +64,6 @@
die_errno(_("failed to unlink '%s'"), dest->buf);
if (!option_no_hardlinks) {
- if (!link(real_path(src->buf), dest->buf))
-+ strbuf_reset(&realpath);
+ strbuf_realpath(&realpath, src->buf, 1);
+ if (!link(realpath.buf, dest->buf))
continue;
@@ -92,7 +91,6 @@
prepare_alt_odb(r);
for (odb = r->objects->odb; odb; odb = odb->next) {
- if (!strcmp(obj_dir_real, real_path(odb->path)))
-+ strbuf_reset(&odb_path_real);
+ strbuf_realpath(&odb_path_real, odb->path, 1);
+ if (!strcmp(obj_dir_real, odb_path_real.buf))
break;
@@ -139,7 +137,6 @@
- write_file(sb.buf, "%s", real_path(sb_git.buf));
+ strbuf_realpath(&realpath, sb_git.buf, 1);
+ write_file(sb.buf, "%s", realpath.buf);
-+ strbuf_reset(&realpath);
+ strbuf_realpath(&realpath, get_git_common_dir(), 1);
write_file(sb_git.buf, "gitdir: %s/worktrees/%s",
- real_path(get_git_common_dir()), name);
@@ -261,7 +258,6 @@
if (*path == '/') {
*path = '\0';
- if (fspathcmp(real_path(path0), work_tree) == 0) {
-+ strbuf_reset(&realpath);
+ strbuf_realpath(&realpath, path0, 1);
+ if (fspathcmp(realpath.buf, work_tree) == 0) {
memmove(path0, path + 1, len - (path - path0));
@@ -274,7 +270,6 @@
/* check whole path */
- if (fspathcmp(real_path(path0), work_tree) == 0) {
-+ strbuf_reset(&realpath);
+ strbuf_realpath(&realpath, path0, 1);
+ if (fspathcmp(realpath.buf, work_tree) == 0) {
*path0 = '\0';
@@ -338,7 +333,6 @@
+ struct strbuf realpath = STRBUF_INIT;
while (argc > 2) {
- puts(real_path(argv[2]));
-+ strbuf_reset(&realpath);
+ strbuf_realpath(&realpath, argv[2], 1);
+ puts(realpath.buf);
argc--;
3: 59af49ad9f6 ! 3: a4917638671 real_path_if_valid(): remove unsafe API
@@ -122,7 +122,6 @@
return NULL;
for (; *list; list++) {
- const char *wt_path = real_path_if_valid((*list)->path);
-+ strbuf_reset(&wt_path);
+ if (!strbuf_realpath(&wt_path, (*list)->path, 0))
+ continue; 4: 2eeefda3d41 ! 4: 41950069a16 get_superproject_working_tree(): return strbuf
@@ -33,7 +33,7 @@
}
-const char *get_superproject_working_tree(void)
-+int get_superproject_working_tree(struct strbuf* buf)
++int get_superproject_working_tree(struct strbuf *buf)
{
- static struct strbuf realpath = STRBUF_INIT;
struct child_process cp = CHILD_PROCESS_INIT;
@@ -94,6 +94,6 @@
+ * another repository, return 0.
*/
-const char *get_superproject_working_tree(void);
-+int get_superproject_working_tree(struct strbuf* buf);
++int get_superproject_working_tree(struct strbuf *buf);
#endifSignature · 1 line
gitgitgadget
PATCH v2 1/4 set_git_dir: fix crash when used with real_path()
From: Alexandr Miloslavskiy <[email protected]>
real_path() returns result from a shared buffer, inviting subtle reentrance bugs. One of these bugs occur when invoked this way:
set_git_dir(real_path(git_dir))
In this case, real_path() has reentrance:
real_path
read_gitfile_gently
repo_set_gitdir
setup_git_env
set_git_dir_1
set_git_dirLater, set_git_dir() uses its now-dead parameter:
!is_absolute_path(path)
Fix this by using a dedicated strbuf to hold strbuf_realpath().
Signed-off-by Alexandr Miloslavskiy <[email protected]>
--- builtin/init-db.c | 4 ++-- cache.h | 2 +- environment.c | 11 ++++++++++- path.c | 2 +- setup.c | 18 +++++++++--------- 5 files changed, 23 insertions(+), 14 deletions(-)
builtin/init-db.c +2 -2
@@ -356,12 +356,12 @@ int init_db(const char *git_dir, const char *real_git_dir,if (!exist_ok && !stat (real_git_dir, &st))die (_ ("%s already exists" ), real_git_dir); -set_git_dir (real_path (real_git_dir)); +set_git_dir (real_git_dir,1 ); git_dir =get_git_dir ();separate_git_dir (git_dir, original_git_dir); }else { -set_git_dir (real_path (git_dir)); +set_git_dir (git_dir,1 ); git_dir =get_git_dir (); } startup_info->have_repository =1 ;
cache.h +1 -1
@@ -543,7 +543,7 @@ const char *get_git_common_dir(void);char *get_object_directory (void );char *get_index_file (void );char *get_graft_file (struct repository *r); -void set_git_dir (const char *path); +void set_git_dir (const char *path,int make_realpath);int get_common_dir_noenv (struct strbuf *sb,const char *gitdir);int get_common_dir (struct strbuf *sb,const char *gitdir);const char *get_git_namespace (void );
environment.c +10 -1
@@ -345,11 +345,20 @@ static void update_relative_gitdir(const char *name,free (path); } -void set_git_dir (const char *path) +void set_git_dir (const char *path,int make_realpath) { +struct strbuf realpath =STRBUF_INIT ; + +if (make_realpath) { +strbuf_realpath (&realpath, path,1 ); + path = realpath.buf; + } +set_git_dir_1 (path);if (!is_absolute_path (path))chdir_notify_register (NULL , update_relative_gitdir,NULL ); + +strbuf_release (&realpath); }const char *get_log_output_encoding (void )
path.c +1 -1
@@ -850,7 +850,7 @@ const char *enter_repo(const char *path, int strict) }if (is_git_directory ("." )) { -set_git_dir ("." ); +set_git_dir ("." ,0 );check_repository_format ();return path; }
setup.c +9 -9
@@ -725,7 +725,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv, }/* #18, #26 */ -set_git_dir (gitdirenv); +set_git_dir (gitdirenv,0 );free (gitfile);return NULL ; }
@@ -747,7 +747,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv, }else if (!git_env_bool (GIT_IMPLICIT_WORK_TREE_ENVIRONMENT ,1 )) {/* #16d */ -set_git_dir (gitdirenv); +set_git_dir (gitdirenv,0 );free (gitfile);return NULL ; }
@@ -759,14 +759,14 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,/* both get_git_work_tree() and cwd are already normalized */ if (!strcmp (cwd->buf, worktree)) {/* cwd == worktree */ -set_git_dir (gitdirenv); +set_git_dir (gitdirenv,0 );free (gitfile);return NULL ; } offset =dir_inside_of (cwd->buf, worktree);if (offset >=0 ) {/* cwd inside worktree? */ -set_git_dir (real_path (gitdirenv)); +set_git_dir (gitdirenv,1 );if (chdir (worktree))die_errno (_ ("cannot chdir to '%s'" ), worktree);strbuf_addch (cwd,'/' );
@@ -775,7 +775,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv, }/* cwd outside worktree */ -set_git_dir (gitdirenv); +set_git_dir (gitdirenv,0 );free (gitfile);return NULL ; }
@@ -804,7 +804,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */ if (is_bare_repository_cfg >0 ) { -set_git_dir (offset == cwd->len ? gitdir :real_path (gitdir)); +set_git_dir (gitdir, (offset != cwd->len));if (chdir (cwd->buf))die_errno (_ ("cannot come back to cwd" ));return NULL ;
@@ -813,7 +813,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,/* #0, #1, #5, #8, #9, #12, #13 */ set_git_work_tree ("." );if (strcmp (gitdir,DEFAULT_GIT_DIR_ENVIRONMENT )) -set_git_dir (gitdir); +set_git_dir (gitdir,0 ); inside_git_dir =0 ; inside_work_tree =1 ;if (offset >= cwd->len)
@@ -856,10 +856,10 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,die_errno (_ ("cannot come back to cwd" )); root_len =offset_1st_component (cwd->buf);strbuf_setlen (cwd, offset > root_len ? offset : root_len); -set_git_dir (cwd->buf); +set_git_dir (cwd->buf,0 ); }else -set_git_dir ("." ); +set_git_dir ("." ,0 );return NULL ; }
Signature · 1 line
gitgitgadget
PATCH v2 2/4 real_path: remove unsafe API
In reply to Alexandr Miloslavskiy via GitGitGadgetFrom: Alexandr Miloslavskiy <[email protected]>
Returning a shared buffer invites very subtle bugs due to reentrancy or multi-threading, as demonstrated by the previous patch.
There was an unfinished effort to abolish this [1].
Let's finally rid of real_path(), using strbuf_realpath() instead.
This patch uses a local strbuf for most places where real_path() was previously called.
However, two places return the value of real_path() to the caller. For them, a static local strbuf was added, effectively pushing the problem one level higher:
read_gitfile_gently()
get_superproject_working_tree()
[1] https://lore.kernel.org/git/[email protected]/
Signed-off-by Alexandr Miloslavskiy <[email protected]>
--- abspath.c | 8 +------- builtin/clone.c | 6 +++++- builtin/commit-graph.c | 5 ++++- builtin/rev-parse.c | 5 ++++- builtin/worktree.c | 9 ++++++--- cache.h | 1 - editor.c | 11 +++++++++-- environment.c | 7 ++++++- path.c | 2 +- setup.c | 15 ++++++++++++--- submodule.c | 4 +++- t/helper/test-path-utils.c | 5 ++++- worktree.c | 5 ++++- 13 files changed, 59 insertions(+), 24 deletions(-)
abspath.c +1 -7
@@ -206,12 +206,6 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,* Resolve `path` into an absolute, cleaned-up path. The return value * comes from a shared buffer. */ -const char *real_path (const char *path) -{ -static struct strbuf realpath =STRBUF_INIT ; -return strbuf_realpath (&realpath, path,1 ); -} -const char *real_path_if_valid (const char *path) {static struct strbuf realpath =STRBUF_INIT ;
@@ -233,7 +227,7 @@ char *real_pathdup(const char *path, int die_on_error)/* * Use this to get an absolute path from a relative one. If you want -* to resolve links, you should use real_path. +* to resolve links, you should use strbuf_realpath. */ const char *absolute_path (const char *path) {
builtin/clone.c +5 -1
@@ -420,6 +420,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,struct dir_iterator *iter;int iter_status;unsigned int flags; +struct strbuf realpath =STRBUF_INIT ;mkdir_if_missing (dest->buf,0777 );
@@ -454,7 +455,8 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,if (unlink (dest->buf) && errno !=ENOENT )die_errno (_ ("failed to unlink '%s'" ), dest->buf);if (!option_no_hardlinks) { -if (!link (real_path (src->buf), dest->buf)) +strbuf_realpath (&realpath, src->buf,1 ); +if (!link (realpath.buf, dest->buf))continue ;if (option_local >0 )die_errno (_ ("failed to create link '%s'" ), dest->buf);
@@ -468,6 +470,8 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,strbuf_setlen (src, src_len);die (_ ("failed to iterate over '%s'" ), src->buf); } + +strbuf_release (&realpath); }static void clone_local (const char *src_repo,const char *dest_repo)
builtin/commit-graph.c +4 -1
@@ -39,14 +39,17 @@ static struct object_directory *find_odb(struct repository *r, {struct object_directory *odb;char *obj_dir_real =real_pathdup (obj_dir,1 ); +struct strbuf odb_path_real =STRBUF_INIT ;prepare_alt_odb (r);for (odb = r->objects->odb; odb; odb = odb->next) { -if (!strcmp (obj_dir_real,real_path (odb->path))) +strbuf_realpath (&odb_path_real, odb->path,1 ); +if (!strcmp (obj_dir_real, odb_path_real.buf))break ; }free (obj_dir_real); +strbuf_release (&odb_path_real);if (!odb)die (_ ("could not find object directory matching %s" ), obj_dir);
builtin/rev-parse.c +4 -1
@@ -857,7 +857,10 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)if (!gitdir && !prefix) gitdir =".git" ;if (gitdir) { -puts (real_path (gitdir)); +struct strbuf realpath =STRBUF_INIT ; +strbuf_realpath (&realpath, gitdir,1 ); +puts (realpath.buf); +strbuf_release (&realpath);continue ; } }
builtin/worktree.c +6 -3
@@ -258,7 +258,7 @@ static int add_worktree(const char *path, const char *refname, const struct add_opts *opts) {struct strbuf sb_git =STRBUF_INIT , sb_repo =STRBUF_INIT ; -struct strbuf sb =STRBUF_INIT ; +struct strbuf sb =STRBUF_INIT , realpath =STRBUF_INIT ;const char *name;struct child_process cp =CHILD_PROCESS_INIT ;struct argv_array child_env =ARGV_ARRAY_INIT ;
@@ -330,9 +330,11 @@ static int add_worktree(const char *path, const char *refname,strbuf_reset (&sb);strbuf_addf (&sb,"%s/gitdir" , sb_repo.buf); -write_file (sb.buf,"%s" ,real_path (sb_git.buf)); +strbuf_realpath (&realpath, sb_git.buf,1 ); +write_file (sb.buf,"%s" , realpath.buf); +strbuf_realpath (&realpath,get_git_common_dir (),1 );write_file (sb_git.buf,"gitdir: %s/worktrees/%s" , -real_path (get_git_common_dir ()), name); + realpath.buf, name);/* * This is to keep resolve_ref() happy. We need a valid HEAD * or is_git_directory() will reject the directory. Any value which
@@ -418,6 +420,7 @@ static int add_worktree(const char *path, const char *refname,strbuf_release (&sb_repo);strbuf_release (&sb_git);strbuf_release (&sb_name); +strbuf_release (&realpath);return ret; }
cache.h +0 -1
@@ -1314,7 +1314,6 @@ static inline int is_absolute_path(const char *path)int is_directory (const char *);char *strbuf_realpath (struct strbuf *resolved,const char *path,int die_on_error); -const char *real_path (const char *path);const char *real_path_if_valid (const char *path);char *real_pathdup (const char *path,int die_on_error);const char *absolute_path (const char *path);
editor.c +9 -2
@@ -54,7 +54,8 @@ static int launch_specified_editor(const char *editor, const char *path,return error ("Terminal is dumb, but EDITOR unset" );if (strcmp (editor,":" )) { -const char *args[] = { editor,real_path (path),NULL }; +struct strbuf realpath =STRBUF_INIT ; +const char *args[] = { editor,NULL ,NULL };struct child_process p =CHILD_PROCESS_INIT ;int ret, sig;int print_waiting_for_editor = advice_waiting_for_editor &&isatty (2 );
@@ -75,16 +76,22 @@ static int launch_specified_editor(const char *editor, const char *path,fflush (stderr); } +strbuf_realpath (&realpath, path,1 ); + args[1 ] = realpath.buf; + p.argv = args; p.env = env; p.use_shell =1 ; p.trace2_child_class ="editor" ; -if (start_command (&p) <0 ) +if (start_command (&p) <0 ) { +strbuf_release (&realpath);return error ("unable to start editor '%s'" , editor); + }sigchain_push (SIGINT ,SIG_IGN );sigchain_push (SIGQUIT ,SIG_IGN ); ret =finish_command (&p); +strbuf_release (&realpath); sig = ret -128 ;sigchain_pop (SIGINT );sigchain_pop (SIGQUIT );
environment.c +6 -1
@@ -254,8 +254,11 @@ static int git_work_tree_initialized;*/ void set_git_work_tree (const char *new_work_tree) { +struct strbuf realpath =STRBUF_INIT ; +if (git_work_tree_initialized) { - new_work_tree =real_path (new_work_tree); +strbuf_realpath (&realpath, new_work_tree,1 ); + new_work_tree = realpath.buf;if (strcmp (new_work_tree, the_repository->worktree)) die("internal error: work tree has already been set\n" "Current worktree: %s\nNew worktree: %s",
@@ -264,6 +267,8 @@ void set_git_work_tree(const char *new_work_tree) } git_work_tree_initialized =1 ;repo_set_worktree (the_repository, new_work_tree); + +strbuf_release (&realpath); }const char *get_git_work_tree (void )
path.c +1 -1
@@ -723,7 +723,7 @@ static struct passwd *getpw_str(const char *username, size_t len)* then it is a newly allocated string. Returns NULL on getpw failure or * if path is NULL. * -* If real_home is true, real_path($HOME) is used in the expansion. +* If real_home is true, strbuf_realpath($HOME) is used in the expansion. */ char *expand_user_path (const char *path,int real_home) {
setup.c +12 -3
@@ -32,6 +32,7 @@ static int abspath_part_inside_repo(char *path)char *path0;int off;const char *work_tree =get_git_work_tree (); +struct strbuf realpath =STRBUF_INIT ;if (!work_tree)return -1 ;
@@ -60,8 +61,10 @@ static int abspath_part_inside_repo(char *path) path++;if (*path =='/' ) { *path ='\0' ; -if (fspathcmp (real_path (path0), work_tree) ==0 ) { +strbuf_realpath (&realpath, path0,1 ); +if (fspathcmp (realpath.buf, work_tree) ==0 ) {memmove (path0, path +1 , len - (path - path0)); +strbuf_release (&realpath);return 0 ; } *path ='/' ;
@@ -69,11 +72,14 @@ static int abspath_part_inside_repo(char *path) }/* check whole path */ -if (fspathcmp (real_path (path0), work_tree) ==0 ) { +strbuf_realpath (&realpath, path0,1 ); +if (fspathcmp (realpath.buf, work_tree) ==0 ) { *path0 ='\0' ; +strbuf_release (&realpath);return 0 ; } +strbuf_release (&realpath);return -1 ; }
@@ -619,6 +625,7 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)struct stat st;int fd;ssize_t len; +static struct strbuf realpath =STRBUF_INIT ;if (stat (path, &st)) {/* NEEDSWORK: discern between ENOENT vs other errors */
@@ -669,7 +676,9 @@ const char *read_gitfile_gently(const char *path, int *return_error_code) error_code =READ_GITFILE_ERR_NOT_A_REPO ; goto cleanup_return; } - path =real_path (dir); + +strbuf_realpath (&realpath, dir,1 ); + path = realpath.buf; cleanup_return:if (return_error_code)
submodule.c +3 -1
@@ -2170,6 +2170,7 @@ void absorb_git_dir_into_superproject(const char *path,const char *get_superproject_working_tree (void ) { +static struct strbuf realpath =STRBUF_INIT ;struct child_process cp =CHILD_PROCESS_INIT ;struct strbuf sb =STRBUF_INIT ;const char *one_up =real_path_if_valid ("../" );
@@ -2231,7 +2232,8 @@ const char *get_superproject_working_tree(void) super_wt =xstrdup (cwd); super_wt[cwd_len - super_sub_len] ='\0' ; - ret =real_path (super_wt); +strbuf_realpath (&realpath, super_wt,1 ); + ret = realpath.buf;free (super_wt); }strbuf_release (&sb);
t/helper/test-path-utils.c +4 -1
@@ -290,11 +290,14 @@ int cmd__path_utils(int argc, const char **argv) }if (argc >=2 && !strcmp (argv[1 ],"real_path" )) { +struct strbuf realpath =STRBUF_INIT ;while (argc >2 ) { -puts (real_path (argv[2 ])); +strbuf_realpath (&realpath, argv[2 ],1 ); +puts (realpath.buf); argc--; argv++; } +strbuf_release (&realpath);return 0 ; }
worktree.c +4 -1
@@ -285,6 +285,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg, unsigned flags) {struct strbuf wt_path =STRBUF_INIT ; +struct strbuf realpath =STRBUF_INIT ;char *path =NULL ;int err, ret =-1 ;
@@ -336,7 +337,8 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg, goto done; } - ret =fspathcmp (path,real_path (git_common_path ("worktrees/%s" , wt->id))); +strbuf_realpath (&realpath,git_common_path ("worktrees/%s" , wt->id),1 ); + ret =fspathcmp (path, realpath.buf);if (ret) strbuf_addf_gently(errmsg, _("'%s' does not point back to '%s'"),
@@ -344,6 +346,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg, done:free (path);strbuf_release (&wt_path); +strbuf_release (&realpath);return ret; }
Signature · 1 line
gitgitgadget
PATCH v2 3/4 real_path_if_valid(): remove unsafe API
In reply to Alexandr Miloslavskiy via GitGitGadgetFrom: Alexandr Miloslavskiy <[email protected]>
This commit continues the work started with previous commit.
Signed-off-by Alexandr Miloslavskiy <[email protected]>
--- abspath.c | 10 ---------- cache.h | 1 - setup.c | 2 +- sha1-file.c | 13 ++++--------- submodule.c | 7 ++++--- worktree.c | 7 +++++-- 6 files changed, 14 insertions(+), 26 deletions(-)
abspath.c +0 -10
@@ -202,16 +202,6 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,return retval; } -/* -* Resolve `path` into an absolute, cleaned-up path. The return value -* comes from a shared buffer. -*/ -const char *real_path_if_valid (const char *path) -{ -static struct strbuf realpath =STRBUF_INIT ; -return strbuf_realpath (&realpath, path,0 ); -} -char *real_pathdup (const char *path,int die_on_error) {struct strbuf realpath =STRBUF_INIT ;
cache.h +0 -1
@@ -1314,7 +1314,6 @@ static inline int is_absolute_path(const char *path)int is_directory (const char *);char *strbuf_realpath (struct strbuf *resolved,const char *path,int die_on_error); -const char *real_path_if_valid (const char *path);char *real_pathdup (const char *path,int die_on_error);const char *absolute_path (const char *path);char *absolute_pathdup (const char *path);
setup.c +1 -1
@@ -886,7 +886,7 @@ static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_/* * A "string_list_each_func_t" function that canonicalizes an entry -* from GIT_CEILING_DIRECTORIES using real_path_if_valid(), or +* from GIT_CEILING_DIRECTORIES using real_pathdup(), or * discards it if unusable. The presence of an empty entry in * GIT_CEILING_DIRECTORIES turns off canonicalization for all * subsequent entries.
sha1-file.c +4 -9
@@ -676,20 +676,15 @@ void add_to_alternates_memory(const char *reference)char *compute_alternate_path (const char *path,struct strbuf *err) {char *ref_git =NULL ; -const char *repo, *ref_git_s; +const char *repo;int seen_error =0 ; - ref_git_s =real_path_if_valid (path); -if (!ref_git_s) { + ref_git =real_pathdup (path,0 ); +if (!ref_git) { seen_error =1 ;strbuf_addf (err,_ ("path '%s' does not exist" ), path); goto out; - }else -/* -* Beware: read_gitfile(), real_path() and mkpath() -* return static buffer -*/ - ref_git =xstrdup (ref_git_s); + } repo =read_gitfile (ref_git);if (!repo)
submodule.c +4 -3
@@ -2173,7 +2173,7 @@ const char *get_superproject_working_tree(void)static struct strbuf realpath =STRBUF_INIT ;struct child_process cp =CHILD_PROCESS_INIT ;struct strbuf sb =STRBUF_INIT ; -const char *one_up =real_path_if_valid ("../" ); +struct strbuf one_up =STRBUF_INIT ;const char *cwd =xgetcwd ();const char *ret =NULL ;const char *subpath;
@@ -2188,10 +2188,11 @@ const char *get_superproject_working_tree(void)*/ return NULL ; -if (!one_up) +if (!strbuf_realpath (&one_up,"../" ,0 ))return NULL ; - subpath =relative_path (cwd, one_up, &sb); + subpath =relative_path (cwd, one_up.buf, &sb); +strbuf_release (&one_up);prepare_submodule_repo_env (&cp.env_array);argv_array_pop (&cp.env_array);
worktree.c +5 -2
@@ -226,17 +226,20 @@ struct worktree *find_worktree(struct worktree **list,struct worktree *find_worktree_by_path (struct worktree **list,const char *p) { +struct strbuf wt_path =STRBUF_INIT ;char *path =real_pathdup (p,0 );if (!path)return NULL ;for (; *list; list++) { -const char *wt_path =real_path_if_valid ((*list)->path); +if (!strbuf_realpath (&wt_path, (*list)->path,0 )) +continue ; -if (wt_path && !fspathcmp (path, wt_path)) +if (!fspathcmp (path, wt_path.buf))break ; }free (path); +strbuf_release (&wt_path);return *list; }
Signature · 1 line
gitgitgadget
PATCH v2 4/4 get_superproject_working_tree(): return strbuf
In reply to Alexandr Miloslavskiy via GitGitGadgetFrom: Alexandr Miloslavskiy <[email protected]>
Together with the previous commits, this commit fully fixes the problem of using shared buffer for real_path() in get_superproject_working_tree().
Signed-off-by Alexandr Miloslavskiy <[email protected]>
--- builtin/rev-parse.c | 7 ++++--- submodule.c | 17 ++++++++--------- submodule.h | 4 ++-- 3 files changed, 14 insertions(+), 14 deletions(-)
builtin/rev-parse.c +4 -3
@@ -808,9 +808,10 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)continue ; }if (!strcmp (arg,"--show-superproject-working-tree" )) { -const char *superproject =get_superproject_working_tree (); -if (superproject) -puts (superproject); +struct strbuf superproject =STRBUF_INIT ; +if (get_superproject_working_tree (&superproject)) +puts (superproject.buf); +strbuf_release (&superproject);continue ; }if (!strcmp (arg,"--show-prefix" )) {
submodule.c +8 -9
@@ -2168,14 +2168,13 @@ void absorb_git_dir_into_superproject(const char *path, } } -const char *get_superproject_working_tree (void ) +int get_superproject_working_tree (struct strbuf *buf) { -static struct strbuf realpath =STRBUF_INIT ;struct child_process cp =CHILD_PROCESS_INIT ;struct strbuf sb =STRBUF_INIT ;struct strbuf one_up =STRBUF_INIT ;const char *cwd =xgetcwd (); -const char *ret =NULL ; +int ret =0 ;const char *subpath;int code;ssize_t len;
@@ -2186,10 +2185,10 @@ const char *get_superproject_working_tree(void)* We might have a superproject, but it is harder * to determine. */ -return NULL ; +return 0 ;if (!strbuf_realpath (&one_up,"../" ,0 )) -return NULL ; +return 0 ; subpath =relative_path (cwd, one_up.buf, &sb);strbuf_release (&one_up);
@@ -2233,8 +2232,8 @@ const char *get_superproject_working_tree(void) super_wt =xstrdup (cwd); super_wt[cwd_len - super_sub_len] ='\0' ; -strbuf_realpath (&realpath, super_wt,1 ); - ret = realpath.buf; +strbuf_realpath (buf, super_wt,1 ); + ret =1 ;free (super_wt); }strbuf_release (&sb);
@@ -2243,10 +2242,10 @@ const char *get_superproject_working_tree(void)if (code ==128 )/* '../' is not a git repository */ -return NULL ; +return 0 ;if (code ==0 && len ==0 )/* There is an unrelated git repository at '../' */ -return NULL ; +return 0 ;if (code)die (_ ("ls-tree returned unexpected return code %d" ), code);
submodule.h +2 -2
@@ -152,8 +152,8 @@ void absorb_git_dir_into_superproject(const char *path,/* * Return the absolute path of the working tree of the superproject, which this * project is a submodule of. If this repository is not a submodule of -* another repository, return NULL. +* another repository, return 0. */ -const char *get_superproject_working_tree (void ); +int get_superproject_working_tree (struct strbuf *buf);#endif
Signature · 1 line
gitgitgadget