{"thread":{"id":"55780","subject":"[PATCH 0/2] Directory traversal fixes","startedAt":"2021-05-27T04:54:02Z","lastAt":"2021-05-27T04:54:07Z","messageCount":3,"participants":["Elijah Newren via GitGitGadget","Derrick Stolee via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"425651","messageId":"pull.961.git.1622091237.gitgitgadget@gmail.com","threadId":"55780","inReplyTo":null,"subject":"[PATCH 0/2] Directory traversal fixes","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-27T04:53:54Z","receivedAt":"2021-05-27T04:54:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This is a resend of the final two patches of my series, as per\nhttps://lore.kernel.org/git/xmqqfsyht49j.fsf@gitster.g/. It will not merge\ncleanly unless the last two patches of en/dir-traversal-fixes are first\nreverted, as Junio suggested he'd be doing.\n\n(Corrected Reviewed-by -> Signed-off-by in the first patch, minor stylistic\nfix for the second.)\n\nDerrick Stolee (1):\n  dir: update stale description of treat_directory()\n\nElijah Newren (1):\n  dir: introduce readdir_skip_dot_and_dotdot() helper\n\n builtin/clean.c    |  4 +---\n builtin/worktree.c |  4 +---\n diff-no-index.c    |  5 ++---\n dir.c              | 38 ++++++++++++++++++++++----------------\n dir.h              |  2 ++\n entry.c            |  5 +----\n notes-merge.c      |  5 +----\n object-file.c      |  4 +---\n packfile.c         |  5 +----\n rerere.c           |  4 +---\n worktree.c         | 12 +++---------\n 11 files changed, 36 insertions(+), 52 deletions(-)\n\n\nbase-commit: dd55fc0df15c338a8dbec6dec6ca6b58edc8acef\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-961%2Fnewren%2Fdirectory-traversal-fixes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-961/newren/directory-traversal-fixes-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/961\n-- \ngitgitgadget\n"},{"id":"425652","messageId":"970fad21c0286fb716b38a332f9704784aa6cf8f.1622091237.git.gitgitgadget@gmail.com","threadId":"55780","inReplyTo":"pull.961.git.1622091237.gitgitgadget@gmail.com","subject":"[PATCH 2/2] dir: introduce readdir_skip_dot_and_dotdot() helper","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-27T04:53:56Z","receivedAt":"2021-05-27T04:54:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nMany places in the code were doing\n    while ((d = readdir(dir)) != NULL) {\n        if (is_dot_or_dotdot(d->d_name))\n            continue;\n        ...process d...\n    }\nIntroduce a readdir_skip_dot_and_dotdot() helper to make that a one-liner:\n    while ((d = readdir_skip_dot_and_dotdot(dir)) != NULL) {\n        ...process d...\n    }\n\nThis helper particularly simplifies checks for empty directories.\n\nAlso use this helper in read_cached_dir() so that our statistics are\nconsistent across platforms.  (In other words, read_cached_dir() should\nhave been using is_dot_or_dotdot() and skipping such entries, but did\nnot and left it to treat_path() to detect and mark such entries as\npath_none.)\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/clean.c    |  4 +---\n builtin/worktree.c |  4 +---\n diff-no-index.c    |  5 ++---\n dir.c              | 25 ++++++++++++++++---------\n dir.h              |  2 ++\n entry.c            |  5 +----\n notes-merge.c      |  5 +----\n object-file.c      |  4 +---\n packfile.c         |  5 +----\n rerere.c           |  4 +---\n worktree.c         | 12 +++---------\n 11 files changed, 30 insertions(+), 45 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 995053b79173..a1a57476153b 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -189,10 +189,8 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n \tstrbuf_complete(path, '/');\n \n \tlen = path->len;\n-\twhile ((e = readdir(dir)) != NULL) {\n+\twhile ((e = readdir_skip_dot_and_dotdot(dir)) != NULL) {\n \t\tstruct stat st;\n-\t\tif (is_dot_or_dotdot(e->d_name))\n-\t\t\tcontinue;\n \n \t\tstrbuf_setlen(path, len);\n \t\tstrbuf_addstr(path, e->d_name);\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 1cd5c2016e3f..e081ca9befd1 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -118,10 +118,8 @@ static void prune_worktrees(void)\n \tstruct dirent *d;\n \tif (!dir)\n \t\treturn;\n-\twhile ((d = readdir(dir)) != NULL) {\n+\twhile ((d = readdir_skip_dot_and_dotdot(dir)) != NULL) {\n \t\tchar *path;\n-\t\tif (is_dot_or_dotdot(d->d_name))\n-\t\t\tcontinue;\n \t\tstrbuf_reset(&reason);\n \t\tif (should_prune_worktree(d->d_name, &reason, &path, expire))\n \t\t\tprune_worktree(d->d_name, reason.buf);\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 7814eabfe028..e5cc87837143 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -26,9 +26,8 @@ static int read_directory_contents(const char *path, struct string_list *list)\n \tif (!(dir = opendir(path)))\n \t\treturn error(\"Could not open directory %s\", path);\n \n-\twhile ((e = readdir(dir)))\n-\t\tif (!is_dot_or_dotdot(e->d_name))\n-\t\t\tstring_list_insert(list, e->d_name);\n+\twhile ((e = readdir_skip_dot_and_dotdot(dir)))\n+\t\tstring_list_insert(list, e->d_name);\n \n \tclosedir(dir);\n \treturn 0;\ndiff --git a/dir.c b/dir.c\nindex ff004b298b2a..b909cf9b03da 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -59,6 +59,17 @@ void dir_init(struct dir_struct *dir)\n \tmemset(dir, 0, sizeof(*dir));\n }\n \n+struct dirent *readdir_skip_dot_and_dotdot(DIR *dirp)\n+{\n+\tstruct dirent *e;\n+\n+\twhile ((e = readdir(dirp)) != NULL) {\n+\t\tif (!is_dot_or_dotdot(e->d_name))\n+\t\t\tbreak;\n+\t}\n+\treturn e;\n+}\n+\n int count_slashes(const char *s)\n {\n \tint cnt = 0;\n@@ -2332,7 +2343,7 @@ static int read_cached_dir(struct cached_dir *cdir)\n \tstruct dirent *de;\n \n \tif (cdir->fdir) {\n-\t\tde = readdir(cdir->fdir);\n+\t\tde = readdir_skip_dot_and_dotdot(cdir->fdir);\n \t\tif (!de) {\n \t\t\tcdir->d_name = NULL;\n \t\t\tcdir->d_type = DT_UNKNOWN;\n@@ -2931,11 +2942,9 @@ int is_empty_dir(const char *path)\n \tif (!dir)\n \t\treturn 0;\n \n-\twhile ((e = readdir(dir)) != NULL)\n-\t\tif (!is_dot_or_dotdot(e->d_name)) {\n-\t\t\tret = 0;\n-\t\t\tbreak;\n-\t\t}\n+\te = readdir_skip_dot_and_dotdot(dir);\n+\tif (e)\n+\t\tret = 0;\n \n \tclosedir(dir);\n \treturn ret;\n@@ -2975,10 +2984,8 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n \tstrbuf_complete(path, '/');\n \n \tlen = path->len;\n-\twhile ((e = readdir(dir)) != NULL) {\n+\twhile ((e = readdir_skip_dot_and_dotdot(dir)) != NULL) {\n \t\tstruct stat st;\n-\t\tif (is_dot_or_dotdot(e->d_name))\n-\t\t\tcontinue;\n \n \t\tstrbuf_setlen(path, len);\n \t\tstrbuf_addstr(path, e->d_name);\ndiff --git a/dir.h b/dir.h\nindex 70c750e3053d..6b3fac082945 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -342,6 +342,8 @@ struct dir_struct {\n \tunsigned visited_directories;\n };\n \n+struct dirent *readdir_skip_dot_and_dotdot(DIR *dirp);\n+\n /*Count the number of slashes for string s*/\n int count_slashes(const char *s);\n \ndiff --git a/entry.c b/entry.c\nindex 7b9f43716f76..e3d3add30007 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -56,12 +56,9 @@ static void remove_subtree(struct strbuf *path)\n \n \tif (!dir)\n \t\tdie_errno(\"cannot opendir '%s'\", path->buf);\n-\twhile ((de = readdir(dir)) != NULL) {\n+\twhile ((de = readdir_skip_dot_and_dotdot(dir)) != NULL) {\n \t\tstruct stat st;\n \n-\t\tif (is_dot_or_dotdot(de->d_name))\n-\t\t\tcontinue;\n-\n \t\tstrbuf_addch(path, '/');\n \t\tstrbuf_addstr(path, de->d_name);\n \t\tif (lstat(path->buf, &st))\ndiff --git a/notes-merge.c b/notes-merge.c\nindex d2771fa3d43c..e9d6f86d3428 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -695,13 +695,10 @@ int notes_merge_commit(struct notes_merge_options *o,\n \n \tstrbuf_addch(&path, '/');\n \tbaselen = path.len;\n-\twhile ((e = readdir(dir)) != NULL) {\n+\twhile ((e = readdir_skip_dot_and_dotdot(dir)) != NULL) {\n \t\tstruct stat st;\n \t\tstruct object_id obj_oid, blob_oid;\n \n-\t\tif (is_dot_or_dotdot(e->d_name))\n-\t\t\tcontinue;\n-\n \t\tif (get_oid_hex(e->d_name, &obj_oid)) {\n \t\t\tif (o->verbosity >= 3)\n \t\t\t\tprintf(\"Skipping non-SHA1 entry '%s%s'\\n\",\ndiff --git a/object-file.c b/object-file.c\nindex 624af408cdcd..77bdcfd21bc8 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -2304,10 +2304,8 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,\n \tstrbuf_addch(path, '/');\n \tbaselen = path->len;\n \n-\twhile ((de = readdir(dir))) {\n+\twhile ((de = readdir_skip_dot_and_dotdot(dir))) {\n \t\tsize_t namelen;\n-\t\tif (is_dot_or_dotdot(de->d_name))\n-\t\t\tcontinue;\n \n \t\tnamelen = strlen(de->d_name);\n \t\tstrbuf_setlen(path, baselen);\ndiff --git a/packfile.c b/packfile.c\nindex ea29f4ba7735..463d61c87761 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -813,10 +813,7 @@ void for_each_file_in_pack_dir(const char *objdir,\n \t}\n \tstrbuf_addch(&path, '/');\n \tdirnamelen = path.len;\n-\twhile ((de = readdir(dir)) != NULL) {\n-\t\tif (is_dot_or_dotdot(de->d_name))\n-\t\t\tcontinue;\n-\n+\twhile ((de = readdir_skip_dot_and_dotdot(dir)) != NULL) {\n \t\tstrbuf_setlen(&path, dirnamelen);\n \t\tstrbuf_addstr(&path, de->d_name);\n \ndiff --git a/rerere.c b/rerere.c\nindex dee60dc6df63..d83d58df4fbc 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -1190,13 +1190,11 @@ void rerere_gc(struct repository *r, struct string_list *rr)\n \tif (!dir)\n \t\tdie_errno(_(\"unable to open rr-cache directory\"));\n \t/* Collect stale conflict IDs ... */\n-\twhile ((e = readdir(dir))) {\n+\twhile ((e = readdir_skip_dot_and_dotdot(dir))) {\n \t\tstruct rerere_dir *rr_dir;\n \t\tstruct rerere_id id;\n \t\tint now_empty;\n \n-\t\tif (is_dot_or_dotdot(e->d_name))\n-\t\t\tcontinue;\n \t\tif (!is_rr_cache_dirname(e->d_name))\n \t\t\tcontinue; /* or should we remove e->d_name? */\n \ndiff --git a/worktree.c b/worktree.c\nindex f35ac40a84a5..237517baee67 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -128,10 +128,8 @@ struct worktree **get_worktrees(void)\n \tdir = opendir(path.buf);\n \tstrbuf_release(&path);\n \tif (dir) {\n-\t\twhile ((d = readdir(dir)) != NULL) {\n+\t\twhile ((d = readdir_skip_dot_and_dotdot(dir)) != NULL) {\n \t\t\tstruct worktree *linked = NULL;\n-\t\t\tif (is_dot_or_dotdot(d->d_name))\n-\t\t\t\tcontinue;\n \n \t\t\tif ((linked = get_linked_worktree(d->d_name))) {\n \t\t\t\tALLOC_GROW(list, counter + 1, alloc);\n@@ -486,13 +484,9 @@ int submodule_uses_worktrees(const char *path)\n \tif (!dir)\n \t\treturn 0;\n \n-\twhile ((d = readdir(dir)) != NULL) {\n-\t\tif (is_dot_or_dotdot(d->d_name))\n-\t\t\tcontinue;\n-\n+\td = readdir_skip_dot_and_dotdot(dir);\n+\tif (d != NULL)\n \t\tret = 1;\n-\t\tbreak;\n-\t}\n \tclosedir(dir);\n \treturn ret;\n }\n-- \ngitgitgadget\n"},{"id":"425653","messageId":"c45644eaf69a3d753fff7ec1c926133b5ed7fa4b.1622091237.git.gitgitgadget@gmail.com","threadId":"55780","inReplyTo":"pull.961.git.1622091237.gitgitgadget@gmail.com","subject":"[PATCH 1/2] dir: update stale description of treat_directory()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-27T04:53:55Z","receivedAt":"2021-05-27T04:54:07Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <stolee@gmail.com>\n\nThe documentation comment for treat_directory() was originally written\nin 095952 (Teach directory traversal about subprojects, 2007-04-11)\nwhich was before the 'struct dir_struct' split its bitfield of named\noptions into a 'flags' enum in 7c4c97c0 (Turn the flags in struct\ndir_struct into a single variable, 2009-02-16). When those flags\nchanged, the comment became stale, since members like\n'show_other_directories' transitioned into flags like\nDIR_SHOW_OTHER_DIRECTORIES.\n\nUpdate the comments for treat_directory() to use these flag names rather\nthan the old member names.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n dir.c | 13 ++++++-------\n 1 file changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex ed68b7e6412c..ff004b298b2a 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1740,13 +1740,13 @@ static enum exist_status directory_exists_in_index(struct index_state *istate,\n  * Case 3: if we didn't have it in the index previously, we\n  * have a few sub-cases:\n  *\n- *  (a) if \"show_other_directories\" is true, we show it as\n- *      just a directory, unless \"hide_empty_directories\" is\n+ *  (a) if DIR_SHOW_OTHER_DIRECTORIES flag is set, we show it as\n+ *      just a directory, unless DIR_HIDE_EMPTY_DIRECTORIES is\n  *      also true, in which case we need to check if it contains any\n  *      untracked and / or ignored files.\n- *  (b) if it looks like a git directory, and we don't have\n- *      'no_gitlinks' set we treat it as a gitlink, and show it\n- *      as a directory.\n+ *  (b) if it looks like a git directory and we don't have the\n+ *      DIR_NO_GITLINKS flag, then we treat it as a gitlink, and\n+ *      show it as a directory.\n  *  (c) otherwise, we recurse into it.\n  */\n static enum path_treatment treat_directory(struct dir_struct *dir,\n@@ -1834,7 +1834,6 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t\treturn path_recurse;\n \t}\n \n-\t/* This is the \"show_other_directories\" case */\n \tassert(dir->flags & DIR_SHOW_OTHER_DIRECTORIES);\n \n \t/*\n@@ -1849,7 +1848,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t/* Special cases for where this directory is excluded/ignored */\n \tif (excluded) {\n \t\t/*\n-\t\t * In the show_other_directories case, if we're not\n+\t\t * If DIR_SHOW_OTHER_DIRECTORIES is set and we're not\n \t\t * hiding empty directories, there is no need to\n \t\t * recurse into an ignored directory.\n \t\t */\n-- \ngitgitgadget\n\n"}]}