{"thread":{"id":"58409","subject":"[PATCH] builtin/mv.c: fix possible segfault in add_slash()","startedAt":"2022-09-08T23:02:37Z","lastAt":"2022-09-09T22:52:14Z","messageCount":9,"participants":["Shaoxuan Yuan","Jeff King","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"462838","messageId":"20220908230223.239970-1-shaoxuan.yuan02@gmail.com","threadId":"58409","inReplyTo":null,"subject":"[PATCH] builtin/mv.c: fix possible segfault in add_slash()","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-09-08T23:02:23Z","receivedAt":"2022-09-08T23:02:37Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"A possible segfault was introduced in c08830de41 (mv: check if\n is a SKIP_WORKTREE_DIR, 2022-08-09).\n\nWhen running t7001 with SANITIZE=address, problem appears when running:\n\n\tgit mv path1/path2/ .\nor\n\tgit mv directory ../\nor\n\tany that makes dest_path[0] an empty string.\n\nThe add_slash() call segfaults when dest_path[0] is an empty string,\nbecause it was accessing a null value in such case.\n\nChange add_slash() to check the path argument is a non-empty string\nbefore accessing its value.\n\nThe purpose of add_slash() is adding a slash to the end of a string to\nconstruct a directory path. And, because adding a slash to an empty\nstring is of no use here, and checking the string value without checking\nit is non-empty leads to segfault, we should make sure the length of the\nstring is positive to solve both problems.\n\nReported-by: Jeff King \nHelped-by: Jeff King \nSigned-off-by: Shaoxuan Yuan \n---\nReference: https://lore.kernel.org/git/YwdJRRuST2SP8ZT7@coredump.intra.peff.net/\n\n builtin/mv.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 2d64c1e80f..3413ad1c9b 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -71,7 +71,7 @@ static const char **internal_prefix_pathspec(const char *prefix,\n static const char *add_slash(const char *path)\n {\n \tsize_t len = strlen(path);\n-\tif (path[len - 1] != '/') {\n+\tif (len && path[len - 1] != '/') {\n \t\tchar *with_slash = xmalloc(st_add(len, 2));\n \t\tmemcpy(with_slash, path, len);\n \t\twith_slash[len++] = '/';\n\nbase-commit: e71f9b1de6941c8b449d0c0e17e457f999664bc9\n-- \n2.37.0\n\n"},{"id":"462845","messageId":"YxqjRphSqOHbBzGz@coredump.intra.peff.net","threadId":"58409","inReplyTo":"20220908230223.239970-1-shaoxuan.yuan02@gmail.com","subject":"Re: [PATCH] builtin/mv.c: fix possible segfault in add_slash()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-09T02:21:58Z","receivedAt":"2022-09-09T02:22:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 08, 2022 at 04:02:23PM -0700, Shaoxuan Yuan wrote:\n\n> The purpose of add_slash() is adding a slash to the end of a string to\n> construct a directory path. And, because adding a slash to an empty\n> string is of no use here, and checking the string value without checking\n> it is non-empty leads to segfault, we should make sure the length of the\n> string is positive to solve both problems.\n\nThanks for picking this up. I had forgotten about it.\n\nThe patch looks obviously fine to me from the perspective of stopping\nthe segfault. I'll take your \"of no use here\" as a given, not being\nfamiliar with the subtleties of mv's path handling. :) Assuming that's\ncorrect, then everything looks good to me.\n\n-Peff\n"},{"id":"462850","messageId":"3cbfd1b4-7699-1301-042c-fdadea649066@github.com","threadId":"58409","inReplyTo":"20220908230223.239970-1-shaoxuan.yuan02@gmail.com","subject":"Re: [PATCH] builtin/mv.c: fix possible segfault in add_slash()","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-09T14:14:00Z","receivedAt":"2022-09-09T14:14:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/8/2022 7:02 PM, Shaoxuan Yuan wrote:\n> A possible segfault was introduced in c08830de41 (mv: check if\n> is a SKIP_WORKTREE_DIR, 2022-08-09).\n> \n> When running t7001 with SANITIZE=address, problem appears when running:\n> \n> \tgit mv path1/path2/ .\n> or\n> \tgit mv directory ../\n> or\n> \tany that makes dest_path[0] an empty string.\n> \n> The add_slash() call segfaults when dest_path[0] is an empty string,\n> because it was accessing a null value in such case.\n\nIt doesn't _always_ seg fault, since we have tests that cover this\ncase. Adding this change will cause t7001-mv.sh to start failing\nin many places:\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 2d64c1e80fe..8216680ad3c 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -71,6 +71,10 @@ static const char **internal_prefix_pathspec(const char *prefix,\n static const char *add_slash(const char *path)\n {\n \tsize_t len = strlen(path);\n+\n+\tif (!len)\n+\t\tdie(\"segfault?\");\n+\n \tif (path[len - 1] != '/') {\n \t\tchar *with_slash = xmalloc(st_add(len, 2));\n \t\tmemcpy(with_slash, path, len);\n\nI suppose it is better to say \"could segfault\". Running the test\nunder --valgrind also causes a failure. It covers both cases, \".\"\nand \"../\".\n\nThis is all to say that there is some subtlety to the situation, which\nhelps justify the lack of a new test case (the tests cover this case,\nbut require extra steps to show a failure).\n\n> Change add_slash() to check the path argument is a non-empty string\n> before accessing its value.\n> \n> The purpose of add_slash() is adding a slash to the end of a string to\n> construct a directory path. And, because adding a slash to an empty\n> string is of no use here, and checking the string value without checking\n> it is non-empty leads to segfault, we should make sure the length of the\n> string is positive to solve both problems.\n\nI agree that the code change is correct.\n\nThanks,\n-Stolee\n"},{"id":"462869","messageId":"xmqq35d0mrm8.fsf@gitster.g","threadId":"58409","inReplyTo":"3cbfd1b4-7699-1301-042c-fdadea649066@github.com","subject":"Re: [PATCH] builtin/mv.c: fix possible segfault in add_slash()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-09T16:37:03Z","receivedAt":"2022-09-09T16:37:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 9/8/2022 7:02 PM, Shaoxuan Yuan wrote:\n>> A possible segfault was introduced in c08830de41 (mv: check if\n>> is a SKIP_WORKTREE_DIR, 2022-08-09).\n>> \n>> When running t7001 with SANITIZE=address, problem appears when running:\n>> \n>> \tgit mv path1/path2/ .\n>> or\n>> \tgit mv directory ../\n>> or\n>> \tany that makes dest_path[0] an empty string.\n>> \n>> The add_slash() call segfaults when dest_path[0] is an empty string,\n>> because it was accessing a null value in such case.\n>\n> It doesn't _always_ seg fault, since we have tests that cover this\n> case. Adding this change will cause t7001-mv.sh to start failing\n> in many places:\n>\n> diff --git a/builtin/mv.c b/builtin/mv.c\n> index 2d64c1e80fe..8216680ad3c 100644\n> --- a/builtin/mv.c\n> +++ b/builtin/mv.c\n> @@ -71,6 +71,10 @@ static const char **internal_prefix_pathspec(const char *prefix,\n> static const char *add_slash(const char *path)\n> {\n> \tsize_t len = strlen(path);\n> +\n> +\tif (!len)\n> +\t\tdie(\"segfault?\");\n> +\n> \tif (path[len - 1] != '/') {\n> \t\tchar *with_slash = xmalloc(st_add(len, 2));\n> \t\tmemcpy(with_slash, path, len);\n>\n> I suppose it is better to say \"could segfault\". Running the test\n> under --valgrind also causes a failure. It covers both cases, \".\"\n> and \"../\".\n\nWhile \"could segfault\" is of course more correct, I do not see a\nhuge difference here, but that is only because I learned to equate\n\"segfaults\" in our log messages with \"makes an access to\ninappropriate memory address\".\n\nIf I were to suggest updating the proposed log message, I would\nrather spend a bit more bytes to explain what callers expect\nadd_slash() to do, why they call the helper for. It would make it\nobvious why it is the right behaviour the callers expect for the\nfunction to return an empty string as-is.\n\nI _think_ the reason is that the caller of add_slash has the name of\na directory in the working tree (relative to the root of the working\ntree) and wants to add strings to form pathnames to things in the\ndirectory. They have \"Documentation\" directory and are told to move\n\"Makefile\" from somewhere into it, so they pass \"Documentation\",\nwant \"Documentation/\" back, to form \"Documentation/Makefile\" by\nconcatenating. If they are told to move something to the toplevel,\nthe target would be originally given as \".\" and while driving the\nmachinery to rename something to \"./Makefile\" might also work,\nbecause the pathnames are normalized fairly early by removing excess\ndots and resolving double-dots, the actual 'path' passed to\nadd_slash() by the caller in this case is an empty string, not a\nsingle dot. And \"move this Makefile sitting somewhere else to .\"\nmeans \"the path to the resulting file is Makefile\" (as opposed to\n\"the path to the resulting file is ./Makefile\"), which is correct.\n\nOf course, I expect the log message to explain it a lot more\nconcisely, instead of spending more than a dozen lines ;-)\n\nThanks.\n\n"},{"id":"462885","messageId":"20220909194458.264735-1-shaoxuan.yuan02@gmail.com","threadId":"58409","inReplyTo":"20220908230223.239970-1-shaoxuan.yuan02@gmail.com","subject":"[PATCH v2] builtin/mv.c: fix possible segfault in add_slash()","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-09-09T19:44:58Z","receivedAt":"2022-09-09T19:47:47Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"A possible segfault was introduced in c08830de41 (mv: check if\n is a SKIP_WORKTREE_DIR, 2022-08-09).\n\nWhen running t7001 with SANITIZE=address, problem appears when running:\n\n\tgit mv path1/path2/ .\nor\n\tgit mv directory ../\nor\n\tany that makes dest_path[0] an empty string.\n\nThe add_slash() call could segfault when dest_path[0] is an empty string,\nbecause it was accessing a null value in such case.\n\nChange add_slash() to check the path argument is a non-empty string\nbefore accessing its value. If the path is empty, return it as-is.\n\nExplanation:\n\nIt's OK for add_slash() to return an empty string as-is. add_slash()\nconverts its path argument to the prefix (for \"folder1/file1\",\n\"folder1/\" is the prefix we mean here) for the result path. The path\nargument is an empty string _iff_ the result path is analyzed to be at\nthe top level (this normalization process is done earlier by\ninternal_prefix_pathspec()).\n\nBecause the prefix for a top-level path is an empty string, thus\nadd_slash() should return an empty path argument as-is, both for\ncorrectness and avoiding inappropriate memory access.\n\nReported-by: Jeff King \nHelped-by: Jeff King \nHelped-by: Junio C Hamano \nHelped-by: Derrick Stolee \nSigned-off-by: Shaoxuan Yuan \n---\nRange-diff against v1:\n1: a5dccc030c ! 1: 82353f457d builtin/mv.c: fix possible segfault in add_slash()\n @@ Commit message\n or\n any that makes dest_path[0] an empty string.\n \n - The add_slash() call segfaults when dest_path[0] is an empty string,\n + The add_slash() call could segfault when dest_path[0] is an empty string,\n because it was accessing a null value in such case.\n \n Change add_slash() to check the path argument is a non-empty string\n - before accessing its value.\n + before accessing its value. If the path is empty, return it as-is.\n \n - The purpose of add_slash() is adding a slash to the end of a string to\n - construct a directory path. And, because adding a slash to an empty\n - string is of no use here, and checking the string value without checking\n - it is non-empty leads to segfault, we should make sure the length of the\n - string is positive to solve both problems.\n + Explanation:\n +\n + It's OK for add_slash() to return an empty string as-is. add_slash()\n + converts its path argument to the prefix (for \"folder1/file1\",\n + \"folder1/\" is the prefix we mean here) for the result path. The path\n + argument is an empty string _iff_ the result path is analyzed to be at\n + the top level (this normalization process is done earlier by\n + internal_prefix_pathspec()).\n +\n + Because the prefix for a top-level path is an empty string, thus\n + add_slash() should return an empty path argument as-is, both for\n + correctness and avoiding inappropriate memory access.\n \n Reported-by: Jeff King \n Helped-by: Jeff King \n + Helped-by: Junio C Hamano \n + Helped-by: Derrick Stolee \n Signed-off-by: Shaoxuan Yuan \n \n ## builtin/mv.c ##\n\n builtin/mv.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 2d64c1e80f..3413ad1c9b 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -71,7 +71,7 @@ static const char **internal_prefix_pathspec(const char *prefix,\n static const char *add_slash(const char *path)\n {\n \tsize_t len = strlen(path);\n-\tif (path[len - 1] != '/') {\n+\tif (len && path[len - 1] != '/') {\n \t\tchar *with_slash = xmalloc(st_add(len, 2));\n \t\tmemcpy(with_slash, path, len);\n \t\twith_slash[len++] = '/';\n\nbase-commit: e71f9b1de6941c8b449d0c0e17e457f999664bc9\n-- \n2.37.0\n\n"},{"id":"462886","messageId":"xmqqo7voiaaf.fsf@gitster.g","threadId":"58409","inReplyTo":"20220909194458.264735-1-shaoxuan.yuan02@gmail.com","subject":"Re: [PATCH v2] builtin/mv.c: fix possible segfault in add_slash()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-09T20:04:56Z","receivedAt":"2022-09-09T20:05:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shaoxuan Yuan writes:\n\n> A possible segfault was introduced in c08830de41 (mv: check if\n> is a SKIP_WORKTREE_DIR, 2022-08-09).\n>\n> When running t7001 with SANITIZE=address, problem appears when running:\n>\n> \tgit mv path1/path2/ .\n> or\n> \tgit mv directory ../\n> or\n> \tany that makes dest_path[0] an empty string.\n>\n> The add_slash() call could segfault when dest_path[0] is an empty string,\n> because it was accessing a null value in such case.\n\nTerminology. The relevant preimage is\n\n> \tsize_t len = strlen(path);\n> -\tif (path[len - 1] != '/') {\n\nAn access to path[-1] is an out-of-bounds access.\n\n> Change add_slash() to check the path argument is a non-empty string\n> before accessing its value. If the path is empty, return it as-is.\n\nThat is not wrong per-se, but...\n\n> Explanation:\n\n... you'd need this funny label here. If this is where your\nexplanation begins, what was the reader reading before it? ;-)\n\nThe logic would flow more naturally if you added your \"explanation\"\nmaterial between \"what is wrong in the current code\" and \"what to do\nto fix it\", perhaps like so:\n\n\t... could segfault when path argument to it is an empty\n\tstring, because it makes an out-of-bounds read to decide if\n\tan extra slash '/' needs to be appended to it.\n\n\tAs add_slash() is used to make sure that a valid pathname to\n\ta file in the given directory can be made by appending a\n\tfilename after the value returned from it, if path is an\n\tempty string, we want to return it as-is. The path to a\n\tfile \"F\" in the top-level of the working tree (i.e.\n\tpath==\"\") is formed by appending \"F\" after \"\" (i.e. path)\n\twithout any slash in between.\n\n\tSo, just like the case where a non-empty path already ends\n\twith a slash, return an empty path as-is.\n\n\n> diff --git a/builtin/mv.c b/builtin/mv.c\n> index 2d64c1e80f..3413ad1c9b 100644\n> --- a/builtin/mv.c\n> +++ b/builtin/mv.c\n> @@ -71,7 +71,7 @@ static const char **internal_prefix_pathspec(const char *prefix,\n> static const char *add_slash(const char *path)\n> {\n> \tsize_t len = strlen(path);\n> -\tif (path[len - 1] != '/') {\n> +\tif (len && path[len - 1] != '/') {\n> \t\tchar *with_slash = xmalloc(st_add(len, 2));\n> \t\tmemcpy(with_slash, path, len);\n> \t\twith_slash[len++] = '/';\n\nYup. It cannot be seen in the patch but the post-context of this\nhunk just returns path as-is, which is what we want to happen.\n\nThanks.\n"},{"id":"462887","messageId":"20220909222736.279362-1-shaoxuan.yuan02@gmail.com","threadId":"58409","inReplyTo":"20220908230223.239970-1-shaoxuan.yuan02@gmail.com","subject":"[PATCH v3] builtin/mv.c: fix possible segfault in add_slash()","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-09-09T22:27:36Z","receivedAt":"2022-09-09T22:37:26Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"A possible segfault was introduced in c08830de41 (mv: check if\n is a SKIP_WORKTREE_DIR, 2022-08-09).\n\nWhen running t7001 with SANITIZE=address, problem appears when running:\n\n\tgit mv path1/path2/ .\nor\n\tgit mv directory ../\nor\n\tany that makes dest_path[0] an empty string.\n\nThe add_slash() call could segfault when path argument to it is an empty\nstring, because it makes an out-of-bounds read to decide if an extra\nslash '/' needs to be appended to it.\n\nAs add_slash() is used to make sure that a valid pathname to a file in\nthe given directory can be made by appending a filename after the value\nreturned from it, if path is an empty string, we want to return it\nas-is. The path to a file \"F\" in the top-level of the working tree\n(i.e. path==\"\") is formed by appending \"F\" after \"\" (i.e. path) without\nany slash in between.\n\nSo, just like the case where a non-empty path already ends with a slash,\nreturn an empty path as-is.\n\nReported-by: Jeff King \nHelped-by: Jeff King \nHelped-by: Junio C Hamano \nHelped-by: Derrick Stolee \nSigned-off-by: Shaoxuan Yuan \n---\nRange-diff against v2:\n1: 1120dc7e6b ! 1: 569e618013 builtin/mv.c: fix possible segfault in add_slash()\n @@ Commit message\n or\n any that makes dest_path[0] an empty string.\n \n - The add_slash() call could segfault when dest_path[0] is an empty string,\n - because it was accessing a null value in such case.\n -\n - Change add_slash() to check the path argument is a non-empty string\n - before accessing its value. If the path is empty, return it as-is.\n -\n - Explanation:\n -\n - It's OK for add_slash() to return an empty string as-is. add_slash()\n - converts its path argument to the prefix (for \"folder1/file1\",\n - \"folder1/\" is the prefix we mean here) for the result path. The path\n - argument is an empty string _iff_ the result path is analyzed to be at\n - the top level (this normalization process is done earlier by\n - internal_prefix_pathspec()).\n -\n - Because the prefix for a top-level path is an empty string, thus\n - add_slash() should return an empty path argument as-is, both for\n - correctness and avoiding inappropriate memory access.\n + The add_slash() call could segfault when path argument to it is an empty\n + string, because it makes an out-of-bounds read to decide if an extra\n + slash '/' needs to be appended to it.\n +\n + As add_slash() is used to make sure that a valid pathname to a file in\n + the given directory can be made by appending a filename after the value\n + returned from it, if path is an empty string, we want to return it\n + as-is. The path to a file \"F\" in the top-level of the working tree\n + (i.e. path==\"\") is formed by appending \"F\" after \"\" (i.e. path) without\n + any slash in between.\n +\n + So, just like the case where a non-empty path already ends with a slash,\n + return an empty path as-is.\n \n Reported-by: Jeff King \n Helped-by: Jeff King \n\n builtin/mv.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 2d64c1e80f..3413ad1c9b 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -71,7 +71,7 @@ static const char **internal_prefix_pathspec(const char *prefix,\n static const char *add_slash(const char *path)\n {\n \tsize_t len = strlen(path);\n-\tif (path[len - 1] != '/') {\n+\tif (len && path[len - 1] != '/') {\n \t\tchar *with_slash = xmalloc(st_add(len, 2));\n \t\tmemcpy(with_slash, path, len);\n \t\twith_slash[len++] = '/';\n\nbase-commit: a6b4b080e4ef65ebbab73e47c0100b5dc12e104c\n-- \n2.37.0\n\n"},{"id":"462888","messageId":"150fabf7-0b5b-9029-0d60-f83885c0cc41@gmail.com","threadId":"58409","inReplyTo":"xmqqo7voiaaf.fsf@gitster.g","subject":"Re: [PATCH v2] builtin/mv.c: fix possible segfault in add_slash()","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-09-09T22:40:43Z","receivedAt":"2022-09-09T22:40:51Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"On 9/9/2022 1:04 PM, Junio C Hamano wrote:\n> Shaoxuan Yuan writes:\n> \n>> A possible segfault was introduced in c08830de41 (mv: check if\n>> is a SKIP_WORKTREE_DIR, 2022-08-09).\n>>\n>> When running t7001 with SANITIZE=address, problem appears when running:\n>>\n>> \tgit mv path1/path2/ .\n>> or\n>> \tgit mv directory ../\n>> or\n>> \tany that makes dest_path[0] an empty string.\n>>\n>> The add_slash() call could segfault when dest_path[0] is an empty string,\n>> because it was accessing a null value in such case.\n> \n> Terminology. The relevant preimage is\n> \n>> \tsize_t len = strlen(path);\n>> -\tif (path[len - 1] != '/') {\n> \n> An access to path[-1] is an out-of-bounds access.\n\nThanks for the term, new thing learned :-)\n\n>> Change add_slash() to check the path argument is a non-empty string\n>> before accessing its value. If the path is empty, return it as-is.\n> \n> That is not wrong per-se, but...\n> \n>> Explanation:\n> \n> ... you'd need this funny label here. If this is where your\n> explanation begins, what was the reader reading before it? ;-)\n> \n> The logic would flow more naturally if you added your \"explanation\"\n> material between \"what is wrong in the current code\" and \"what to do\n> to fix it\", perhaps like so:\n\nIndeed, explanation before action sounds more reasonable.\n\n> \t... could segfault when path argument to it is an empty\n> \tstring, because it makes an out-of-bounds read to decide if\n> \tan extra slash '/' needs to be appended to it.\n> \n> \tAs add_slash() is used to make sure that a valid pathname to\n> \ta file in the given directory can be made by appending a\n> \tfilename after the value returned from it, if path is an\n> \tempty string, we want to return it as-is. The path to a\n> \tfile \"F\" in the top-level of the working tree (i.e.\n> \tpath==\"\") is formed by appending \"F\" after \"\" (i.e. path)\n> \twithout any slash in between.\n> \n> \tSo, just like the case where a non-empty path already ends\n> \twith a slash, return an empty path as-is.\n> \n\nThanks for the paraphrase, I put it in the v3 just sent.\n\n>> diff --git a/builtin/mv.c b/builtin/mv.c\n>> index 2d64c1e80f..3413ad1c9b 100644\n>> --- a/builtin/mv.c\n>> +++ b/builtin/mv.c\n>> @@ -71,7 +71,7 @@ static const char **internal_prefix_pathspec(const char *prefix,\n>> static const char *add_slash(const char *path)\n>> {\n>> \tsize_t len = strlen(path);\n>> -\tif (path[len - 1] != '/') {\n>> +\tif (len && path[len - 1] != '/') {\n>> \t\tchar *with_slash = xmalloc(st_add(len, 2));\n>> \t\tmemcpy(with_slash, path, len);\n>> \t\twith_slash[len++] = '/';\n> \n> Yup. It cannot be seen in the patch but the post-context of this\n> hunk just returns path as-is, which is what we want to happen.\n\nYes.\n\nThanks,\nShaoxuan\n"},{"id":"462889","messageId":"xmqqczc4i2jq.fsf@gitster.g","threadId":"58409","inReplyTo":"20220909222736.279362-1-shaoxuan.yuan02@gmail.com","subject":"Re: [PATCH v3] builtin/mv.c: fix possible segfault in add_slash()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-09T22:52:09Z","receivedAt":"2022-09-09T22:52:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shaoxuan Yuan writes:\n\n> A possible segfault was introduced in c08830de41 (mv: check if\n> is a SKIP_WORKTREE_DIR, 2022-08-09).\n\nThis iteration looks good to me (v1 was sufficiently readable\nalready but an extra explanation made it easier to grok).\n\nThanks, will queue and let's merge it to 'next'.\n"}]}