{"thread":{"id":"60080","subject":"[PATCH] fix `git mv existing-dir non-existing-dir`*","startedAt":"2023-08-08T17:12:44Z","lastAt":"2023-08-12T01:14:42Z","messageCount":7,"participants":["Sebastian Thiel via GitGitGadget","Junio C Hamano","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"480292","messageId":"pull.1561.git.1691506431114.gitgitgadget@gmail.com","threadId":"60080","inReplyTo":null,"subject":"[PATCH] fix `git mv existing-dir non-existing-dir`*","fromName":"Sebastian Thiel via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-08T14:53:50Z","receivedAt":"2023-08-08T17:12:44Z","isPatch":true,"sender":{"key":"sebastian.thiel@icloud.com","avatar":"https://avatars.githubusercontent.com/u/63622?v=4"},"body":"From: Sebastian Thiel \n\n*in some environments.\n\nWhen moving a directory onto another with `gix mv`\nvarious checks are performed. One of of these\nvalidates that the destination is not an existing\nfile.\n\nWhen calling `lstat` on the destination path and\nit fails as the path doesn't exist, some\nenvironments seem to overwrite the passed in\n`stat` memory nonetheless.\n(I observed this issue on debian 12 of x86_64,\nrunning on OrbStack on ARM, emulated with Rosetta)\n\nThis would affect the code that followed as it\nwould still acccess a now\nmodified `st` structure, which now seems to\ncontain uninitialized memory.\n`S_ISDIR(st_dir_mode)` would then typically\nreturn false causing the code to run into a bad\ncase.\n\nThe fix avoids overwriting the existing `st`\nstructure, providing an alternative that exists\nonly for that purpose.\n\nNote that this patch minimizes complexity instead of stack-size.\n\nSigned-off-by: Sebastian Thiel \n---\n fix git mv existing-dir non-existing-dir*\n \n fix git mv existing-dir non-existing-dir*\n \n *in some environments.\n \n When moving a directory onto another with gix mv various checks are\n performed. One of of these validates that the destination is not an\n existing file.\n \n When calling lstat on the destination path and it fails as the path\n doesn't exist, some environments seem to overwrite the passed in stat\n memory nonetheless. (I observed this issue on debian 12 of x86_64,\n running on OrbStack on ARM, emulated with Rosetta)\n \n This would affect the code that followed as it would still acccess a now\n modified st structure, which now seems to contain uninitialized memory.\n S_ISDIR(st_dir_mode) would then typically return false causing the code\n to run into a bad case.\n \n The fix avoids overwriting the existing st structure, providing an\n alternative that exists only for that purpose.\n \n ------------------------------------------------------------------------\n \n It's worth pointing out that the test demonstrates this case only if one\n happens to execute it in one of the environments that happen to have an\n lstat that writes into stat even on error. Thus it already worked for me\n on MacOS, even without the patch applied, which matches my observation\n that a certain script works there but doesn't work on the VM.\n \n Even though the patch now minimizes size, I can imagine one might\n instead want to rather copy st.st_mode to protect only the relevant\n field from being affected by potential rewrites of st later on.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1561%2FByron%2Ffix-mv-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1561/Byron/fix-mv-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1561\n\n builtin/mv.c | 4 ++--\n t/t7001-mv.sh | 6 ++++++\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex fa84fcb20d8..05e7156034e 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -184,7 +184,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tint src_dir_nr = 0, src_dir_alloc = 0;\n \tstruct strbuf a_src_dir = STRBUF_INIT;\n \tenum update_mode *modes, dst_mode = 0;\n-\tstruct stat st;\n+\tstruct stat st, dest_st;\n \tstruct string_list src_for_dst = STRING_LIST_INIT_NODUP;\n \tstruct lock_file lock_file = LOCK_INIT;\n \tstruct cache_entry *ce;\n@@ -304,7 +304,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\tgoto act_on_entry;\n \t\t}\n \t\tif (S_ISDIR(st.st_mode)\n-\t\t && lstat(dst, &st) == 0) {\n+\t\t && lstat(dst, &dest_st) == 0) {\n \t\t\tbad = _(\"cannot move directory over file\");\n \t\t\tgoto act_on_entry;\n \t\t}\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 898a9205328..9894bc45ee6 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -174,6 +174,12 @@ test_expect_success 'do not move directory over existing directory' '\n \ttest_must_fail git mv path2 path0\n '\n \n+test_expect_success 'rename directory to non-existing directory' '\n+\tmkdir dir-a && touch dir-a/f &&\n+\tgit add dir-a &&\n+\tgit mv dir-a non-existing-dir\n+'\n+\n test_expect_success 'move into \".\"' '\n \tgit mv path1/path2/ .\n '\n\nbase-commit: 1b0a5129563ebe720330fdc8f5c6843d27641137\n-- \ngitgitgadget\n"},{"id":"480301","messageId":"xmqqy1il77wp.fsf@gitster.g","threadId":"60080","inReplyTo":"pull.1561.git.1691506431114.gitgitgadget@gmail.com","subject":"Re: [PATCH] fix `git mv existing-dir non-existing-dir`*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-08T17:36:54Z","receivedAt":"2023-08-08T18:25:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Sebastian Thiel via GitGitGadget\" writes:\n\n> From: Sebastian Thiel \n>\n> *in some environments.\n\nPlease do not chop a single sentence in the middle and mark that\nfact with an asterisk nobody understands what it means.\n\n Subject: [PATCH] mv: handle lstat() failure correctly\n\nperhaps?\n\n> When moving a directory onto another with `gix mv` various checks\n> are performed. One of of these validates that the destination is\n> not an existing file.\n>\n> When calling `lstat` on the destination path and it fails as the\n> path doesn't exist, some environments seem to overwrite the passed\n> in `stat` memory nonetheless. (I observed this issue on debian 12\n> of x86_64, running on OrbStack on ARM, emulated with Rosetta)\n\nVery cleanly written, except \"gix\" -> \"git\".\n\nPOSIX does not seem to specify what should happen to buf when the\ncall fails, which I take to mean that its contents can become any\ngarbage at that point.\n\n> diff --git a/builtin/mv.c b/builtin/mv.c\n> index fa84fcb20d8..05e7156034e 100644\n> --- a/builtin/mv.c\n> +++ b/builtin/mv.c\n> @@ -184,7 +184,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n> \tint src_dir_nr = 0, src_dir_alloc = 0;\n> \tstruct strbuf a_src_dir = STRBUF_INIT;\n> \tenum update_mode *modes, dst_mode = 0;\n> -\tstruct stat st;\n> +\tstruct stat st, dest_st;\n> \tstruct string_list src_for_dst = STRING_LIST_INIT_NODUP;\n> \tstruct lock_file lock_file = LOCK_INIT;\n> \tstruct cache_entry *ce;\n> @@ -304,7 +304,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n> \t\t\tgoto act_on_entry;\n> \t\t}\n> \t\tif (S_ISDIR(st.st_mode)\n> -\t\t && lstat(dst, &st) == 0) {\n> +\t\t && lstat(dst, &dest_st) == 0) {\n\nThis is good. After this \"if (S_ISDIR)\" thing, there is another \"if\n(S_ISDIR)\" on the same st.st_mode, so clobbering st like the\noriginal was a stupid thing to do.\n\n> \t\t\tbad = _(\"cannot move directory over file\");\n\nWhat is curious is that dest_st.st_mode, after lstat on dst\nsucceeds, is never checked, even though the error message claims\nthat it detected an attempt to move directory over file. What\nshould happen when the user did this then?\n\n $ git mv existing-dir another-existing-dir\n\nShouldn't it do something similar to\n\n $ mv D1 D2\n\nwhich is to move the entire hierarchy of D1 and make it appear at\nD2/D1?\n\nEven if the answer to the above question is \"yes\", that is a\nseparate bugfix, so let's not get distracted and see how our\ntest looks like.\n\n> \t\t\tgoto act_on_entry;\n> \t\t}\n> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\n> index 898a9205328..9894bc45ee6 100755\n> --- a/t/t7001-mv.sh\n> +++ b/t/t7001-mv.sh\n> @@ -174,6 +174,12 @@ test_expect_success 'do not move directory over existing directory' '\n> \ttest_must_fail git mv path2 path0\n> '\n> \n> +test_expect_success 'rename directory to non-existing directory' '\n> +\tmkdir dir-a && touch dir-a/f &&\n\nOne command per line, and reserve the use of \"touch\" to cases where\nyou care about the timestamps, not existence. I.e.\n\n\tmkdir dir-a &&\n\t>dir-a/f &&\n\n> +\tgit add dir-a &&\n> +\tgit mv dir-a non-existing-dir\n> +'\n\nOK, there is no guarantee that this would fail on a system whose\nlstat() may clobber buf when it notices that the path does not\nexist, but it is a good test to have.\n\nThanks.\n"},{"id":"480312","messageId":"xmqqttt9740q.fsf@gitster.g","threadId":"60080","inReplyTo":"xmqqy1il77wp.fsf@gitster.g","subject":"Re: [PATCH] fix `git mv existing-dir non-existing-dir`*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-08T19:00:53Z","receivedAt":"2023-08-08T19:36:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> What is curious is that dest_st.st_mode, after lstat on dst\n> succeeds, is never checked, even though the error message claims\n> that it detected an attempt to move directory over file. What\n> should happen when the user did this then?\n>\n> $ git mv existing-dir another-existing-dir\n>\n> Shouldn't it do something similar to\n>\n> $ mv D1 D2\n>\n> which is to move the entire hierarchy of D1 and make it appear at\n> D2/D1?\n\nAh, that case is handled in a different codepath, so this lstat of\ndst needs to check only the existence. So there is no (unrelated)\nbug there. Sorry for the noise.\n"},{"id":"480322","messageId":"20230808184054.cjhiboifschkwuoz@tb-raspi4","threadId":"60080","inReplyTo":"xmqqy1il77wp.fsf@gitster.g","subject":"Re: [PATCH] fix `git mv existing-dir non-existing-dir`*","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2023-08-08T18:40:54Z","receivedAt":"2023-08-08T20:09:18Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Tue, Aug 08, 2023 at 10:36:54AM -0700, Junio C Hamano wrote:\n> \"Sebastian Thiel via GitGitGadget\" writes:\n>\n> > From: Sebastian Thiel \n> >\n\nThe patch makes sense to me, Junio's comments included.\n\n> Shouldn't it do something similar to\n>\n> $ mv D1 D2\n\nCouldn't resist to test it ;-)\n\nThe result would be\n renamed: D1/file1 -> D2/D1/file1\n\n\n"},{"id":"480342","messageId":"xmqqmsz16w1q.fsf@gitster.g","threadId":"60080","inReplyTo":"20230808184054.cjhiboifschkwuoz@tb-raspi4","subject":"Re: [PATCH] fix `git mv existing-dir non-existing-dir`*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-08T21:53:05Z","receivedAt":"2023-08-08T21:53:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> On Tue, Aug 08, 2023 at 10:36:54AM -0700, Junio C Hamano wrote:\n>> \"Sebastian Thiel via GitGitGadget\" writes:\n>>\n>> > From: Sebastian Thiel \n>> >\n>\n> The patch makes sense to me, Junio's comments included.\n>\n>> Shouldn't it do something similar to\n>>\n>> $ mv D1 D2\n>\n> Couldn't resist to test it ;-)\n>\n> The result would be\n> renamed: D1/file1 -> D2/D1/file1\n\nSure. The lstat() in question is about the case where a different\nD2/D1 already exists, either as a file (which will definitely break\nas we do not and should not do unlink-and-then-mkdir) or as a\ndirectory (which may be OK in some cases to get a union of the\ncontents in the original D1 and D2/D1, but in general not a good\nidea).\n\nAnd in the latter case, i.e. when D2/D1 exists as a directory, we\nshould not say \"cannot move directory over file\". So, the check\nthat does not care what the dest_dir's type is fine. but the error\nmessage is wrong.\n\n \"cannot move directory over file, source=D1, destination=D1/D2\"\n\nis the message we would get in such a case. We probably just should\nsay\n\n \"destination already exists, source=D1, destination=D1/D2\"\n\nor something like that.\n"},{"id":"480350","messageId":"pull.1561.v2.git.1691567261701.gitgitgadget@gmail.com","threadId":"60080","inReplyTo":"pull.1561.git.1691506431114.gitgitgadget@gmail.com","subject":"[PATCH v2] fix `git mv existing-dir non-existing-dir` in some environments.","fromName":"Sebastian Thiel via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-09T07:47:41Z","receivedAt":"2023-08-09T07:47:49Z","isPatch":true,"sender":{"key":"sebastian.thiel@icloud.com","avatar":"https://avatars.githubusercontent.com/u/63622?v=4"},"body":"From: Sebastian Thiel \n\nWhen moving a directory onto another with `git mv` various checks are\nperformed. One of of these validates that the destination is not existing.\n\nWhen calling `lstat` on the destination path and it fails as the path\ndoesn't exist, some environments seem to overwrite the passed in\n`stat` memory nonetheless (I observed this issue on debian 12 of x86_64,\nrunning on OrbStack on ARM, emulated with Rosetta).\n\nThis would affect the code that followed as it would still acccess a now\nmodified `st` structure, which now seems to contain uninitialized memory.\n`S_ISDIR(st_dir_mode)` would then typically return false causing the code\nto run into a bad case.\n\nThe fix avoids overwriting the existing `st` structure, providing an\nalternative that exists only for that purpose.\n\nNote that this patch minimizes complexity instead of stack-frame size.\n\nSigned-off-by: Sebastian Thiel \n---\n [PATCH] mv: handle lstat() failure correctly\n \n When moving a directory onto another with git mv various checks are\n performed. One of of these validates that the destination is not\n existing.\n \n When calling lstat on the destination path and it fails as the path\n doesn't exist, some environments seem to overwrite the passed in stat\n memory nonetheless (I observed this issue on debian 12 of x86_64,\n running on OrbStack on ARM, emulated with Rosetta).\n \n This would affect the code that followed as it would still acccess a now\n modified st structure, which now seems to contain uninitialized memory.\n S_ISDIR(st_dir_mode) would then typically return false causing the code\n to run into a bad case.\n \n The fix avoids overwriting the existing st structure, providing an\n alternative that exists only for that purpose.\n \n \n Note that this patch minimizes complexity instead of stack-frame size.\n ======================================================================\n \n It's worth pointing out that the test demonstrates this case only if one\n happens to execute it in one of the environments that happen to have an\n lstat that writes into stat even on error. Thus it already worked for me\n on MacOS, even without the patch applied, which matches my observation\n that a certain script works there but doesn't work on the VM.\n \n Even though the patch now minimizes size, I can imagine one might\n instead want to rather copy st.st_mode to protect only the relevant\n field from being affected by potential rewrites of st later on.\n \n Changes since v1:\n \n * replaced previous title with recommendation by Junio C Hermano\n * improved formatting of commit message and renamed gix to git. Let's\n call that a typo\n * apply Junio C Hermano's suggestions to test-case\n * I refrained from changing the error message as this would mean all\n translations need adjustment (and I don't know how this is tracked\n then)\n \n I also want to apologise for the possibly terrible formatting and the\n repetition - it feels strange but is what gitgadget seems to suggests.\n Further, it's my honour to submit a patch to git and interact with the\n maintainers, it's like meeting my idols!\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1561%2FByron%2Ffix-mv-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1561/Byron/fix-mv-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1561\n\nRange-diff vs v1:\n\n 1: ad0e6754e2d ! 1: 8908fd228fe fix `git mv existing-dir non-existing-dir`*\n @@ Metadata\n Author: Sebastian Thiel \n \n ## Commit message ##\n - fix `git mv existing-dir non-existing-dir`*\n + fix `git mv existing-dir non-existing-dir` in some environments.\n \n - *in some environments.\n + When moving a directory onto another with `git mv` various checks are\n + performed. One of of these validates that the destination is not existing.\n \n - When moving a directory onto another with `gix mv`\n - various checks are performed. One of of these\n - validates that the destination is not an existing\n - file.\n + When calling `lstat` on the destination path and it fails as the path\n + doesn't exist, some environments seem to overwrite the passed in\n + `stat` memory nonetheless (I observed this issue on debian 12 of x86_64,\n + running on OrbStack on ARM, emulated with Rosetta).\n \n - When calling `lstat` on the destination path and\n - it fails as the path doesn't exist, some\n - environments seem to overwrite the passed in\n - `stat` memory nonetheless.\n - (I observed this issue on debian 12 of x86_64,\n - running on OrbStack on ARM, emulated with Rosetta)\n + This would affect the code that followed as it would still acccess a now\n + modified `st` structure, which now seems to contain uninitialized memory.\n + `S_ISDIR(st_dir_mode)` would then typically return false causing the code\n + to run into a bad case.\n \n - This would affect the code that followed as it\n - would still acccess a now\n - modified `st` structure, which now seems to\n - contain uninitialized memory.\n - `S_ISDIR(st_dir_mode)` would then typically\n - return false causing the code to run into a bad\n - case.\n + The fix avoids overwriting the existing `st` structure, providing an\n + alternative that exists only for that purpose.\n \n - The fix avoids overwriting the existing `st`\n - structure, providing an alternative that exists\n - only for that purpose.\n -\n - Note that this patch minimizes complexity instead of stack-size.\n + Note that this patch minimizes complexity instead of stack-frame size.\n \n Signed-off-by: Sebastian Thiel \n \n @@ t/t7001-mv.sh: test_expect_success 'do not move directory over existing director\n '\n \n +test_expect_success 'rename directory to non-existing directory' '\n -+\tmkdir dir-a && touch dir-a/f &&\n ++\tmkdir dir-a &&\n ++\t>dir-a/f &&\n +\tgit add dir-a &&\n +\tgit mv dir-a non-existing-dir\n +'\n\n\n builtin/mv.c | 4 ++--\n t/t7001-mv.sh | 7 +++++++\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex fa84fcb20d8..05e7156034e 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -184,7 +184,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tint src_dir_nr = 0, src_dir_alloc = 0;\n \tstruct strbuf a_src_dir = STRBUF_INIT;\n \tenum update_mode *modes, dst_mode = 0;\n-\tstruct stat st;\n+\tstruct stat st, dest_st;\n \tstruct string_list src_for_dst = STRING_LIST_INIT_NODUP;\n \tstruct lock_file lock_file = LOCK_INIT;\n \tstruct cache_entry *ce;\n@@ -304,7 +304,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\tgoto act_on_entry;\n \t\t}\n \t\tif (S_ISDIR(st.st_mode)\n-\t\t && lstat(dst, &st) == 0) {\n+\t\t && lstat(dst, &dest_st) == 0) {\n \t\t\tbad = _(\"cannot move directory over file\");\n \t\t\tgoto act_on_entry;\n \t\t}\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 898a9205328..f136ea76f7f 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -174,6 +174,13 @@ test_expect_success 'do not move directory over existing directory' '\n \ttest_must_fail git mv path2 path0\n '\n \n+test_expect_success 'rename directory to non-existing directory' '\n+\tmkdir dir-a &&\n+\t>dir-a/f &&\n+\tgit add dir-a &&\n+\tgit mv dir-a non-existing-dir\n+'\n+\n test_expect_success 'move into \".\"' '\n \tgit mv path1/path2/ .\n '\n\nbase-commit: 1b0a5129563ebe720330fdc8f5c6843d27641137\n-- \ngitgitgadget\n"},{"id":"480620","messageId":"xmqqjzu1njt0.fsf@gitster.g","threadId":"60080","inReplyTo":"xmqqmsz16w1q.fsf@gitster.g","subject":"[PATCH] mv: fix error for moving directory to another","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-12T01:14:35Z","receivedAt":"2023-08-12T01:14:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"If both directories D1 and D2 already exists, and further there is a\nfilesystem entity D2/D1, \"git mv D1 D2\" would fail, and we get an\nerror message that says:\n\n \"cannot move directory over file, source=D1, destination=D2/D1\"\n\nregardless of the type of existing \"D2/D1\". If it is a file, the\nmessage is correct, but if it is a directory, it is not (we could\nmake the D2/D1 directory a union of its original contents and what\nwas in D1/, but that is not what we do).\n\nThe code that decies to issue the error message only checks for\nexistence of \"D2/D1\" and does not care what kind of thing sits at\nthe path.\n\nRephrase the message to say\n\n \"destination already exists, source=D1, destination=D2/D1\"\n\nthat would be suitable for any kind of thing being in the way.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Just so that we do not forget what we discussed. It is kind of\n interesting that no tests need adjustment for this change, which\n make me suspect how good our test coverage is.\n\n This patch has a trivial textual conflict with Sebastian's patch,\n but the resolution should be obvious.\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 665bd27448..80fc7a3c70 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -304,7 +304,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tif (S_ISDIR(st.st_mode)\n \t\t && lstat(dst, &st) == 0) {\n-\t\t\tbad = _(\"cannot move directory over file\");\n+\t\t\tbad = _(\"destination already exists\");\n \t\t\tgoto act_on_entry;\n \t\t}\n \n-- \n2.42.0-rc1\n\n"}]}