{"thread":{"id":"57900","subject":"[PATCH] setup: don't die if realpath(3) fails on getcwd(3)","startedAt":"2022-05-19T23:47:29Z","lastAt":"2022-05-28T01:28:09Z","messageCount":22,"participants":["Kevin Locke","Junio C Hamano","Elijah Newren","Derrick Stolee","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"455543","messageId":"80eeba2b2a58af2a5497f398beb5c03447c41f61.1653003552.git.kevin@kevinlocke.name","threadId":"57900","inReplyTo":null,"subject":"[PATCH] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Kevin Locke","fromEmail":"kevin@kevinlocke.name","sentAt":"2022-05-19T23:39:13Z","receivedAt":"2022-05-19T23:47:29Z","isPatch":true,"sender":{"key":"kevin@kevinlocke.name","avatar":"https://avatars.githubusercontent.com/u/336433?v=4"},"body":"Prior to Git 2.35.0, git could be run from an inaccessible working\ndirectory so long as the git repository specified by options and/or\nenvironment variables was accessible. For example:\n\n git init repo\n mkdir -p a/b\n cd a/b\n chmod u-x ..\n git -C \"${PWD%/a/b}/repo\" status\n\nIf this example seems a bit contrived, consider running with the\nrepository owner as a substitute UID (e.g. with runuser(1) or sudo(8))\nwithout ensuring the working directory is accessible by that user.\n\nThe code added by e6f8861bd4 to preserve the working directory attempts\nto normalize the path using strbuf_realpath(). If that fails, as in the\ncase above, it is treated as a fatal error. To avoid this, we can\ncontinue after the error. At worst, git will fail to detect that the\nworking directory is inside the worktree, resulting in the pre-2.35.0\nbehavior of not preserving the working directory.\n\nFixes: e6f8861bd4 (\"setup: introduce startup_info->original_cwd\")\nSigned-off-by: Kevin Locke \n---\n setup.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex a7b36f3ffb..fb68caaae0 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -458,11 +458,13 @@ static void setup_original_cwd(void)\n \t * not startup_info->original_cwd.\n \t */\n \n-\t/* Normalize the directory */\n-\tstrbuf_realpath(&tmp, tmp_original_cwd, 1);\n-\tfree((char*)tmp_original_cwd);\n+\t/* Try to normalize the directory. Fails if ancestor not readable. */\n+\tif (strbuf_realpath(&tmp, tmp_original_cwd, 0)) {\n+\t\tfree((char*)tmp_original_cwd);\n+\t\tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n+\t} else\n+\t\tstartup_info->original_cwd = tmp_original_cwd;\n \ttmp_original_cwd = NULL;\n-\tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n \n \t/*\n \t * Get our worktree; we only protect the current working directory\n-- \n2.35.1\n\n"},{"id":"455589","messageId":"xmqqtu9kgibk.fsf@gitster.g","threadId":"57900","inReplyTo":"80eeba2b2a58af2a5497f398beb5c03447c41f61.1653003552.git.kevin@kevinlocke.name","subject":"Re: [PATCH] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-20T18:38:55Z","receivedAt":"2022-05-20T18:39:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Locke writes:\n\n> Prior to Git 2.35.0, git could be run from an inaccessible working\n> directory so long as the git repository specified by options and/or\n> environment variables was accessible. For example:\n>\n> git init repo\n> mkdir -p a/b\n> cd a/b\n> chmod u-x ..\n> git -C \"${PWD%/a/b}/repo\" status\n>\n> If this example seems a bit contrived, consider running with the\n> repository owner as a substitute UID (e.g. with runuser(1) or sudo(8))\n> without ensuring the working directory is accessible by that user.\n>\n> The code added by e6f8861bd4 to preserve the working directory attempts\n> to normalize the path using strbuf_realpath(). If that fails, as in the\n> case above, it is treated as a fatal error.\n\nThanks. As this thing is primarily for safety, I am inclined to say\nthat I'd prefer to see it error out when we cannot figure out the\nnecessary info to keep that safety promise to the users, than using\nan unnormalized value as a stand-in and letting the logic that is\ndesigned to be fed a normalized value do random (and possibly wrong)\nthings. But see below.\n\n> Fixes: e6f8861bd4 (\"setup: introduce startup_info->original_cwd\")\n\nAFAIK, we do not use this kind of trailer in this project. Casting\nin stone the claim that this \"fixes\" would be embarrassing when it\nturns out that it does not fix it (or even worse, breaks it).\n\n> Signed-off-by: Kevin Locke \n> ---\n> setup.c | 10 ++++++----\n> 1 file changed, 6 insertions(+), 4 deletions(-)\n>\n> diff --git a/setup.c b/setup.c\n> index a7b36f3ffb..fb68caaae0 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -458,11 +458,13 @@ static void setup_original_cwd(void)\n> \t * not startup_info->original_cwd.\n> \t */\n> \n> -\t/* Normalize the directory */\n> -\tstrbuf_realpath(&tmp, tmp_original_cwd, 1);\n> -\tfree((char*)tmp_original_cwd);\n> +\t/* Try to normalize the directory. Fails if ancestor not readable. */\n> +\tif (strbuf_realpath(&tmp, tmp_original_cwd, 0)) {\n> +\t\tfree((char*)tmp_original_cwd);\n> +\t\tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n> +\t} else\n> +\t\tstartup_info->original_cwd = tmp_original_cwd;\n\nI am OK to loosen the \"we try not to remove the original cwd\" logic\nso that it does not kick in when we cannot figure out the original\ncwd in the first place. But if that is the case, then I'd rather\nsee \"startrup_info->original_cwd set to NULL\" as the signal that we\nare in such a situation.\n\nElijah, what's your take on this change?\n\n> \ttmp_original_cwd = NULL;\n> -\tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n> \n> \t/*\n> \t * Get our worktree; we only protect the current working directory\n"},{"id":"455662","messageId":"CABPp-BH+=zd_ZpPMy=S5Q-ygTW85ZXD9-RLOk9Apt_Q1_SgzzA@mail.gmail.com","threadId":"57900","inReplyTo":"80eeba2b2a58af2a5497f398beb5c03447c41f61.1653003552.git.kevin@kevinlocke.name","subject":"Re: [PATCH] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-05-21T00:14:41Z","receivedAt":"2022-05-21T00:14:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nThanks for sending along a fix!\n\nOn Thu, May 19, 2022 at 4:39 PM Kevin Locke wrote:\n>\n> Prior to Git 2.35.0, git could be run from an inaccessible working\n> directory so long as the git repository specified by options and/or\n> environment variables was accessible. For example:\n\nAh, good point.\n\n> git init repo\n> mkdir -p a/b\n> cd a/b\n> chmod u-x ..\n> git -C \"${PWD%/a/b}/repo\" status\n>\n> If this example seems a bit contrived, consider running with the\n> repository owner as a substitute UID (e.g. with runuser(1) or sudo(8))\n> without ensuring the working directory is accessible by that user.\n>\n> The code added by e6f8861bd4 to preserve the working directory attempts\n\nWhen referencing commits in commit messages, this project prefers that the first\nreference in the commit message use the output from `git log --no-walk\n--pretty=reference $HASH` rather than just $HASH.\nSo, here, it'd be\n\n The code added by e6f8861bd4 (setup: introduce\nstartup_info->original_cwd, 2021-12-09) to preserve the ...\n\n> to normalize the path using strbuf_realpath(). If that fails, as in the\n> case above, it is treated as a fatal error. To avoid this, we can\n> continue after the error. At worst, git will fail to detect that the\n> working directory is inside the worktree, resulting in the pre-2.35.0\n> behavior of not preserving the working directory.\n>\n> Fixes: e6f8861bd4 (\"setup: introduce startup_info->original_cwd\")\n\nI was slightly surprised to see this tag, but it appears others in\ngit.git have used it, so it must just be me that's not familiar with\nit.\n\n> Signed-off-by: Kevin Locke \n\nNicely explained commit message.\n\n> ---\n> setup.c | 10 ++++++----\n> 1 file changed, 6 insertions(+), 4 deletions(-)\n>\n> diff --git a/setup.c b/setup.c\n> index a7b36f3ffb..fb68caaae0 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -458,11 +458,13 @@ static void setup_original_cwd(void)\n> * not startup_info->original_cwd.\n> */\n>\n> - /* Normalize the directory */\n> - strbuf_realpath(&tmp, tmp_original_cwd, 1);\n> - free((char*)tmp_original_cwd);\n> + /* Try to normalize the directory. Fails if ancestor not readable. */\n\nIs that the only reason it fails? I'm unsure if the second half of\nthe comment helps there.\n\n> + if (strbuf_realpath(&tmp, tmp_original_cwd, 0)) {\n> + free((char*)tmp_original_cwd);\n> + startup_info->original_cwd = strbuf_detach(&tmp, NULL);\n> + } else\n\ngit.git coding style: if either of the if/else blocks use braces, use\nbraces for both\n\n> + startup_info->original_cwd = tmp_original_cwd;\n\ntmp_original_cwd is not required to be normalized, and there are very\nstrong normalization assumptions on startup_info->original_cwd. While\na non-normalized value would work to get pre-2.35.0 behavior, it's by\naccident rather than design, and might be confusing for others to\nlater reason about. Also, I think it might be possible for\ntmp_original_cwd to still be NULL, and some of the immediately\nfollowing code I believe will assume it's operating with a non-NULL\nvalue, so you'd need to skip that stuff. I think the else block here\nshould use \"goto no_prevention_needed\", as the no_prevention_needed\nblock will handle setting startup_info->original_cwd to NULL for you,\nand get you the pre-2.35.0 behavior.\n\n> tmp_original_cwd = NULL;\n\nAfter changing the above else block to a goto, you may also want to\ncopy this to the no_prevention_needed block or else copy it to the\nelse portion of the block above (just before the goto you add).\n\n> - startup_info->original_cwd = strbuf_detach(&tmp, NULL);\n>\n> /*\n> * Get our worktree; we only protect the current working directory\n> --\n> 2.35.1\n\nAgain, thanks for sending in a bug fix! Looking forward to seeing\nyour revised patch.\n"},{"id":"455671","messageId":"Yoji02YhO+sE817q@kevinlocke.name","threadId":"57900","inReplyTo":"CABPp-BH+=zd_ZpPMy=S5Q-ygTW85ZXD9-RLOk9Apt_Q1_SgzzA@mail.gmail.com","subject":"Re: [PATCH] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Kevin Locke","fromEmail":"kevin@kevinlocke.name","sentAt":"2022-05-21T13:02:11Z","receivedAt":"2022-05-21T13:02:32Z","isPatch":true,"sender":{"key":"kevin@kevinlocke.name","avatar":"https://avatars.githubusercontent.com/u/336433?v=4"},"body":"Thanks Elijah and Junio,\n\nI appreciate your reviews. I agree with all of your suggestions. I'll\nsend a revised patch that incorporates the suggested changes shortly.\n\nCheers,\nKevin\n\n\nOn Fri, 2022-05-20 at 17:14 -0700, Elijah Newren wrote:\n> On Thu, May 19, 2022 at 4:39 PM Kevin Locke wrote:\n>> git init repo\n>> mkdir -p a/b\n>> cd a/b\n>> chmod u-x ..\n>> git -C \"${PWD%/a/b}/repo\" status\n>>\n>> If this example seems a bit contrived, consider running with the\n>> repository owner as a substitute UID (e.g. with runuser(1) or sudo(8))\n>> without ensuring the working directory is accessible by that user.\n>>\n>> The code added by e6f8861bd4 to preserve the working directory attempts\n> \n> When referencing commits in commit messages, this project prefers that the first\n> reference in the commit message use the output from `git log --no-walk\n> --pretty=reference $HASH` rather than just $HASH.\n> So, here, it'd be\n> \n> The code added by e6f8861bd4 (setup: introduce\n> startup_info->original_cwd, 2021-12-09) to preserve the ...\n> \n>> to normalize the path using strbuf_realpath(). If that fails, as in the\n>> case above, it is treated as a fatal error. To avoid this, we can\n>> continue after the error. At worst, git will fail to detect that the\n>> working directory is inside the worktree, resulting in the pre-2.35.0\n>> behavior of not preserving the working directory.\n>>\n>> Fixes: e6f8861bd4 (\"setup: introduce startup_info->original_cwd\")\n> \n> I was slightly surprised to see this tag, but it appears others in\n> git.git have used it, so it must just be me that's not familiar with\n> it.\n> \n>> Signed-off-by: Kevin Locke \n> \n> Nicely explained commit message.\n> \n>> ---\n>> setup.c | 10 ++++++----\n>> 1 file changed, 6 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/setup.c b/setup.c\n>> index a7b36f3ffb..fb68caaae0 100644\n>> --- a/setup.c\n>> +++ b/setup.c\n>> @@ -458,11 +458,13 @@ static void setup_original_cwd(void)\n>> * not startup_info->original_cwd.\n>> */\n>>\n>> - /* Normalize the directory */\n>> - strbuf_realpath(&tmp, tmp_original_cwd, 1);\n>> - free((char*)tmp_original_cwd);\n>> + /* Try to normalize the directory. Fails if ancestor not readable. */\n> \n> Is that the only reason it fails? I'm unsure if the second half of\n> the comment helps there.\n> \n>> + if (strbuf_realpath(&tmp, tmp_original_cwd, 0)) {\n>> + free((char*)tmp_original_cwd);\n>> + startup_info->original_cwd = strbuf_detach(&tmp, NULL);\n>> + } else\n> \n> git.git coding style: if either of the if/else blocks use braces, use\n> braces for both\n> \n>> + startup_info->original_cwd = tmp_original_cwd;\n> \n> tmp_original_cwd is not required to be normalized, and there are very\n> strong normalization assumptions on startup_info->original_cwd. While\n> a non-normalized value would work to get pre-2.35.0 behavior, it's by\n> accident rather than design, and might be confusing for others to\n> later reason about. Also, I think it might be possible for\n> tmp_original_cwd to still be NULL, and some of the immediately\n> following code I believe will assume it's operating with a non-NULL\n> value, so you'd need to skip that stuff. I think the else block here\n> should use \"goto no_prevention_needed\", as the no_prevention_needed\n> block will handle setting startup_info->original_cwd to NULL for you,\n> and get you the pre-2.35.0 behavior.\n> \n>> tmp_original_cwd = NULL;\n> \n> After changing the above else block to a goto, you may also want to\n> copy this to the no_prevention_needed block or else copy it to the\n> else portion of the block above (just before the goto you add).\n"},{"id":"455672","messageId":"7c064f43ed426c9e3b54e1ae5313d6b9332a47cb.1653141169.git.kevin@kevinlocke.name","threadId":"57900","inReplyTo":"80eeba2b2a58af2a5497f398beb5c03447c41f61.1653003552.git.kevin@kevinlocke.name","subject":"[PATCH v2] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Kevin Locke","fromEmail":"kevin@kevinlocke.name","sentAt":"2022-05-21T13:53:02Z","receivedAt":"2022-05-21T13:53:43Z","isPatch":true,"sender":{"key":"kevin@kevinlocke.name","avatar":"https://avatars.githubusercontent.com/u/336433?v=4"},"body":"Prior to Git 2.35.0, git could be run from an inaccessible working\ndirectory so long as the git repository specified by options and/or\nenvironment variables was accessible. For example:\n\n git init repo\n mkdir -p a/b\n cd a/b\n chmod u-x ..\n git -C \"${PWD%/a/b}/repo\" status\n\nIf this example seems a bit contrived, consider running with the\nrepository owner as a substitute UID (e.g. with runuser(1) or sudo(8))\nwithout ensuring the working directory is accessible by that user.\n\nThe code added by e6f8861bd4 (\"setup: introduce\nstartup_info->original_cwd\") to preserve the working directory attempts\nto normalize the path using strbuf_realpath(). If that fails, as in the\ncase above, it is treated as a fatal error.\n\nThis commit treats strbuf_realpath() errors as non-fatal. If an error\noccurs, setup_original_cwd() will continue without applying removal\nprevention for cwd, resulting in the pre-2.35.0 behavior. The risk\nshould be minimal, since git will not operate on a repository with\ninaccessible ancestors, this behavior is only known to occur when cwd is\na descendant of the repository, an ancestor of cwd is inaccessible, and\nno ancestors of the repository are inaccessible.\n\nSigned-off-by: Kevin Locke \n---\n\nChanges since v1:\n * Set startup_info->original_cwd = NULL when strbuf_realpath() fails,\n rather than setting it to the un-normalized path.\n * Add a trace message when realpath fails to aid debugging.\n * Remove potential realpath failure cause from comment before it.\n * Improve format for reference to e6f8861bd4 in commit message.\n * Clarify when the pre-2.35.0 behavior may occur as a result of this\n commit in the commit message.\n * Remove 'Fixes:' tag from commit message.\n\n setup.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex a7b36f3ffb..22430a663c 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -458,11 +458,16 @@ static void setup_original_cwd(void)\n \t * not startup_info->original_cwd.\n \t */\n \n-\t/* Normalize the directory */\n-\tstrbuf_realpath(&tmp, tmp_original_cwd, 1);\n-\tfree((char*)tmp_original_cwd);\n-\ttmp_original_cwd = NULL;\n-\tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n+\t/* Try to normalize the directory. */\n+\tif (strbuf_realpath(&tmp, tmp_original_cwd, 0)) {\n+\t\tfree((char*)tmp_original_cwd);\n+\t\ttmp_original_cwd = NULL;\n+\t\tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n+\t} else {\n+\t\ttrace_printf(\"realpath(cwd) failed: %s\\n\", strerror(errno));\n+\t\ttmp_original_cwd = NULL;\n+\t\tgoto no_prevention_needed;\n+\t}\n \n \t/*\n \t * Get our worktree; we only protect the current working directory\n-- \n2.35.1\n\n"},{"id":"455806","messageId":"acc3c765-7476-ae00-1546-f6cc07fb5c75@github.com","threadId":"57900","inReplyTo":"Yoji02YhO+sE817q@kevinlocke.name","subject":"Re: [PATCH] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-23T18:44:27Z","receivedAt":"2022-05-23T19:07:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/21/22 9:02 AM, Kevin Locke wrote:\n> Thanks Elijah and Junio,\n> \n> I appreciate your reviews. I agree with all of your suggestions. I'll\n> send a revised patch that incorporates the suggested changes shortly.\n> \n> Cheers,\n> Kevin\n> \n> \n> On Fri, 2022-05-20 at 17:14 -0700, Elijah Newren wrote:\n>> On Thu, May 19, 2022 at 4:39 PM Kevin Locke wrote:\n\nHi Kevin,\n\nWelcome to the Git mailing list!\n\nOne minor thing to keep in mind is that we try to avoid\ntop-posting.\n\nA few other things that were mentioned in this v1 would\nbe noticed by reading a few documents in the tree:\n\n * Documentation/CodingGuidelines\n * Documentation/MyFirstContribution.txt\n * Documentation/SubmittingPatches\n\nThere's A LOT in there, so we all forget some of the\nthings some of the time.\n\nThanks,\n-Stolee\n"},{"id":"455808","messageId":"1580ad10-43f6-bc73-901a-b65b1aea73ff@github.com","threadId":"57900","inReplyTo":"7c064f43ed426c9e3b54e1ae5313d6b9332a47cb.1653141169.git.kevin@kevinlocke.name","subject":"Re: [PATCH v2] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-23T18:57:56Z","receivedAt":"2022-05-23T19:21:25Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/21/22 9:53 AM, Kevin Locke wrote:\n> -\t/* Normalize the directory */\n> -\tstrbuf_realpath(&tmp, tmp_original_cwd, 1);\n> -\tfree((char*)tmp_original_cwd);\n> -\ttmp_original_cwd = NULL;\n> -\tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n> +\t/* Try to normalize the directory. */\n> +\tif (strbuf_realpath(&tmp, tmp_original_cwd, 0)) {\n\nI had to look it up, but this last argument going from 1 to 0\nis \"die_on_error\", so it makes sense why it wasn't checked by\nan 'if' earlier _and_ why it needs to change now.\n\n> +\t\tfree((char*)tmp_original_cwd);\n\nHm. I'm never a fan of this casting, but it existed before. It's\nbecause tmp_original_cwd is exposed globally in cache.h, which\nis _really widely_. However, there are only two uses: setup.c,\nwhich defines it, and common-main.c, which initializes it during\nprocess startup.\n\nThe following diff could apply before your commit, removing this\nuse of \"const char *\", but maybe it doesn't fit normal Git\ncoding guidelines (putting the extern directly in a *.c file):\n\n--- >8 ---\n\ndiff --git a/cache.h b/cache.h\nindex aaf334e2aa4..ce9cd6fa3f0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1797,7 +1797,6 @@ struct startup_info {\n \tconst char *original_cwd;\n };\n extern struct startup_info *startup_info;\n-extern const char *tmp_original_cwd;\n \n /* merge.c */\n struct commit_list;\ndiff --git a/common-main.c b/common-main.c\nindex 29fb7452f8a..e472258b83b 100644\n--- a/common-main.c\n+++ b/common-main.c\n@@ -23,6 +23,8 @@ static void restore_sigpipe_to_default(void)\n \tsignal(SIGPIPE, SIG_DFL);\n }\n \n+extern char *tmp_original_cwd;\n+\n int main(int argc, const char **argv)\n {\n \tint result;\ndiff --git a/setup.c b/setup.c\nindex 04ce33cdcd4..86986317490 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -12,7 +12,7 @@ static int work_tree_config_is_bogus;\n \n static struct startup_info the_startup_info;\n struct startup_info *startup_info = &the_startup_info;\n-const char *tmp_original_cwd;\n+char *tmp_original_cwd;\n \n /*\n * The input parameter must contain an absolute path, and it must already be\n@@ -459,7 +459,7 @@ static void setup_original_cwd(void)\n \n \t/* Normalize the directory */\n \tstrbuf_realpath(&tmp, tmp_original_cwd, 1);\n-\tfree((char*)tmp_original_cwd);\n+\tfree(tmp_original_cwd);\n \ttmp_original_cwd = NULL;\n \tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n \n--- >8 ---\n\n> +\t\ttmp_original_cwd = NULL;\n> +\t\tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n> +\t} else {\n> +\t\ttrace_printf(\"realpath(cwd) failed: %s\\n\", strerror(errno));\n\nIt could also be helpful to include a trace2 output, since\nmost modern tracing uses that mechanism:\n\n\t\ttrace2_data_string(\"setup\", the_repository,\n\t\t\t\t \"realpath-path\", tmp_original_cwd);\n\t\ttrace2_data_string(\"setup\", the_repository,\n\t\t\t\t \"realpath-failure\", strerror(errno));\n\nBut this is also unlikely to be necessary. We can instruct\na user to rerun their command with GIT_TRACE=1 if we see\nthis error in the wild.\n\n> +\t\ttmp_original_cwd = NULL;\n> +\t\tgoto no_prevention_needed;\n> +\t}\n\nSo, I don't see a need for this patch to change before it\nis merged. I'm curious to hear thoughts on the diff I put\ninline, though.\n\nThanks,\n-Stolee\n"},{"id":"455902","messageId":"YozlZ9DPrRLPBTBP@kevinlocke.name","threadId":"57900","inReplyTo":"1580ad10-43f6-bc73-901a-b65b1aea73ff@github.com","subject":"Re: [PATCH v2] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Kevin Locke","fromEmail":"kevin@kevinlocke.name","sentAt":"2022-05-24T14:02:15Z","receivedAt":"2022-05-24T14:02:33Z","isPatch":true,"sender":{"key":"kevin@kevinlocke.name","avatar":"https://avatars.githubusercontent.com/u/336433?v=4"},"body":"On Mon, 2022-05-23 at 14:57 -0400, Derrick Stolee wrote:\n> On 5/21/22 9:53 AM, Kevin Locke wrote:\n> > +\t\tfree((char*)tmp_original_cwd);\n> \n> Hm. I'm never a fan of this casting, but it existed before. It's\n> because tmp_original_cwd is exposed globally in cache.h, which\n> is _really widely_. However, there are only two uses: setup.c,\n> which defines it, and common-main.c, which initializes it during\n> process startup.\n> \n> The following diff could apply before your commit, removing this\n> use of \"const char *\", but maybe it doesn't fit normal Git\n> coding guidelines (putting the extern directly in a *.c file):\n> \n> --- >8 ---\n> \n> diff --git a/cache.h b/cache.h\n> index aaf334e2aa4..ce9cd6fa3f0 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1797,7 +1797,6 @@ struct startup_info {\n> \tconst char *original_cwd;\n> };\n> extern struct startup_info *startup_info;\n> -extern const char *tmp_original_cwd;\n> \n> /* merge.c */\n> struct commit_list;\n> diff --git a/common-main.c b/common-main.c\n> index 29fb7452f8a..e472258b83b 100644\n> --- a/common-main.c\n> +++ b/common-main.c\n> @@ -23,6 +23,8 @@ static void restore_sigpipe_to_default(void)\n> \tsignal(SIGPIPE, SIG_DFL);\n> }\n> \n> +extern char *tmp_original_cwd;\n> +\n> int main(int argc, const char **argv)\n> {\n> \tint result;\n> diff --git a/setup.c b/setup.c\n> index 04ce33cdcd4..86986317490 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -12,7 +12,7 @@ static int work_tree_config_is_bogus;\n> \n> static struct startup_info the_startup_info;\n> struct startup_info *startup_info = &the_startup_info;\n> -const char *tmp_original_cwd;\n> +char *tmp_original_cwd;\n> \n> /*\n> * The input parameter must contain an absolute path, and it must already be\n> @@ -459,7 +459,7 @@ static void setup_original_cwd(void)\n> \n> \t/* Normalize the directory */\n> \tstrbuf_realpath(&tmp, tmp_original_cwd, 1);\n> -\tfree((char*)tmp_original_cwd);\n> +\tfree(tmp_original_cwd);\n> \ttmp_original_cwd = NULL;\n> \tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n> \n> --- >8 ---\n\nThis approach seems reasonable to me, as does casting to free(). It's\nnot clear to me which is preferable in this case. How to balance the\ntrade-offs between exposing const interfaces, limiting (internal)\ninterfaces to headers, and avoiding casts might be worth discussing\nand documenting a matter of project coding style. `grep -rF 'free(('`\nlists about 100 casts to free, suggesting the discussion may be\nworthwhile. Introducing a free_const() macro could be another option\nto consider.\n\n>> +\t\ttmp_original_cwd = NULL;\n>> +\t\tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n>> +\t} else {\n>> +\t\ttrace_printf(\"realpath(cwd) failed: %s\\n\", strerror(errno));\n> \n> It could also be helpful to include a trace2 output, since\n> most modern tracing uses that mechanism:\n> \n> \t\ttrace2_data_string(\"setup\", the_repository,\n> \t\t\t\t \"realpath-path\", tmp_original_cwd);\n> \t\ttrace2_data_string(\"setup\", the_repository,\n> \t\t\t\t \"realpath-failure\", strerror(errno));\n> \n> But this is also unlikely to be necessary. We can instruct\n> a user to rerun their command with GIT_TRACE=1 if we see\n> this error in the wild.\n\nThat's a great idea. I hadn't realized the difference between the\ntrace_* and trace2_* APIs until investigating as a result of your\nsuggestion. Trace2 seems preferable for many reasons. I'll send an\nupdated patch shortly.\n\nThanks for the review and feedback Stolee!\n\nCheers,\nKevin\n"},{"id":"455907","messageId":"68c66aef7ca4dba53faec9e6d2f3b70fe58ac33e.1653403877.git.kevin@kevinlocke.name","threadId":"57900","inReplyTo":"7c064f43ed426c9e3b54e1ae5313d6b9332a47cb.1653141169.git.kevin@kevinlocke.name","subject":"[PATCH v3] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Kevin Locke","fromEmail":"kevin@kevinlocke.name","sentAt":"2022-05-24T14:51:31Z","receivedAt":"2022-05-24T14:51:49Z","isPatch":true,"sender":{"key":"kevin@kevinlocke.name","avatar":"https://avatars.githubusercontent.com/u/336433?v=4"},"body":"Prior to Git 2.35.0, git could be run from an inaccessible working\ndirectory so long as the git repository specified by options and/or\nenvironment variables was accessible. For example:\n\n git init repo\n mkdir -p a/b\n cd a/b\n chmod u-x ..\n git -C \"${PWD%/a/b}/repo\" status\n\nIf this example seems a bit contrived, consider running with the\nrepository owner as a substitute UID (e.g. with runuser(1) or sudo(8))\nwithout ensuring the working directory is accessible by that user.\n\nThe code added by e6f8861bd4 (\"setup: introduce\nstartup_info->original_cwd\") to preserve the working directory attempts\nto normalize the path using strbuf_realpath(). If that fails, as in the\ncase above, it is treated as a fatal error.\n\nThis commit treats strbuf_realpath() errors as non-fatal. If an error\noccurs, setup_original_cwd() will continue without applying removal\nprevention for cwd, resulting in the pre-2.35.0 behavior. The risk\nshould be minimal, since git will not operate on a repository with\ninaccessible ancestors, this behavior is only known to occur when cwd is\na descendant of the repository, an ancestor of cwd is inaccessible, and\nno ancestors of the repository are inaccessible.\n\nSigned-off-by: Kevin Locke \n---\n\nNotes:\nChanges since v2:\n * Use trace2_data_string(), rather than trace_printf(), to report\n realpath failure.\n\nChanges since v1:\n * Set startup_info->original_cwd = NULL when strbuf_realpath() fails,\n rather than setting it to the un-normalized path.\n * Add a trace message when realpath fails to aid debugging.\n * Remove potential realpath failure cause from comment before it.\n * Improve format for reference to e6f8861bd4 in commit message.\n * Clarify when the pre-2.35.0 behavior may occur as a result of this\n commit in the commit message.\n * Remove 'Fixes:' tag from commit message.\n\n setup.c | 18 +++++++++++++-----\n 1 file changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex a7b36f3ffbf..38bd55cbac1 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -458,11 +458,19 @@ static void setup_original_cwd(void)\n \t * not startup_info->original_cwd.\n \t */\n \n-\t/* Normalize the directory */\n-\tstrbuf_realpath(&tmp, tmp_original_cwd, 1);\n-\tfree((char*)tmp_original_cwd);\n-\ttmp_original_cwd = NULL;\n-\tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n+\t/* Try to normalize the directory. */\n+\tif (strbuf_realpath(&tmp, tmp_original_cwd, 0)) {\n+\t\tfree((char*)tmp_original_cwd);\n+\t\ttmp_original_cwd = NULL;\n+\t\tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n+\t} else {\n+\t\ttrace2_data_string(\"setup\", the_repository,\n+\t\t\t\t \"realpath-path\", tmp_original_cwd);\n+\t\ttrace2_data_string(\"setup\", the_repository,\n+\t\t\t\t \"realpath-failure\", strerror(errno));\n+\t\ttmp_original_cwd = NULL;\n+\t\tgoto no_prevention_needed;\n+\t}\n \n \t/*\n \t * Get our worktree; we only protect the current working directory\n-- \n2.35.1\n\n"},{"id":"455910","messageId":"CABPp-BGZTDKorz+CFScfTfx47c+TuJaAD_Zyyo1Jj_tymYkVXQ@mail.gmail.com","threadId":"57900","inReplyTo":"YozlZ9DPrRLPBTBP@kevinlocke.name","subject":"Re: [PATCH v2] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-05-24T15:20:08Z","receivedAt":"2022-05-24T15:20:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, May 24, 2022 at 7:02 AM Kevin Locke wrote:\n>\n> On Mon, 2022-05-23 at 14:57 -0400, Derrick Stolee wrote:\n> > On 5/21/22 9:53 AM, Kevin Locke wrote:\n> > > + free((char*)tmp_original_cwd);\n> >\n> > Hm. I'm never a fan of this casting, but it existed before. It's\n> > because tmp_original_cwd is exposed globally in cache.h, which\n> > is _really widely_. However, there are only two uses: setup.c,\n> > which defines it, and common-main.c, which initializes it during\n> > process startup.\n> >\n> > The following diff could apply before your commit, removing this\n> > use of \"const char *\", but maybe it doesn't fit normal Git\n> > coding guidelines (putting the extern directly in a *.c file):\n> >\n> > --- >8 ---\n> >\n> > diff --git a/cache.h b/cache.h\n> > index aaf334e2aa4..ce9cd6fa3f0 100644\n> > --- a/cache.h\n> > +++ b/cache.h\n> > @@ -1797,7 +1797,6 @@ struct startup_info {\n> > const char *original_cwd;\n> > };\n> > extern struct startup_info *startup_info;\n> > -extern const char *tmp_original_cwd;\n> >\n> > /* merge.c */\n> > struct commit_list;\n> > diff --git a/common-main.c b/common-main.c\n> > index 29fb7452f8a..e472258b83b 100644\n> > --- a/common-main.c\n> > +++ b/common-main.c\n> > @@ -23,6 +23,8 @@ static void restore_sigpipe_to_default(void)\n> > signal(SIGPIPE, SIG_DFL);\n> > }\n> >\n> > +extern char *tmp_original_cwd;\n> > +\n> > int main(int argc, const char **argv)\n> > {\n> > int result;\n> > diff --git a/setup.c b/setup.c\n> > index 04ce33cdcd4..86986317490 100644\n> > --- a/setup.c\n> > +++ b/setup.c\n> > @@ -12,7 +12,7 @@ static int work_tree_config_is_bogus;\n> >\n> > static struct startup_info the_startup_info;\n> > struct startup_info *startup_info = &the_startup_info;\n> > -const char *tmp_original_cwd;\n> > +char *tmp_original_cwd;\n> >\n> > /*\n> > * The input parameter must contain an absolute path, and it must already be\n> > @@ -459,7 +459,7 @@ static void setup_original_cwd(void)\n> >\n> > /* Normalize the directory */\n> > strbuf_realpath(&tmp, tmp_original_cwd, 1);\n> > - free((char*)tmp_original_cwd);\n> > + free(tmp_original_cwd);\n> > tmp_original_cwd = NULL;\n> > startup_info->original_cwd = strbuf_detach(&tmp, NULL);\n> >\n> > --- >8 ---\n>\n> This approach seems reasonable to me, as does casting to free(). It's\n> not clear to me which is preferable in this case. How to balance the\n> trade-offs between exposing const interfaces, limiting (internal)\n> interfaces to headers, and avoiding casts might be worth discussing\n> and documenting a matter of project coding style. `grep -rF 'free(('`\n> lists about 100 casts to free, suggesting the discussion may be\n> worthwhile. Introducing a free_const() macro could be another option\n> to consider.\n\nI'd prefer either a free_const() as you suggest (though as a separate\npatch from what you are submitting here), or leaving the code as-is.\nfree() could have been written to take a const void* instead of just\nvoid*, since it's not going to modify what the pointer points at. The\nreason we call free() is because the variable isn't needed anymore,\nand using a non-const value after freeing is just as wrong as using a\nconst one after freeing, so casting away the constness cannot really\ncause any new problems. So, I think the signature of free() is just\nwrong: it should have taken a const void* all along. Unfortunately,\nthe wrong type signature sadly makes people feel like they have to\nchoose between (a) dropping the added safety of const that the\ncompiler can enforce for you during the lifetime of the variable, or\n(b) leaking memory you no longer need. I think it's a bad choice and\nyou should just typecast when free'ing, but clearly others just don't\nwant to see any typecasts and are willing to dispense with const on\nconstant variables.\n"},{"id":"455911","messageId":"CABPp-BHHMUJdKbj31B-5Ov0xQLeyAqJOQiHxT_Z_9zXbYEcZRA@mail.gmail.com","threadId":"57900","inReplyTo":"68c66aef7ca4dba53faec9e6d2f3b70fe58ac33e.1653403877.git.kevin@kevinlocke.name","subject":"Re: [PATCH v3] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-05-24T15:21:01Z","receivedAt":"2022-05-24T15:21:19Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, May 24, 2022 at 7:51 AM Kevin Locke wrote:\n>\n> Prior to Git 2.35.0, git could be run from an inaccessible working\n> directory so long as the git repository specified by options and/or\n> environment variables was accessible. For example:\n>\n> git init repo\n> mkdir -p a/b\n> cd a/b\n> chmod u-x ..\n> git -C \"${PWD%/a/b}/repo\" status\n>\n> If this example seems a bit contrived, consider running with the\n> repository owner as a substitute UID (e.g. with runuser(1) or sudo(8))\n> without ensuring the working directory is accessible by that user.\n>\n> The code added by e6f8861bd4 (\"setup: introduce\n> startup_info->original_cwd\") to preserve the working directory attempts\n> to normalize the path using strbuf_realpath(). If that fails, as in the\n> case above, it is treated as a fatal error.\n>\n> This commit treats strbuf_realpath() errors as non-fatal. If an error\n> occurs, setup_original_cwd() will continue without applying removal\n> prevention for cwd, resulting in the pre-2.35.0 behavior. The risk\n> should be minimal, since git will not operate on a repository with\n> inaccessible ancestors, this behavior is only known to occur when cwd is\n> a descendant of the repository, an ancestor of cwd is inaccessible, and\n> no ancestors of the repository are inaccessible.\n>\n> Signed-off-by: Kevin Locke \n> ---\n>\n> Notes:\n> Changes since v2:\n> * Use trace2_data_string(), rather than trace_printf(), to report\n> realpath failure.\n>\n> Changes since v1:\n> * Set startup_info->original_cwd = NULL when strbuf_realpath() fails,\n> rather than setting it to the un-normalized path.\n> * Add a trace message when realpath fails to aid debugging.\n> * Remove potential realpath failure cause from comment before it.\n> * Improve format for reference to e6f8861bd4 in commit message.\n> * Clarify when the pre-2.35.0 behavior may occur as a result of this\n> commit in the commit message.\n> * Remove 'Fixes:' tag from commit message.\n>\n> setup.c | 18 +++++++++++++-----\n> 1 file changed, 13 insertions(+), 5 deletions(-)\n>\n> diff --git a/setup.c b/setup.c\n> index a7b36f3ffbf..38bd55cbac1 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -458,11 +458,19 @@ static void setup_original_cwd(void)\n> * not startup_info->original_cwd.\n> */\n>\n> - /* Normalize the directory */\n> - strbuf_realpath(&tmp, tmp_original_cwd, 1);\n> - free((char*)tmp_original_cwd);\n> - tmp_original_cwd = NULL;\n> - startup_info->original_cwd = strbuf_detach(&tmp, NULL);\n> + /* Try to normalize the directory. */\n> + if (strbuf_realpath(&tmp, tmp_original_cwd, 0)) {\n> + free((char*)tmp_original_cwd);\n> + tmp_original_cwd = NULL;\n> + startup_info->original_cwd = strbuf_detach(&tmp, NULL);\n> + } else {\n> + trace2_data_string(\"setup\", the_repository,\n> + \"realpath-path\", tmp_original_cwd);\n> + trace2_data_string(\"setup\", the_repository,\n> + \"realpath-failure\", strerror(errno));\n> + tmp_original_cwd = NULL;\n> + goto no_prevention_needed;\n> + }\n>\n> /*\n> * Get our worktree; we only protect the current working directory\n> --\n> 2.35.1\n\nLooks good to me:\n\nReviewed-by: Elijah Newren \n"},{"id":"455916","messageId":"563606bd-551f-39b2-74f0-40547b7a0113@github.com","threadId":"57900","inReplyTo":"CABPp-BGZTDKorz+CFScfTfx47c+TuJaAD_Zyyo1Jj_tymYkVXQ@mail.gmail.com","subject":"Re: [PATCH v2] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-24T17:38:30Z","receivedAt":"2022-05-24T17:38:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/24/2022 11:20 AM, Elijah Newren wrote:\n> On Tue, May 24, 2022 at 7:02 AM Kevin Locke wrote:\n>>\n>> On Mon, 2022-05-23 at 14:57 -0400, Derrick Stolee wrote:\n>>> On 5/21/22 9:53 AM, Kevin Locke wrote:\n>>>> + free((char*)tmp_original_cwd);\n>>>\n>>> Hm. I'm never a fan of this casting, but it existed before. It's\n>>> because tmp_original_cwd is exposed globally in cache.h, which\n>>> is _really widely_. However, there are only two uses: setup.c,\n>>> which defines it, and common-main.c, which initializes it during\n>>> process startup.\n...>> This approach seems reasonable to me, as does casting to free(). It's\n>> not clear to me which is preferable in this case. How to balance the\n>> trade-offs between exposing const interfaces, limiting (internal)\n>> interfaces to headers, and avoiding casts might be worth discussing\n>> and documenting a matter of project coding style. `grep -rF 'free(('`\n>> lists about 100 casts to free, suggesting the discussion may be\n>> worthwhile. Introducing a free_const() macro could be another option\n>> to consider.\n> \n> I'd prefer either a free_const() as you suggest (though as a separate\n> patch from what you are submitting here), or leaving the code as-is.\n> free() could have been written to take a const void* instead of just\n> void*, since it's not going to modify what the pointer points at. The\n> reason we call free() is because the variable isn't needed anymore,\n> and using a non-const value after freeing is just as wrong as using a\n> const one after freeing, so casting away the constness cannot really\n> cause any new problems. So, I think the signature of free() is just\n> wrong: it should have taken a const void* all along. Unfortunately,\n> the wrong type signature sadly makes people feel like they have to\n> choose between (a) dropping the added safety of const that the\n> compiler can enforce for you during the lifetime of the variable, or\n> (b) leaking memory you no longer need. I think it's a bad choice and\n> you should just typecast when free'ing, but clearly others just don't\n> want to see any typecasts and are willing to dispense with const on\n> constant variables.\n\nI mostly agree with you: if free() didn't have the const, then the\nanswer would be simple. We probably wouldn't also have the convention\nof \"const pointers are for memory we don't own\".\n\nSpecifically with 'const char *' this can sometimes point to a\ncompiled string literal, so I tend to be more careful than usual\naround these kinds of casts.\n\nI'm willing to concede this point as it is much messier than just\nthe goals of this patch.\n\nThanks,\n-Stolee\n"},{"id":"455917","messageId":"dafc8178-d213-ffd0-9e2a-32ac88307b39@github.com","threadId":"57900","inReplyTo":"68c66aef7ca4dba53faec9e6d2f3b70fe58ac33e.1653403877.git.kevin@kevinlocke.name","subject":"Re: [PATCH v3] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-24T17:41:12Z","receivedAt":"2022-05-24T17:41:30Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/24/2022 10:51 AM, Kevin Locke wrote:> -\t/* Normalize the directory */\n> -\tstrbuf_realpath(&tmp, tmp_original_cwd, 1);\n> -\tfree((char*)tmp_original_cwd);\n> -\ttmp_original_cwd = NULL;\n> -\tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n> +\t/* Try to normalize the directory. */\n> +\tif (strbuf_realpath(&tmp, tmp_original_cwd, 0)) {\n> +\t\tfree((char*)tmp_original_cwd);\n> +\t\ttmp_original_cwd = NULL;\n> +\t\tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n> +\t} else {\n> +\t\ttrace2_data_string(\"setup\", the_repository,\n> +\t\t\t\t \"realpath-path\", tmp_original_cwd);\n> +\t\ttrace2_data_string(\"setup\", the_repository,\n> +\t\t\t\t \"realpath-failure\", strerror(errno));\n> +\t\ttmp_original_cwd = NULL;\n\nI didn't catch this in v2, but should this line instead be\n\n\t\tstartup_info->original_cwd = NULL;\n\n? Especially based on this comment:\n\n> Changes since v1:\n> * Set startup_info->original_cwd = NULL when strbuf_realpath() fails,\n> rather than setting it to the un-normalized path.\n\nThanks,\n-Stolee\n"},{"id":"455920","messageId":"Yo0dS550l2NCmx9O@kevinlocke.name","threadId":"57900","inReplyTo":"dafc8178-d213-ffd0-9e2a-32ac88307b39@github.com","subject":"Re: [PATCH v3] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Kevin Locke","fromEmail":"kevin@kevinlocke.name","sentAt":"2022-05-24T18:00:43Z","receivedAt":"2022-05-24T18:00:49Z","isPatch":true,"sender":{"key":"kevin@kevinlocke.name","avatar":"https://avatars.githubusercontent.com/u/336433?v=4"},"body":"On Tue, 2022-05-24 at 13:41 -0400, Derrick Stolee wrote:\n> On 5/24/2022 10:51 AM, Kevin Locke wrote:> -\t/* Normalize the directory */\n>> -\tstrbuf_realpath(&tmp, tmp_original_cwd, 1);\n>> -\tfree((char*)tmp_original_cwd);\n>> -\ttmp_original_cwd = NULL;\n>> -\tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n>> +\t/* Try to normalize the directory. */\n>> +\tif (strbuf_realpath(&tmp, tmp_original_cwd, 0)) {\n>> +\t\tfree((char*)tmp_original_cwd);\n>> +\t\ttmp_original_cwd = NULL;\n>> +\t\tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n>> +\t} else {\n>> +\t\ttrace2_data_string(\"setup\", the_repository,\n>> +\t\t\t\t \"realpath-path\", tmp_original_cwd);\n>> +\t\ttrace2_data_string(\"setup\", the_repository,\n>> +\t\t\t\t \"realpath-failure\", strerror(errno));\n>> +\t\ttmp_original_cwd = NULL;\n> \n> I didn't catch this in v2, but should this line instead be\n> \n> \t\tstartup_info->original_cwd = NULL;\n> \n> ? Especially based on this comment:\n\nNo worries. It's a good question. Setting startup_info->original_cwd\nto NULL is handled by the no_prevention_needed label. But I just\nrealized it's not actually required in this case, since original_cwd\nis NULL when setup_original_cwd() is called. I should probably return\nrather than jumping to no_prevention_needed from the else block to\navoid the unnecessary free(NULL) and assignment.\n\nYour comment made me realize that v2 and later neglect to free\ntmp_original_cwd when strbuf_realpath() fails. How embarrassing.\n\nI'll send an update to fix both those issues shortly.\n\nThanks again,\nKevin\n"},{"id":"455933","messageId":"8b20840014d214023c50ee62439147f798e6f9cc.1653419993.git.kevin@kevinlocke.name","threadId":"57900","inReplyTo":"68c66aef7ca4dba53faec9e6d2f3b70fe58ac33e.1653403877.git.kevin@kevinlocke.name","subject":"[PATCH v4] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Kevin Locke","fromEmail":"kevin@kevinlocke.name","sentAt":"2022-05-24T19:20:12Z","receivedAt":"2022-05-24T19:20:37Z","isPatch":true,"sender":{"key":"kevin@kevinlocke.name","avatar":"https://avatars.githubusercontent.com/u/336433?v=4"},"body":"Prior to Git 2.35.0, git could be run from an inaccessible working\ndirectory so long as the git repository specified by options and/or\nenvironment variables was accessible. For example:\n\n git init repo\n mkdir -p a/b\n cd a/b\n chmod u-x ..\n git -C \"${PWD%/a/b}/repo\" status\n\nIf this example seems a bit contrived, consider running with the\nrepository owner as a substitute UID (e.g. with runuser(1) or sudo(8))\nwithout ensuring the working directory is accessible by that user.\n\nThe code added by e6f8861bd4 (\"setup: introduce\nstartup_info->original_cwd\") to preserve the working directory attempts\nto normalize the path using strbuf_realpath(). If that fails, as in the\ncase above, it is treated as a fatal error.\n\nThis commit treats strbuf_realpath() errors as non-fatal. If an error\noccurs, setup_original_cwd() will continue without applying removal\nprevention for cwd, resulting in the pre-2.35.0 behavior. The risk\nshould be minimal, since git will not operate on a repository with\ninaccessible ancestors, this behavior is only known to occur when cwd is\na descendant of the repository, an ancestor of cwd is inaccessible, and\nno ancestors of the repository are inaccessible.\n\nSigned-off-by: Kevin Locke \n---\n\nChanges since v3:\n * Free tmp_original_cwd in both codepaths.\n * Return after strbuf_realpath() fails, rather than jumping to\n no_prevention_needed, to avoid unnecessary free(NULL) and NULL\n reassignment.\n * Invert the condition and remove the else block to match the\n return-on-error code style for better readability.\n * Stop adding \"Try\" to comment, since strbuf_realpath() hasn't\n been optional since v1.\n\nChanges since v2:\n * Use trace2_data_string(), rather than trace_printf(), to report\n realpath failure.\n\nChanges since v1:\n * Set startup_info->original_cwd = NULL when strbuf_realpath() fails,\n rather than setting it to the un-normalized path.\n * Add a trace message when realpath fails to aid debugging.\n * Remove potential realpath failure cause from comment before it.\n * Improve format for reference to e6f8861bd4 in commit message.\n * Clarify when the pre-2.35.0 behavior may occur as a result of this\n commit in the commit message.\n * Remove 'Fixes:' tag from commit message.\n\n setup.c | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex a7b36f3ffbf..e0a99df512f 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -459,7 +459,16 @@ static void setup_original_cwd(void)\n \t */\n \n \t/* Normalize the directory */\n-\tstrbuf_realpath(&tmp, tmp_original_cwd, 1);\n+\tif (!strbuf_realpath(&tmp, tmp_original_cwd, 0)) {\n+\t\ttrace2_data_string(\"setup\", the_repository,\n+\t\t\t\t \"realpath-path\", tmp_original_cwd);\n+\t\ttrace2_data_string(\"setup\", the_repository,\n+\t\t\t\t \"realpath-failure\", strerror(errno));\n+\t\tfree((char*)tmp_original_cwd);\n+\t\ttmp_original_cwd = NULL;\n+\t\treturn;\n+\t}\n+\n \tfree((char*)tmp_original_cwd);\n \ttmp_original_cwd = NULL;\n \tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n-- \n2.35.1\n\n"},{"id":"455951","messageId":"e311a560-0606-e5f7-0c41-a6b0f42062cf@github.com","threadId":"57900","inReplyTo":"8b20840014d214023c50ee62439147f798e6f9cc.1653419993.git.kevin@kevinlocke.name","subject":"Re: [PATCH v4] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-24T20:40:17Z","receivedAt":"2022-05-24T20:40:23Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/24/2022 3:20 PM, Kevin Locke wrote:\n> Changes since v3:\n> * Free tmp_original_cwd in both codepaths.\n> * Return after strbuf_realpath() fails, rather than jumping to\n> no_prevention_needed, to avoid unnecessary free(NULL) and NULL\n> reassignment.\n> * Invert the condition and remove the else block to match the\n> return-on-error code style for better readability.\n> * Stop adding \"Try\" to comment, since strbuf_realpath() hasn't\n> been optional since v1.\n\n...\n\n> \t/* Normalize the directory */\n> -\tstrbuf_realpath(&tmp, tmp_original_cwd, 1);\n> +\tif (!strbuf_realpath(&tmp, tmp_original_cwd, 0)) {\n> +\t\ttrace2_data_string(\"setup\", the_repository,\n> +\t\t\t\t \"realpath-path\", tmp_original_cwd);\n> +\t\ttrace2_data_string(\"setup\", the_repository,\n> +\t\t\t\t \"realpath-failure\", strerror(errno));\n> +\t\tfree((char*)tmp_original_cwd);\n> +\t\ttmp_original_cwd = NULL;\n> +\t\treturn;\n> +\t}\n\nThis is much easier to reason about.\n\n> \tfree((char*)tmp_original_cwd);\n> \ttmp_original_cwd = NULL;\n> \tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\nI had considered trying to remove this duplicate code freeing\ntemp_original_cwd. It requires adding a variable storing the\nreturn from strbuf_realpath() _or_ knowing that tmp will have\nzero length if strbuf_realpath() fails. It would look gross,\nthough:\n\n\tstrbuf_realpath(&tmp, tmp_original_cwd, 0);\n\n\tif (!tmp->len) {\n\t\ttrace2_data_string(\"setup\", the_repository,\n\t\t\t\t \"realpath-path\", tmp_original_cwd);\n\t\ttrace2_data_string(\"setup\", the_repository,\n\t\t\t\t \"realpath-failure\", strerror(errno));\n\t}\n\tfree((char*)tmp_original_cwd);\n\ttmp_original_cwd = NULL;\n\tif (!tmp->len)\n\t\treturn;\n\n\tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n\n...and that doesn't look very good at all. Thus, I think your v4\nis ready to merge. Thanks for working on it!\n\nThanks,\n-Stolee\n"},{"id":"455961","messageId":"xmqqk0aafwsd.fsf@gitster.g","threadId":"57900","inReplyTo":"8b20840014d214023c50ee62439147f798e6f9cc.1653419993.git.kevin@kevinlocke.name","subject":"Re: [PATCH v4] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-24T21:25:22Z","receivedAt":"2022-05-24T21:25:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Locke writes:\n\n> \t/* Normalize the directory */\n> -\tstrbuf_realpath(&tmp, tmp_original_cwd, 1);\n> +\tif (!strbuf_realpath(&tmp, tmp_original_cwd, 0)) {\n> +\t\ttrace2_data_string(\"setup\", the_repository,\n> +\t\t\t\t \"realpath-path\", tmp_original_cwd);\n> +\t\ttrace2_data_string(\"setup\", the_repository,\n> +\t\t\t\t \"realpath-failure\", strerror(errno));\n> +\t\tfree((char*)tmp_original_cwd);\n> +\t\ttmp_original_cwd = NULL;\n> +\t\treturn;\n> +\t}\n> +\n> \tfree((char*)tmp_original_cwd);\n> \ttmp_original_cwd = NULL;\n> \tstartup_info->original_cwd = strbuf_detach(&tmp, NULL);\n\nIt is somewhat sad that we cannot readily use FREE_AND_NULL() here.\nIf it casted away the constness (see the attached at the end), we\ncould have saved two lines from the above snippet.\n\nThe startup_info->original_cwd member is initialized to NULL, and\nI think it is a safe assumption that it still is so when the control\nreaches here. Otherwise, the assignment of strbuf_detach() to it\nwithout first freeing the current value we see in the post context\nis leaking already.\n\nSo, overall this looks good to me. Anybody else who have already\nspent cycles to review this want to add Reviewed-by: to it?\n\nThanks.\n\ndiff --git i/git-compat-util.h w/git-compat-util.h\nindex 58fd813bd0..56c6c48461 100644\n--- i/git-compat-util.h\n+++ w/git-compat-util.h\n@@ -976,7 +976,7 @@ int xstrncmpz(const char *s, const char *t, size_t len);\n * FREE_AND_NULL(ptr) is like free(ptr) followed by ptr = NULL. Note\n * that ptr is used twice, so don't pass e.g. ptr++.\n */\n-#define FREE_AND_NULL(p) do { free(p); (p) = NULL; } while (0)\n+#define FREE_AND_NULL(p) do { free((void*)p); (p) = NULL; } while (0)\n \n #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))\n #define CALLOC_ARRAY(x, alloc) (x) = xcalloc((alloc), sizeof(*(x)))\n"},{"id":"455995","messageId":"CABPp-BHCC2WkSTpnucZRSriYbhXyGQCGKKKuOFbLU-Odf_2UtA@mail.gmail.com","threadId":"57900","inReplyTo":"563606bd-551f-39b2-74f0-40547b7a0113@github.com","subject":"Re: [PATCH v2] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-05-25T03:47:47Z","receivedAt":"2022-05-25T03:48:09Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, May 24, 2022 at 10:38 AM Derrick Stolee\n wrote:\n>\n> On 5/24/2022 11:20 AM, Elijah Newren wrote:\n> > On Tue, May 24, 2022 at 7:02 AM Kevin Locke wrote:\n> >>\n> >> On Mon, 2022-05-23 at 14:57 -0400, Derrick Stolee wrote:\n> >>> On 5/21/22 9:53 AM, Kevin Locke wrote:\n> >>>> + free((char*)tmp_original_cwd);\n> >>>\n> >>> Hm. I'm never a fan of this casting, but it existed before. It's\n> >>> because tmp_original_cwd is exposed globally in cache.h, which\n> >>> is _really widely_. However, there are only two uses: setup.c,\n> >>> which defines it, and common-main.c, which initializes it during\n> >>> process startup.\n> ...>> This approach seems reasonable to me, as does casting to free(). It's\n> >> not clear to me which is preferable in this case. How to balance the\n> >> trade-offs between exposing const interfaces, limiting (internal)\n> >> interfaces to headers, and avoiding casts might be worth discussing\n> >> and documenting a matter of project coding style. `grep -rF 'free(('`\n> >> lists about 100 casts to free, suggesting the discussion may be\n> >> worthwhile. Introducing a free_const() macro could be another option\n> >> to consider.\n> >\n> > I'd prefer either a free_const() as you suggest (though as a separate\n> > patch from what you are submitting here), or leaving the code as-is.\n> > free() could have been written to take a const void* instead of just\n> > void*, since it's not going to modify what the pointer points at. The\n> > reason we call free() is because the variable isn't needed anymore,\n> > and using a non-const value after freeing is just as wrong as using a\n> > const one after freeing, so casting away the constness cannot really\n> > cause any new problems. So, I think the signature of free() is just\n> > wrong: it should have taken a const void* all along. Unfortunately,\n> > the wrong type signature sadly makes people feel like they have to\n> > choose between (a) dropping the added safety of const that the\n> > compiler can enforce for you during the lifetime of the variable, or\n> > (b) leaking memory you no longer need. I think it's a bad choice and\n> > you should just typecast when free'ing, but clearly others just don't\n> > want to see any typecasts and are willing to dispense with const on\n> > constant variables.\n>\n> I mostly agree with you: if free() didn't have the const, then the\n> answer would be simple. We probably wouldn't also have the convention\n> of \"const pointers are for memory we don't own\".\n>\n> Specifically with 'const char *' this can sometimes point to a\n> compiled string literal, so I tend to be more careful than usual\n> around these kinds of casts.\n\nAh, fair enough.\n\n\n> I'm willing to concede this point as it is much messier than just\n> the goals of this patch.\n\n:-)\n"},{"id":"455996","messageId":"CABPp-BHZQXt-=XtTk7Ez463rhbcQQBNGUVkbbWiavZT+Rsjn_A@mail.gmail.com","threadId":"57900","inReplyTo":"xmqqk0aafwsd.fsf@gitster.g","subject":"Re: [PATCH v4] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-05-25T03:51:50Z","receivedAt":"2022-05-25T03:52:07Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, May 24, 2022 at 2:25 PM Junio C Hamano wrote:\n>\n> Kevin Locke writes:\n>\n> > /* Normalize the directory */\n> > - strbuf_realpath(&tmp, tmp_original_cwd, 1);\n> > + if (!strbuf_realpath(&tmp, tmp_original_cwd, 0)) {\n> > + trace2_data_string(\"setup\", the_repository,\n> > + \"realpath-path\", tmp_original_cwd);\n> > + trace2_data_string(\"setup\", the_repository,\n> > + \"realpath-failure\", strerror(errno));\n> > + free((char*)tmp_original_cwd);\n> > + tmp_original_cwd = NULL;\n> > + return;\n> > + }\n> > +\n> > free((char*)tmp_original_cwd);\n> > tmp_original_cwd = NULL;\n> > startup_info->original_cwd = strbuf_detach(&tmp, NULL);\n>\n> It is somewhat sad that we cannot readily use FREE_AND_NULL() here.\n> If it casted away the constness (see the attached at the end), we\n> could have saved two lines from the above snippet.\n>\n> The startup_info->original_cwd member is initialized to NULL, and\n> I think it is a safe assumption that it still is so when the control\n> reaches here. Otherwise, the assignment of strbuf_detach() to it\n> without first freeing the current value we see in the post context\n> is leaking already.\n>\n> So, overall this looks good to me. Anybody else who have already\n> spent cycles to review this want to add Reviewed-by: to it?\n\nWell, I added my Reviewed-by to v3 and apparently missed a few things\nwhich were fixed up in v4. So, my review apparently wasn't careful\nenough. But I am happy with this version, so here it is again:\n\nReviewed-by: Elijah Newren \n\n(Now someone is free to spot more problems and embarrass me even more...)\n\n>\n> Thanks.\n>\n> diff --git i/git-compat-util.h w/git-compat-util.h\n> index 58fd813bd0..56c6c48461 100644\n> --- i/git-compat-util.h\n> +++ w/git-compat-util.h\n> @@ -976,7 +976,7 @@ int xstrncmpz(const char *s, const char *t, size_t len);\n> * FREE_AND_NULL(ptr) is like free(ptr) followed by ptr = NULL. Note\n> * that ptr is used twice, so don't pass e.g. ptr++.\n> */\n> -#define FREE_AND_NULL(p) do { free(p); (p) = NULL; } while (0)\n> +#define FREE_AND_NULL(p) do { free((void*)p); (p) = NULL; } while (0)\n>\n> #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))\n> #define CALLOC_ARRAY(x, alloc) (x) = xcalloc((alloc), sizeof(*(x)))\n\nI also like this change, even if it feels like it should be part of a\nseparate patch.\n"},{"id":"455999","messageId":"xmqq7d6ab3i0.fsf@gitster.g","threadId":"57900","inReplyTo":"CABPp-BHZQXt-=XtTk7Ez463rhbcQQBNGUVkbbWiavZT+Rsjn_A@mail.gmail.com","subject":"Re: [PATCH v4] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-25T05:11:35Z","receivedAt":"2022-05-25T05:11:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> diff --git i/git-compat-util.h w/git-compat-util.h\n>> index 58fd813bd0..56c6c48461 100644\n>> --- i/git-compat-util.h\n>> +++ w/git-compat-util.h\n>> @@ -976,7 +976,7 @@ int xstrncmpz(const char *s, const char *t, size_t len);\n>> * FREE_AND_NULL(ptr) is like free(ptr) followed by ptr = NULL. Note\n>> * that ptr is used twice, so don't pass e.g. ptr++.\n>> */\n>> -#define FREE_AND_NULL(p) do { free(p); (p) = NULL; } while (0)\n>> +#define FREE_AND_NULL(p) do { free((void*)p); (p) = NULL; } while (0)\n>>\n>> #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))\n>> #define CALLOC_ARRAY(x, alloc) (x) = xcalloc((alloc), sizeof(*(x)))\n>\n> I also like this change, even if it feels like it should be part of a\n> separate patch.\n\nSorry, but I did fail to make it clear that I didn't mean the above\nto be anything related to Kevin's patch. It was \"if FREE_AND_NULL()\nwere defined like this, then we could have used it\". I didn't mean\nto say \"let's update FREE_AND_NULL() this way so that we can use\nit\".\n\nThanks.\n"},{"id":"456268","messageId":"220527.865ylr4d4g.gmgdl@evledraar.gmail.com","threadId":"57900","inReplyTo":"CABPp-BGZTDKorz+CFScfTfx47c+TuJaAD_Zyyo1Jj_tymYkVXQ@mail.gmail.com","subject":"Re: [PATCH v2] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-27T07:48:43Z","receivedAt":"2022-05-27T08:02:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 24 2022, Elijah Newren wrote:\n\n> [...] So, I think the signature of free() is just\n> wrong: it should have taken a const void* all along. Unfortunately,\n> the wrong type signature sadly makes people feel like they have to\n> choose between (a) dropping the added safety of const that the\n> compiler can enforce for you during the lifetime of the variable, or\n> (b) leaking memory you no longer need.\n\nHrm, don't you mean that it would be better as:\n\n\tvoid free(void *const ptr);\n\nNot:\n\n\tvoid free(const void *ptr);\n\nBut standard C doesn't make much (any?) use of the former form for its\nlibrary functions by convention.\n\nc.f.:\n\n\tcdecl> explain const void *var\n\tdeclare var as pointer to const void\n\tcdecl> explain void *const var\n\tdeclare var as const pointer to void\n\nI.e. the whole point of malloc() is to give us a pointer to memory that\nisn't \"const\". If we stored that in a variable that was \"void *const\"\nwe'd save ourselves from some amount of foot-guns, but you're rarely\ndoing pointer arithmetic accidentally, so probably not really.\n\nBut yeah, we really should have this documented somewhere, i.e. the\ncases where we \"lie\" and expose a \"const char *\" which is really (as far\nas the machine is concerned) mutable.\n\nThe confusion being that we're seeking to overlay our own \"no, this\nisn't mutable\" on the basis of our desired API boundaries, not just to\nuse it to inform us & the compiler about the \"real\" nature of the\nunderlying data.\n"},{"id":"456341","messageId":"CABPp-BEd9bRuh9ruaDN32RUUT8ZtdQGPefVmiB+HM-gFCdnZ7A@mail.gmail.com","threadId":"57900","inReplyTo":"220527.865ylr4d4g.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] setup: don't die if realpath(3) fails on getcwd(3)","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-05-28T01:27:48Z","receivedAt":"2022-05-28T01:28:09Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, May 27, 2022 at 1:02 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Tue, May 24 2022, Elijah Newren wrote:\n>\n> > [...] So, I think the signature of free() is just\n> > wrong: it should have taken a const void* all along. Unfortunately,\n> > the wrong type signature sadly makes people feel like they have to\n> > choose between (a) dropping the added safety of const that the\n> > compiler can enforce for you during the lifetime of the variable, or\n> > (b) leaking memory you no longer need.\n>\n> Hrm, don't you mean that it would be better as:\n>\n> void free(void *const ptr);\n>\n> Not:\n>\n> void free(const void *ptr);\n\nNope, I definitely meant the latter; the stuff pointed to is const,\nnot the pointer itself.\n\nIn fact, I don't see any point at all in the former; with the free()\nthat exists today:\n\n void free(void *ptr)\n\nI can pass it a \"void * const myptr\" already without problems, because\nfree's ptr parameter will be a copy of myptr, and thus modifying ptr\ncannot affect myptr. So such a call signature change could not\npossibly provide any benefit to the outside caller. But that call\nsignature change could hinder the actual implementation of free() for\nsome folks (particularly if a given implementation of free() keeps\nextra data near the allocated block with information about the size of\nthe block and the next allocated block in the list).\n\nIn contrast, I cannot pass a \"const void *myptr\" or \"const char\n*myptr\" to free(), but only because of the current type signature;\nfree() doesn't actually modify any of the contents the pointer points\nto. (And if you want to claim that free effectively does modify what\nmyptr points to because someone else could allocate that same memory,\nremember that use-after-free is undefined regardless of whether the\ndata pointed to is const or not, and thus you cannot access that data\nafter free with or without the const.) So, free()'s real type that it\nacts on is a const void *. Sadly, the declared type signature is\nrather void *, which unnecessarily forces users to cast their types\nwhen calling.\n"}]}