{"thread":{"id":"61270","subject":"[PATCH] apply: replace mksnpath() with a mkpathdup() call","startedAt":"2024-04-04T21:08:51Z","lastAt":"2024-04-24T01:11:39Z","messageCount":15,"participants":["René Scharfe","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"492269","messageId":"df774306-f29b-4a75-a282-59db89812b9a@web.de","threadId":"61270","inReplyTo":null,"subject":"[PATCH] apply: replace mksnpath() with a mkpathdup() call","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-04T21:08:38Z","receivedAt":"2024-04-04T21:08:51Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Support paths longer than PATH_MAX in create_one_file() (which is not a\nhard limit e.g. on Linux) by calling mkpathdup() instead of mksnpath().\nRemove the latter, as it becomes unused by this change.  Resist the\ntemptation of using the more convenient mkpath() to avoid introducing a\ndependency on a static variable deep inside the apply machinery.\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n apply.c | 13 +++++++++----\n path.c  | 17 -----------------\n path.h  |  6 ------\n 3 files changed, 9 insertions(+), 27 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 432837a674..4793b05f3d 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4502,17 +4502,22 @@ static int create_one_file(struct apply_state *state,\n \t\tunsigned int nr = getpid();\n\n \t\tfor (;;) {\n-\t\t\tchar newpath[PATH_MAX];\n-\t\t\tmksnpath(newpath, sizeof(newpath), \"%s~%u\", path, nr);\n+\t\t\tchar *newpath = mkpathdup(\"%s~%u\", path, nr);\n \t\t\tres = try_create_file(state, newpath, mode, buf, size);\n-\t\t\tif (res < 0)\n+\t\t\tif (res < 0) {\n+\t\t\t\tfree(newpath);\n \t\t\t\treturn -1;\n+\t\t\t}\n \t\t\tif (!res) {\n-\t\t\t\tif (!rename(newpath, path))\n+\t\t\t\tif (!rename(newpath, path)) {\n+\t\t\t\t\tfree(newpath);\n \t\t\t\t\treturn 0;\n+\t\t\t\t}\n \t\t\t\tunlink_or_warn(newpath);\n+\t\t\t\tfree(newpath);\n \t\t\t\tbreak;\n \t\t\t}\n+\t\t\tfree(newpath);\n \t\t\tif (errno != EEXIST)\n \t\t\t\tbreak;\n \t\t\t++nr;\ndiff --git a/path.c b/path.c\nindex 8bb223c92c..67229edb9c 100644\n--- a/path.c\n+++ b/path.c\n@@ -28,8 +28,6 @@ static int get_st_mode_bits(const char *path, int *mode)\n \treturn 0;\n }\n\n-static char bad_path[] = \"/bad-path/\";\n-\n static struct strbuf *get_pathname(void)\n {\n \tstatic struct strbuf pathname_array[4] = {\n@@ -59,21 +57,6 @@ static void strbuf_cleanup_path(struct strbuf *sb)\n \t\tstrbuf_remove(sb, 0, path - sb->buf);\n }\n\n-char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n-{\n-\tva_list args;\n-\tunsigned len;\n-\n-\tva_start(args, fmt);\n-\tlen = vsnprintf(buf, n, fmt, args);\n-\tva_end(args);\n-\tif (len >= n) {\n-\t\tstrlcpy(buf, bad_path, n);\n-\t\treturn buf;\n-\t}\n-\treturn (char *)cleanup_path(buf);\n-}\n-\n static int dir_prefix(const char *buf, const char *dir)\n {\n \tint len = strlen(dir);\ndiff --git a/path.h b/path.h\nindex e053effef2..ea96487b29 100644\n--- a/path.h\n+++ b/path.h\n@@ -23,12 +23,6 @@ const char *mkpath(const char *fmt, ...)\n char *mkpathdup(const char *fmt, ...)\n \t__attribute__((format (printf, 1, 2)));\n\n-/*\n- * Construct a path and place the result in the provided buffer `buf`.\n- */\n-char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n-\t__attribute__((format (printf, 3, 4)));\n-\n /*\n  * The `git_common_path` family of functions will construct a path into a\n  * repository's common git directory, which is shared by all worktrees.\n--\n2.44.0\n"},{"id":"492271","messageId":"xmqqo7aozuih.fsf@gitster.g","threadId":"61270","inReplyTo":"df774306-f29b-4a75-a282-59db89812b9a@web.de","subject":"Re: [PATCH] apply: replace mksnpath() with a mkpathdup() call","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-04T21:29:42Z","receivedAt":"2024-04-04T21:29:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Support paths longer than PATH_MAX in create_one_file() (which is not a\n> hard limit e.g. on Linux) by calling mkpathdup() instead of mksnpath().\n> Remove the latter, as it becomes unused by this change.  Resist the\n> temptation of using the more convenient mkpath() to avoid introducing a\n> dependency on a static variable deep inside the apply machinery.\n>\n> Suggested-by: Jeff King <peff@peff.net>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  apply.c | 13 +++++++++----\n>  path.c  | 17 -----------------\n>  path.h  |  6 ------\n>  3 files changed, 9 insertions(+), 27 deletions(-)\n\nAs a patch to \"apply\", this is perfectly good, but in the larger\npicture, the value of the patch is in getting rid of mksnpath, isn't\nit?\n\nI am tempted to\n\n - queue it on rs/retire-mksnpath topic branch\n\n - remove the mention of mksnpath in contrib/vscode/init.sh (I think\n   it is registering non-words to spell checker, so having an extra\n   entry there would not hurt, but it would more likely to be a typo\n   of mkpath than before this patch retires mksnpath function).\n\n - ask you to come up with a title with more focus on mksnpath than\n   on apply.\n\nThanks.\n\n> diff --git a/apply.c b/apply.c\n> index 432837a674..4793b05f3d 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -4502,17 +4502,22 @@ static int create_one_file(struct apply_state *state,\n>  \t\tunsigned int nr = getpid();\n>\n>  \t\tfor (;;) {\n> -\t\t\tchar newpath[PATH_MAX];\n> -\t\t\tmksnpath(newpath, sizeof(newpath), \"%s~%u\", path, nr);\n> +\t\t\tchar *newpath = mkpathdup(\"%s~%u\", path, nr);\n>  \t\t\tres = try_create_file(state, newpath, mode, buf, size);\n> -\t\t\tif (res < 0)\n> +\t\t\tif (res < 0) {\n> +\t\t\t\tfree(newpath);\n>  \t\t\t\treturn -1;\n> +\t\t\t}\n>  \t\t\tif (!res) {\n> -\t\t\t\tif (!rename(newpath, path))\n> +\t\t\t\tif (!rename(newpath, path)) {\n> +\t\t\t\t\tfree(newpath);\n>  \t\t\t\t\treturn 0;\n> +\t\t\t\t}\n>  \t\t\t\tunlink_or_warn(newpath);\n> +\t\t\t\tfree(newpath);\n>  \t\t\t\tbreak;\n>  \t\t\t}\n> +\t\t\tfree(newpath);\n>  \t\t\tif (errno != EEXIST)\n>  \t\t\t\tbreak;\n>  \t\t\t++nr;\n> diff --git a/path.c b/path.c\n> index 8bb223c92c..67229edb9c 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -28,8 +28,6 @@ static int get_st_mode_bits(const char *path, int *mode)\n>  \treturn 0;\n>  }\n>\n> -static char bad_path[] = \"/bad-path/\";\n> -\n>  static struct strbuf *get_pathname(void)\n>  {\n>  \tstatic struct strbuf pathname_array[4] = {\n> @@ -59,21 +57,6 @@ static void strbuf_cleanup_path(struct strbuf *sb)\n>  \t\tstrbuf_remove(sb, 0, path - sb->buf);\n>  }\n>\n> -char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n> -{\n> -\tva_list args;\n> -\tunsigned len;\n> -\n> -\tva_start(args, fmt);\n> -\tlen = vsnprintf(buf, n, fmt, args);\n> -\tva_end(args);\n> -\tif (len >= n) {\n> -\t\tstrlcpy(buf, bad_path, n);\n> -\t\treturn buf;\n> -\t}\n> -\treturn (char *)cleanup_path(buf);\n> -}\n> -\n>  static int dir_prefix(const char *buf, const char *dir)\n>  {\n>  \tint len = strlen(dir);\n> diff --git a/path.h b/path.h\n> index e053effef2..ea96487b29 100644\n> --- a/path.h\n> +++ b/path.h\n> @@ -23,12 +23,6 @@ const char *mkpath(const char *fmt, ...)\n>  char *mkpathdup(const char *fmt, ...)\n>  \t__attribute__((format (printf, 1, 2)));\n>\n> -/*\n> - * Construct a path and place the result in the provided buffer `buf`.\n> - */\n> -char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n> -\t__attribute__((format (printf, 3, 4)));\n> -\n>  /*\n>   * The `git_common_path` family of functions will construct a path into a\n>   * repository's common git directory, which is shared by all worktrees.\n> --\n> 2.44.0\n"},{"id":"492275","messageId":"20240404225313.GA2512966@coredump.intra.peff.net","threadId":"61270","inReplyTo":"df774306-f29b-4a75-a282-59db89812b9a@web.de","subject":"free and errno, was Re: [PATCH] apply: replace mksnpath() with a mkpathdup() call","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-04T22:53:13Z","receivedAt":"2024-04-04T22:53:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2024 at 11:08:38PM +0200, René Scharfe wrote:\n\n> Support paths longer than PATH_MAX in create_one_file() (which is not a\n> hard limit e.g. on Linux) by calling mkpathdup() instead of mksnpath().\n> Remove the latter, as it becomes unused by this change.  Resist the\n> temptation of using the more convenient mkpath() to avoid introducing a\n> dependency on a static variable deep inside the apply machinery.\n> \n> Suggested-by: Jeff King <peff@peff.net>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n\nHeh, so obviously I had the same patch stewing. But one thing that gave\nme pause is: do we need to worry about preserving errno across free()\nboundaries?\n\nTraditionally touching errno was something free() was allowed to do, and\nthere were definitely cases where glibc would do so (mostly due to\nmunmap). But recent versions of POSIX clarify that it should not touch\nerrno, and glibc as of 2.33 does not (which dates to 2021).\n\nThis issue from 2015 talks about \"the next version of POSIX\" making that\nchange:\n\n  https://sourceware.org/bugzilla/show_bug.cgi?id=17924\n\nbut it looks to me from:\n\n  https://www.austingroupbugs.net/view.php?id=385\n\nthat the change wasn't accepted there until 2020 (and AFAICT that hasn't\nresulted in a new published spec yet).\n\nNow it would be pretty darn useful to not have to worry about preserving\nerrno. It's subtle code that's hard to remember is needed, and sometimes\nhard to get right depending on the rest of the flow control.\n\nYears like \"2020\" and \"2021\" are too recent for us to say \"oh, that's\nancient history, we don't have to care anymore\". But I wonder if we can\nbe a little cavalier here for two reasons:\n\n  - it's rare; for the most part free() isn't going to touch errno.\n    Failures from munmap() are pretty rare, and small allocations like\n    this are probably done with sbrk() anyway. Of course that's just\n    talking about glibc, and there are other platforms. But it still\n    seems like it should be a rarity for any free() implementation to\n    fail or to want to touch errno.\n\n  - the stakes are usually pretty low; the outcome in most cases would\n    be a misleading error message as we clobber errno. But note that it\n    does sometimes affect control flow (this patch is an example; we are\n    checking for EEXIST to break out of the loop).\n\nSo would it be that unreasonable to assume the modern, desired behavior,\nand mostly shrug our shoulders for other platforms? We could even\nprovide:\n\n  #ifdef FREE_CLOBBERS_ERRNO\n  void git_free(void *ptr)\n  {\n        int saved_errno = errno;\n        free(ptr);\n        errno = saved_errno;\n  }\n  #define free(ptr) git_free(ptr)\n  #endif\n\nif we really wanted to be careful, though it's hard to know which\nplatforms even need it (and again, it's so rare to come up in practice\nthat I'd suspect people could go for a long time before realizing their\nplatform was a problem). I guess the flip side is that we could use the\nfunction above by default, and disable it selectively (the advantage of\ndisabling it being that it's slightly more efficient; maybe that's not\neven measurable?).\n\n>  \t\tfor (;;) {\n> -\t\t\tchar newpath[PATH_MAX];\n> -\t\t\tmksnpath(newpath, sizeof(newpath), \"%s~%u\", path, nr);\n> +\t\t\tchar *newpath = mkpathdup(\"%s~%u\", path, nr);\n>  \t\t\tres = try_create_file(state, newpath, mode, buf, size);\n> -\t\t\tif (res < 0)\n> +\t\t\tif (res < 0) {\n> +\t\t\t\tfree(newpath);\n>  \t\t\t\treturn -1;\n> +\t\t\t}\n>  \t\t\tif (!res) {\n> -\t\t\t\tif (!rename(newpath, path))\n> +\t\t\t\tif (!rename(newpath, path)) {\n> +\t\t\t\t\tfree(newpath);\n>  \t\t\t\t\treturn 0;\n> +\t\t\t\t}\n>  \t\t\t\tunlink_or_warn(newpath);\n> +\t\t\t\tfree(newpath);\n>  \t\t\t\tbreak;\n>  \t\t\t}\n> +\t\t\tfree(newpath);\n>  \t\t\tif (errno != EEXIST)\n>  \t\t\t\tbreak;\n>  \t\t\t++nr;\n\nAt any rate, you can probably see the places where free() clobbering\nerrno would be a problem here. Our return when \"res < 0\" (though I don't\nthink any of the callers actually care about errno after that), the\ncheck for EEXIST at the bottom of the loop, and after we break out of\nthe loop, we use error_errno() to report it.\n\n-Peff\n"},{"id":"492276","messageId":"xmqqv84wybdo.fsf@gitster.g","threadId":"61270","inReplyTo":"20240404225313.GA2512966@coredump.intra.peff.net","subject":"Re: free and errno, was Re: [PATCH] apply: replace mksnpath() with a mkpathdup() call","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-04T23:08:19Z","receivedAt":"2024-04-04T23:08:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> +\t\t\tfree(newpath);\n>>  \t\t\tif (errno != EEXIST)\n>>  \t\t\t\tbreak;\n>>  \t\t\t++nr;\n>\n> At any rate, you can probably see the places where free() clobbering\n> errno would be a problem here. Our return when \"res < 0\" (though I don't\n> think any of the callers actually care about errno after that), the\n> check for EEXIST at the bottom of the loop, and after we break out of\n> the loop, we use error_errno() to report it.\n\nYeah, a failing free() is unlikely to set errno to EEXIST ;-)\n"},{"id":"492288","messageId":"14c99998-cc4a-4581-aab3-607d7fac7edb@web.de","threadId":"61270","inReplyTo":"20240404225313.GA2512966@coredump.intra.peff.net","subject":"Re: free and errno, was Re: [PATCH] apply: replace mksnpath() with a mkpathdup() call","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-05T10:52:35Z","receivedAt":"2024-04-05T10:52:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.04.24 um 00:53 schrieb Jeff King:\n> On Thu, Apr 04, 2024 at 11:08:38PM +0200, René Scharfe wrote:\n>\n>> Support paths longer than PATH_MAX in create_one_file() (which is not a\n>> hard limit e.g. on Linux) by calling mkpathdup() instead of mksnpath().\n>> Remove the latter, as it becomes unused by this change.  Resist the\n>> temptation of using the more convenient mkpath() to avoid introducing a\n>> dependency on a static variable deep inside the apply machinery.\n>>\n>> Suggested-by: Jeff King <peff@peff.net>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>\n> Heh, so obviously I had the same patch stewing. But one thing that gave\n> me pause is: do we need to worry about preserving errno across free()\n> boundaries?\n>\n> Traditionally touching errno was something free() was allowed to do, and\n> there were definitely cases where glibc would do so (mostly due to\n> munmap). But recent versions of POSIX clarify that it should not touch\n> errno, and glibc as of 2.33 does not (which dates to 2021).\n>\n> This issue from 2015 talks about \"the next version of POSIX\" making that\n> change:\n>\n>   https://sourceware.org/bugzilla/show_bug.cgi?id=17924\n>\n> but it looks to me from:\n>\n>   https://www.austingroupbugs.net/view.php?id=385\n>\n> that the change wasn't accepted there until 2020 (and AFAICT that hasn't\n> resulted in a new published spec yet).\n\nHorrible.  OS implementation details peeking through an API that has no\nreturn value and no defined errors is outright Lovecraftian.\n\n> Now it would be pretty darn useful to not have to worry about preserving\n> errno. It's subtle code that's hard to remember is needed, and sometimes\n> hard to get right depending on the rest of the flow control.\n\nYes.\n\n> Years like \"2020\" and \"2021\" are too recent for us to say \"oh, that's\n> ancient history, we don't have to care anymore\". But I wonder if we can\n> be a little cavalier here for two reasons:\n>\n>   - it's rare; for the most part free() isn't going to touch errno.\n>     Failures from munmap() are pretty rare, and small allocations like\n>     this are probably done with sbrk() anyway. Of course that's just\n>     talking about glibc, and there are other platforms. But it still\n>     seems like it should be a rarity for any free() implementation to\n>     fail or to want to touch errno.\n>\n>   - the stakes are usually pretty low; the outcome in most cases would\n>     be a misleading error message as we clobber errno. But note that it\n>     does sometimes affect control flow (this patch is an example; we are\n>     checking for EEXIST to break out of the loop).\n>\n> So would it be that unreasonable to assume the modern, desired behavior,\n> and mostly shrug our shoulders for other platforms? We could even\n> provide:\n>\n>   #ifdef FREE_CLOBBERS_ERRNO\n>   void git_free(void *ptr)\n>   {\n>         int saved_errno = errno;\n>         free(ptr);\n>         errno = saved_errno;\n>   }\n>   #define free(ptr) git_free(ptr)\n>   #endif\n>\n> if we really wanted to be careful, though it's hard to know which\n> platforms even need it (and again, it's so rare to come up in practice\n> that I'd suspect people could go for a long time before realizing their\n> platform was a problem). I guess the flip side is that we could use the\n> function above by default, and disable it selectively (the advantage of\n> disabling it being that it's slightly more efficient; maybe that's not\n> even measurable?).\n\nI think performing this ritual automatically every time on all platforms\n(i.e. to use git_free() unconditionally) to provide sane errno values\naround free(3) calls is better than having to worry about attacks from\nthe deep.  But then again I'm easily scared and still a bit in shock, so\nperhaps I'm overreacting.\n\nRené\n"},{"id":"492289","messageId":"1b5f3b1d-60e2-4fe7-9ac8-a63ad861cd16@web.de","threadId":"61270","inReplyTo":"df774306-f29b-4a75-a282-59db89812b9a@web.de","subject":"[PATCH v2 1/2] apply: avoid fixed-size buffer in create_one_file()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-05T10:53:23Z","receivedAt":"2024-04-05T10:53:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"PATH_MAX is not always a hard limit and 'path' in create_one_file()\ncould be longer -- it's taken from the patch file and allocated\ndynamically.  Allocate the name of the temporary file on the heap as\nwell instead of using a fixed-size buffer to avoid that arbitrary limit.\n\nResist the temptation of using the more convenient mkpath() to avoid\nintroducing a dependency on a static variable deep inside the apply\nmachinery.\n\nTake care to work around (arguably buggy) implementations of free(3)\nthat modify errno, by calling it only after using the errno value.\n\nSuggested-by: Jeff King <peff@peff.net>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nChanges since v1:\n- Split out removal of mksnpath() into a separate patch.\n- Use errno only before calling free(3).\n\n apply.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 432837a674..e171b42904 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4441,6 +4441,7 @@ static int create_one_file(struct apply_state *state,\n \t\t\t   const char *buf,\n \t\t\t   unsigned long size)\n {\n+\tchar *newpath = NULL;\n \tint res;\n\n \tif (state->cached)\n@@ -4502,24 +4503,26 @@ static int create_one_file(struct apply_state *state,\n \t\tunsigned int nr = getpid();\n\n \t\tfor (;;) {\n-\t\t\tchar newpath[PATH_MAX];\n-\t\t\tmksnpath(newpath, sizeof(newpath), \"%s~%u\", path, nr);\n+\t\t\tnewpath = mkpathdup(\"%s~%u\", path, nr);\n \t\t\tres = try_create_file(state, newpath, mode, buf, size);\n \t\t\tif (res < 0)\n-\t\t\t\treturn -1;\n+\t\t\t\tgoto out;\n \t\t\tif (!res) {\n \t\t\t\tif (!rename(newpath, path))\n-\t\t\t\t\treturn 0;\n+\t\t\t\t\tgoto out;\n \t\t\t\tunlink_or_warn(newpath);\n \t\t\t\tbreak;\n \t\t\t}\n \t\t\tif (errno != EEXIST)\n \t\t\t\tbreak;\n \t\t\t++nr;\n+\t\t\tFREE_AND_NULL(newpath);\n \t\t}\n \t}\n-\treturn error_errno(_(\"unable to write file '%s' mode %o\"),\n-\t\t\t   path, mode);\n+\tres = error_errno(_(\"unable to write file '%s' mode %o\"), path, mode);\n+out:\n+\tfree(newpath);\n+\treturn res;\n }\n\n static int add_conflicted_stages_file(struct apply_state *state,\n--\n2.44.0\n"},{"id":"492290","messageId":"c085abab-2f10-488d-87d4-65e92e0ea24b@web.de","threadId":"61270","inReplyTo":"1b5f3b1d-60e2-4fe7-9ac8-a63ad861cd16@web.de","subject":"[PATCH v2 2/2] path: remove mksnpath()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-05T10:56:48Z","receivedAt":"2024-04-05T10:56:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Remove the function mksnpath(), which has become unused.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nChanges since v1:\n- Split out removal of mksnpath() into a separate patch, this one.\n- Remove occurrence in contrib/vscode/init.sh as well.\n\n contrib/vscode/init.sh |  1 -\n path.c                 | 17 -----------------\n path.h                 |  6 ------\n 3 files changed, 24 deletions(-)\n\ndiff --git a/contrib/vscode/init.sh b/contrib/vscode/init.sh\nindex 521d303722..f2d61bb0e6 100755\n--- a/contrib/vscode/init.sh\n+++ b/contrib/vscode/init.sh\n@@ -92,7 +92,6 @@ cat >.vscode/settings.json.new <<\\EOF ||\n         \"isexe\",\n         \"iskeychar\",\n         \"kompare\",\n-        \"mksnpath\",\n         \"mktag\",\n         \"mktree\",\n         \"mmblob\",\ndiff --git a/path.c b/path.c\nindex 8bb223c92c..67229edb9c 100644\n--- a/path.c\n+++ b/path.c\n@@ -28,8 +28,6 @@ static int get_st_mode_bits(const char *path, int *mode)\n \treturn 0;\n }\n\n-static char bad_path[] = \"/bad-path/\";\n-\n static struct strbuf *get_pathname(void)\n {\n \tstatic struct strbuf pathname_array[4] = {\n@@ -59,21 +57,6 @@ static void strbuf_cleanup_path(struct strbuf *sb)\n \t\tstrbuf_remove(sb, 0, path - sb->buf);\n }\n\n-char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n-{\n-\tva_list args;\n-\tunsigned len;\n-\n-\tva_start(args, fmt);\n-\tlen = vsnprintf(buf, n, fmt, args);\n-\tva_end(args);\n-\tif (len >= n) {\n-\t\tstrlcpy(buf, bad_path, n);\n-\t\treturn buf;\n-\t}\n-\treturn (char *)cleanup_path(buf);\n-}\n-\n static int dir_prefix(const char *buf, const char *dir)\n {\n \tint len = strlen(dir);\ndiff --git a/path.h b/path.h\nindex e053effef2..ea96487b29 100644\n--- a/path.h\n+++ b/path.h\n@@ -23,12 +23,6 @@ const char *mkpath(const char *fmt, ...)\n char *mkpathdup(const char *fmt, ...)\n \t__attribute__((format (printf, 1, 2)));\n\n-/*\n- * Construct a path and place the result in the provided buffer `buf`.\n- */\n-char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n-\t__attribute__((format (printf, 3, 4)));\n-\n /*\n  * The `git_common_path` family of functions will construct a path into a\n  * repository's common git directory, which is shared by all worktrees.\n--\n2.44.0\n"},{"id":"492299","messageId":"xmqqfrvzwy5w.fsf@gitster.g","threadId":"61270","inReplyTo":"1b5f3b1d-60e2-4fe7-9ac8-a63ad861cd16@web.de","subject":"Re: [PATCH v2 1/2] apply: avoid fixed-size buffer in create_one_file()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-05T16:51:23Z","receivedAt":"2024-04-05T16:51:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Changes since v1:\n> - Split out removal of mksnpath() into a separate patch.\n> - Use errno only before calling free(3).\n\nRelative to the previous round, the body of the loop has become much\neasier to follow.  Nice clean-up.\n\nThanks, both.\n\n>  apply.c | 15 +++++++++------\n>  1 file changed, 9 insertions(+), 6 deletions(-)\n>\n> diff --git a/apply.c b/apply.c\n> index 432837a674..e171b42904 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -4441,6 +4441,7 @@ static int create_one_file(struct apply_state *state,\n>  \t\t\t   const char *buf,\n>  \t\t\t   unsigned long size)\n>  {\n> +\tchar *newpath = NULL;\n>  \tint res;\n>\n>  \tif (state->cached)\n> @@ -4502,24 +4503,26 @@ static int create_one_file(struct apply_state *state,\n>  \t\tunsigned int nr = getpid();\n>\n>  \t\tfor (;;) {\n> -\t\t\tchar newpath[PATH_MAX];\n> -\t\t\tmksnpath(newpath, sizeof(newpath), \"%s~%u\", path, nr);\n> +\t\t\tnewpath = mkpathdup(\"%s~%u\", path, nr);\n>  \t\t\tres = try_create_file(state, newpath, mode, buf, size);\n>  \t\t\tif (res < 0)\n> -\t\t\t\treturn -1;\n> +\t\t\t\tgoto out;\n>  \t\t\tif (!res) {\n>  \t\t\t\tif (!rename(newpath, path))\n> -\t\t\t\t\treturn 0;\n> +\t\t\t\t\tgoto out;\n>  \t\t\t\tunlink_or_warn(newpath);\n>  \t\t\t\tbreak;\n>  \t\t\t}\n>  \t\t\tif (errno != EEXIST)\n>  \t\t\t\tbreak;\n>  \t\t\t++nr;\n> +\t\t\tFREE_AND_NULL(newpath);\n>  \t\t}\n>  \t}\n> -\treturn error_errno(_(\"unable to write file '%s' mode %o\"),\n> -\t\t\t   path, mode);\n> +\tres = error_errno(_(\"unable to write file '%s' mode %o\"), path, mode);\n> +out:\n> +\tfree(newpath);\n> +\treturn res;\n>  }\n>\n>  static int add_conflicted_stages_file(struct apply_state *state,\n> --\n> 2.44.0\n"},{"id":"492306","messageId":"20240405173517.GA2529133@coredump.intra.peff.net","threadId":"61270","inReplyTo":"14c99998-cc4a-4581-aab3-607d7fac7edb@web.de","subject":"Re: free and errno, was Re: [PATCH] apply: replace mksnpath() with a mkpathdup() call","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-05T17:35:17Z","receivedAt":"2024-04-05T17:35:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2024 at 12:52:35PM +0200, René Scharfe wrote:\n\n> > So would it be that unreasonable to assume the modern, desired behavior,\n> > and mostly shrug our shoulders for other platforms? We could even\n> > provide:\n> >\n> >   #ifdef FREE_CLOBBERS_ERRNO\n> >   void git_free(void *ptr)\n> >   {\n> >         int saved_errno = errno;\n> >         free(ptr);\n> >         errno = saved_errno;\n> >   }\n> >   #define free(ptr) git_free(ptr)\n> >   #endif\n> >\n> > if we really wanted to be careful, though it's hard to know which\n> > platforms even need it (and again, it's so rare to come up in practice\n> > that I'd suspect people could go for a long time before realizing their\n> > platform was a problem). I guess the flip side is that we could use the\n> > function above by default, and disable it selectively (the advantage of\n> > disabling it being that it's slightly more efficient; maybe that's not\n> > even measurable?).\n> \n> I think performing this ritual automatically every time on all platforms\n> (i.e. to use git_free() unconditionally) to provide sane errno values\n> around free(3) calls is better than having to worry about attacks from\n> the deep.  But then again I'm easily scared and still a bit in shock, so\n> perhaps I'm overreacting.\n\nYou may be right. The main reason not to do it is the extra assignments\n(and call to errno, which I think can be a function hidden behind a\nmacro these days). But I kind of doubt it is measurable, and we expect\nmalloc/free to be a bit heavyweight (compared to regular instructions)\nanyway. So it is probably me being overly cautious about the performance\nside.\n\nThe other reason is that macros (especially of system names) can create\ntheir own headaches.  We could require xfree() everywhere as a\ncomplement to xmalloc (or maybe even just places where the errno\npreservation seems useful), but that's one more thing to remember.\n\nWith the patch below you can see both in action:\n\n  - hyperfine seems to show the git_free() version as ~1% slower to run\n    \"git log\" (which I picked as something that probably does a\n    reasonable number of mallocs). Frankly, I'm still suspicious that it\n    could have such an impact. Maybe inlining git_free() would help?\n\n  - usually #defining free(ptr) with an argument will avoid confusion\n    with the word \"free\" as a token. But in this case there's a function\n    pointer which is then called as struct->free(ptr), causing\n    confusion.\n\ndiff --git a/convert.c b/convert.c\nindex 35b25eb3cb..dfb31ee3a3 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -1549,7 +1549,7 @@ typedef void (*free_fn)(struct stream_filter *);\n \n struct stream_filter_vtbl {\n \tfilter_fn filter;\n-\tfree_fn free;\n+\tfree_fn free_stream;\n };\n \n struct stream_filter {\n@@ -1582,7 +1582,7 @@ static void null_free_fn(struct stream_filter *filter UNUSED)\n \n static struct stream_filter_vtbl null_vtbl = {\n \t.filter = null_filter_fn,\n-\t.free = null_free_fn,\n+\t.free_stream = null_free_fn,\n };\n \n static struct stream_filter null_filter_singleton = {\n@@ -1691,7 +1691,7 @@ static void lf_to_crlf_free_fn(struct stream_filter *filter)\n \n static struct stream_filter_vtbl lf_to_crlf_vtbl = {\n \t.filter = lf_to_crlf_filter_fn,\n-\t.free = lf_to_crlf_free_fn,\n+\t.free_stream = lf_to_crlf_free_fn,\n };\n \n static struct stream_filter *lf_to_crlf_filter(void)\n@@ -1787,7 +1787,7 @@ static void cascade_free_fn(struct stream_filter *filter)\n \n static struct stream_filter_vtbl cascade_vtbl = {\n \t.filter = cascade_filter_fn,\n-\t.free = cascade_free_fn,\n+\t.free_stream = cascade_free_fn,\n };\n \n static struct stream_filter *cascade_filter(struct stream_filter *one,\n@@ -1939,7 +1939,7 @@ static void ident_free_fn(struct stream_filter *filter)\n \n static struct stream_filter_vtbl ident_vtbl = {\n \t.filter = ident_filter_fn,\n-\t.free = ident_free_fn,\n+\t.free_stream = ident_free_fn,\n };\n \n static struct stream_filter *ident_filter(const struct object_id *oid)\n@@ -1992,7 +1992,7 @@ struct stream_filter *get_stream_filter(struct index_state *istate,\n \n void free_stream_filter(struct stream_filter *filter)\n {\n-\tfilter->vtbl->free(filter);\n+\tfilter->vtbl->free_stream(filter);\n }\n \n int stream_filter(struct stream_filter *filter,\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 7c2a6538e5..324c47fdff 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1094,6 +1094,9 @@ static inline void move_array(void *dst, const void *src, size_t n, size_t size)\n \tCOPY_ARRAY(ALLOC_ARRAY((dst), dup_array_n_), (src), dup_array_n_); \\\n } while (0)\n \n+void git_free(void *ptr);\n+#define free(ptr) git_free(ptr)\n+\n /*\n  * These functions help you allocate structs with flex arrays, and copy\n  * the data directly into the array. For example, if you had:\ndiff --git a/wrapper.c b/wrapper.c\nindex eeac3741cf..cfffb177e3 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -822,3 +822,14 @@ uint32_t git_rand(void)\n \n \treturn result;\n }\n+\n+/* this should probably go in its own file, otherwise anything\n+ * added below here uses the bare free()\n+ */\n+#undef free\n+void git_free(void *ptr)\n+{\n+\tint saved_errno = errno;\n+\tfree(ptr);\n+\terrno = saved_errno;\n+}\n"},{"id":"492307","messageId":"20240405173718.GB2529133@coredump.intra.peff.net","threadId":"61270","inReplyTo":"c085abab-2f10-488d-87d4-65e92e0ea24b@web.de","subject":"Re: [PATCH v2 2/2] path: remove mksnpath()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-05T17:37:18Z","receivedAt":"2024-04-05T17:37:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2024 at 12:56:48PM +0200, René Scharfe wrote:\n\n> Remove the function mksnpath(), which has become unused.\n> \n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> Changes since v1:\n> - Split out removal of mksnpath() into a separate patch, this one.\n> - Remove occurrence in contrib/vscode/init.sh as well.\n\nThanks, both patches (including the errno handling in the first one)\nlook good to me. I'd probably have waxed philosophical about why\nmksnpath() is a bad interface in this one, but I think it's OK to skip\nthat. ;)\n\n-Peff\n"},{"id":"492308","messageId":"20240405174136.GC2529133@coredump.intra.peff.net","threadId":"61270","inReplyTo":"20240405173517.GA2529133@coredump.intra.peff.net","subject":"Re: free and errno, was Re: [PATCH] apply: replace mksnpath() with a mkpathdup() call","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-05T17:41:36Z","receivedAt":"2024-04-05T17:41:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2024 at 01:35:17PM -0400, Jeff King wrote:\n\n> With the patch below you can see both in action:\n> \n>   - hyperfine seems to show the git_free() version as ~1% slower to run\n>     \"git log\" (which I picked as something that probably does a\n>     reasonable number of mallocs). Frankly, I'm still suspicious that it\n>     could have such an impact. Maybe inlining git_free() would help?\n\nThis may have been noise. Weirdly it seemed consistent from run to run\nusing hyperfine. But switching my CPU governor from \"powersave\" to\n\"performance\", I got a run where the new code is fast.\n\n-Peff\n"},{"id":"492401","messageId":"0fc77134-94fb-4d60-95b4-509c3582e20f@web.de","threadId":"61270","inReplyTo":"20240405173517.GA2529133@coredump.intra.peff.net","subject":"Re: free and errno, was Re: [PATCH] apply: replace mksnpath() with a mkpathdup() call","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-06T17:45:17Z","receivedAt":"2024-04-06T17:45:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.04.24 um 19:35 schrieb Jeff King:\n> On Fri, Apr 05, 2024 at 12:52:35PM +0200, René Scharfe wrote:\n>\n>>> So would it be that unreasonable to assume the modern, desired behavior,\n>>> and mostly shrug our shoulders for other platforms? We could even\n>>> provide:\n>>>\n>>>   #ifdef FREE_CLOBBERS_ERRNO\n>>>   void git_free(void *ptr)\n>>>   {\n>>>         int saved_errno = errno;\n>>>         free(ptr);\n>>>         errno = saved_errno;\n>>>   }\n>>>   #define free(ptr) git_free(ptr)\n>>>   #endif\n>>>\n>>> if we really wanted to be careful, though it's hard to know which\n>>> platforms even need it (and again, it's so rare to come up in practice\n>>> that I'd suspect people could go for a long time before realizing their\n>>> platform was a problem). I guess the flip side is that we could use the\n>>> function above by default, and disable it selectively (the advantage of\n>>> disabling it being that it's slightly more efficient; maybe that's not\n>>> even measurable?).\n>>\n>> I think performing this ritual automatically every time on all platforms\n>> (i.e. to use git_free() unconditionally) to provide sane errno values\n>> around free(3) calls is better than having to worry about attacks from\n>> the deep.  But then again I'm easily scared and still a bit in shock, so\n>> perhaps I'm overreacting.\n\nI calmed down a bit now.  And ask myself how widespread the issue actually\nis.  Used the following silly Coccinelle rule to find functions that use\nerrno after a free(3) call:\n\n@@\n@@\n- free(...);\n  ...\n  errno\n\nFound only a handful of places, and they all set errno explicitly, so\nthey are fine.\n\nNo idea how to match any use of errno except assignment.  And no idea how\nto find indirect callers of free(3) that use errno with no potential\nassignment in between.\n\n> You may be right. The main reason not to do it is the extra assignments\n> (and call to errno, which I think can be a function hidden behind a\n> macro these days). But I kind of doubt it is measurable, and we expect\n> malloc/free to be a bit heavyweight (compared to regular instructions)\n> anyway. So it is probably me being overly cautious about the performance\n> side.\n>\n> The other reason is that macros (especially of system names) can create\n> their own headaches.  We could require xfree() everywhere as a\n> complement to xmalloc (or maybe even just places where the errno\n> preservation seems useful), but that's one more thing to remember.\n\nAn xfree() to go along with xmalloc()/xrealloc()/xcalloc()/xstrdup() would\nfit in nicely and might be easier to remember than free() after a while.\nHaving to convert thousands of calls is unappealing, though.\n\n> With the patch below you can see both in action:\n>\n>   - hyperfine seems to show the git_free() version as ~1% slower to run\n>     \"git log\" (which I picked as something that probably does a\n>     reasonable number of mallocs). Frankly, I'm still suspicious that it\n>     could have such an impact. Maybe inlining git_free() would help?\n\nI get a slowdown of ca. 0.5%:\n\nBenchmark 1: ./git_2.44.0 log v2.44.0\n  Time (mean ± σ):     705.8 ms ±   1.7 ms    [User: 674.0 ms, System: 28.3 ms]\n  Range (min … max):   702.3 ms … 709.2 ms    10 runs\n\nBenchmark 2: ./git log v2.44.0\n  Time (mean ± σ):     708.1 ms ±   2.0 ms    [User: 676.4 ms, System: 28.7 ms]\n  Range (min … max):   705.0 ms … 710.9 ms    10 runs\n\nHmm.\n\nWhat if we do the opposite and poison errno in git_free() instead of\ncarrying over the original value?  That's only half the work.\n\nBenchmark 1: ./git_2.44.0 log v2.44.0\n  Time (mean ± σ):     704.2 ms ±   1.3 ms    [User: 674.2 ms, System: 27.6 ms]\n  Range (min … max):   702.2 ms … 705.8 ms    10 runs\n\nBenchmark 2: ./git log v2.44.0\n  Time (mean ± σ):     706.3 ms ±   0.9 ms    [User: 676.3 ms, System: 27.5 ms]\n  Range (min … max):   704.8 ms … 708.2 ms    10 runs\n\nSo that's slightly better.  But the measurements are quite noisy.\n\nFound four places that did not expect free(3) to mess up their errno by\nrunning the test suite with that.  Patch below.\n\n>   - usually #defining free(ptr) with an argument will avoid confusion\n>     with the word \"free\" as a token. But in this case there's a function\n>     pointer which is then called as struct->free(ptr), causing\n>     confusion.\n\nIn other contexts we use \"clear\" or \"release\" to name functions like that.\n\nRené\n\n\n compat/precompose_utf8.c | 2 ++\n git.c                    | 3 +++\n lockfile.c               | 3 +++\n tempfile.c               | 2 ++\n 4 files changed, 10 insertions(+)\n\ndiff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c\nindex 0bd5c24250..4da80b40ce 100644\n--- a/compat/precompose_utf8.c\n+++ b/compat/precompose_utf8.c\n@@ -114,8 +114,10 @@ PREC_DIR *precompose_utf8_opendir(const char *dirname)\n\n \tprec_dir->dirp = opendir(dirname);\n \tif (!prec_dir->dirp) {\n+\t\tint save_errno = errno;\n \t\tfree(prec_dir->dirent_nfc);\n \t\tfree(prec_dir);\n+\t\terrno = save_errno;\n \t\treturn NULL;\n \t} else {\n \t\tint ret_errno = errno;\ndiff --git a/git.c b/git.c\nindex 654d615a18..275674f873 100644\n--- a/git.c\n+++ b/git.c\n@@ -773,6 +773,7 @@ static int run_argv(int *argcp, const char ***argv)\n \tint done_alias = 0;\n \tstruct string_list cmd_list = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *seen;\n+\tint save_errno;\n\n \twhile (1) {\n \t\t/*\n@@ -853,7 +854,9 @@ static int run_argv(int *argcp, const char ***argv)\n \t\tdone_alias = 1;\n \t}\n\n+\tsave_errno = errno;\n \tstring_list_clear(&cmd_list, 0);\n+\terrno = save_errno;\n\n \treturn done_alias;\n }\ndiff --git a/lockfile.c b/lockfile.c\nindex 1d5ed01682..b8401f5059 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -76,6 +76,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags,\n \t\t     int mode)\n {\n \tstruct strbuf filename = STRBUF_INIT;\n+\tint save_errno;\n\n \tstrbuf_addstr(&filename, path);\n \tif (!(flags & LOCK_NO_DEREF))\n@@ -83,7 +84,9 @@ static int lock_file(struct lock_file *lk, const char *path, int flags,\n\n \tstrbuf_addstr(&filename, LOCK_SUFFIX);\n \tlk->tempfile = create_tempfile_mode(filename.buf, mode);\n+\tsave_errno = errno;\n \tstrbuf_release(&filename);\n+\terrno = save_errno;\n \treturn lk->tempfile ? lk->tempfile->fd : -1;\n }\n\ndiff --git a/tempfile.c b/tempfile.c\nindex ed88cf8431..029b760fa9 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -144,7 +144,9 @@ struct tempfile *create_tempfile_mode(const char *path, int mode)\n \t\ttempfile->fd = open(tempfile->filename.buf,\n \t\t\t\t    O_RDWR | O_CREAT | O_EXCL, mode);\n \tif (tempfile->fd < 0) {\n+\t\tint save_errno = errno;\n \t\tdeactivate_tempfile(tempfile);\n+\t\terrno = save_errno;\n \t\treturn NULL;\n \t}\n \tactivate_tempfile(tempfile);\n"},{"id":"492422","messageId":"20240407011834.GA1085004@coredump.intra.peff.net","threadId":"61270","inReplyTo":"0fc77134-94fb-4d60-95b4-509c3582e20f@web.de","subject":"Re: free and errno, was Re: [PATCH] apply: replace mksnpath() with a mkpathdup() call","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-07T01:18:34Z","receivedAt":"2024-04-07T01:18:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 06, 2024 at 07:45:17PM +0200, René Scharfe wrote:\n\n> I calmed down a bit now.  And ask myself how widespread the issue actually\n> is.  Used the following silly Coccinelle rule to find functions that use\n> errno after a free(3) call:\n> \n> @@\n> @@\n> - free(...);\n>   ...\n>   errno\n> \n> Found only a handful of places, and they all set errno explicitly, so\n> they are fine.\n\nIs that enough, though? Imagine I have:\n\n  int foo(const char *x)\n  {\n     char *y = xstfrmt(\"something/%s\", x);\n     int fd = open(y, ...);\n     free(y);\n     return fd;\n  }\n\nThen errno is important if some caller of foo() checks errno after foo()\nreturns an error. And that caller might not even be in the same file.\n\nIn other words, it really depends on the contract of foo() with respect\nto errno. And I don't think there is a way in C to specify that\ncontract in away that the compiler can understand.\n\n> No idea how to match any use of errno except assignment.  And no idea how\n> to find indirect callers of free(3) that use errno with no potential\n> assignment in between.\n\nYeah, I guess the indirect callers of free() are really the flip-side.\nMy example was indirect users of errno. ;)\n\n> > The other reason is that macros (especially of system names) can create\n> > their own headaches.  We could require xfree() everywhere as a\n> > complement to xmalloc (or maybe even just places where the errno\n> > preservation seems useful), but that's one more thing to remember.\n> \n> An xfree() to go along with xmalloc()/xrealloc()/xcalloc()/xstrdup() would\n> fit in nicely and might be easier to remember than free() after a while.\n> Having to convert thousands of calls is unappealing, though.\n\nMy biggest concern with it is that we'd have to remember to use it, and\nthere's not good compiler enforcement. But I guess coccinelle can help\nus there.\n\nMy secondary concern is that it might make people think that xmalloc()\nand xfree() are always paired, and thus you can do clever things in one\nas long as the other matches it. But we sometimes free memory from\nsystem routines like getline(). Maybe a scary comment would be enough?\n\n> Found four places that did not expect free(3) to mess up their errno by\n> running the test suite with that.  Patch below.\n\nThese are perhaps worth fixing (though not if we come up with a\nuniversal solution). But I'd be surprised if they are the only ones. By\nits nature, this problem only manifests when there are actual errors,\nand our test suite is mostly checking happy paths. So I'd assume there\nare a ton of \"if (ret < 0) { free(foo); return -1; }\" spots that are\nsimply not exercised by the test suite at all.\n\n-Peff\n"},{"id":"492922","messageId":"10b7fff3-7a2c-4555-9210-8000aae43583@web.de","threadId":"61270","inReplyTo":"20240407011834.GA1085004@coredump.intra.peff.net","subject":"Re: free and errno, was Re: [PATCH] apply: replace mksnpath() with a mkpathdup() call","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-14T15:17:23Z","receivedAt":"2024-04-14T15:17:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.04.24 um 03:18 schrieb Jeff King:\n> On Sat, Apr 06, 2024 at 07:45:17PM +0200, René Scharfe wrote:\n>\n>> I calmed down a bit now.  And ask myself how widespread the issue actually\n>> is.  Used the following silly Coccinelle rule to find functions that use\n>> errno after a free(3) call:\n>>\n>> @@\n>> @@\n>> - free(...);\n>>   ...\n>>   errno\n>>\n>> Found only a handful of places, and they all set errno explicitly, so\n>> they are fine.\n>\n> Is that enough, though? Imagine I have:\n>\n>   int foo(const char *x)\n>   {\n>      char *y = xstfrmt(\"something/%s\", x);\n>      int fd = open(y, ...);\n>      free(y);\n>      return fd;\n>   }\n>\n> Then errno is important if some caller of foo() checks errno after foo()\n> returns an error. And that caller might not even be in the same file.\n\nYes.\n\n> In other words, it really depends on the contract of foo() with respect\n> to errno. And I don't think there is a way in C to specify that\n> contract in away that the compiler can understand.\n\nThe context attribute of sparse could be used, I guess.  We'd have to\nduplicate the declaration of all library functions, though, to tag them\nwith a may_have_changed_errno attribute.  And we'd need to clear it on\nsuccess, so we'd have to modify all callers.  On the flip side it would\nallow detecting consecutive errno changes, e.g. by two I/O functions\nwith no error checking in between.  But the implementation effort seems\nway too high.\n\nIt would be easier if the error information was in one place instead of\none bit in the return value (NULL or less than 0) and the rest in errno.\n\n>> No idea how to match any use of errno except assignment.  And no idea how\n>> to find indirect callers of free(3) that use errno with no potential\n>> assignment in between.\n>\n> Yeah, I guess the indirect callers of free() are really the flip-side.\n> My example was indirect users of errno. ;)\n\nIndeed.\n\n>>> The other reason is that macros (especially of system names) can create\n>>> their own headaches.  We could require xfree() everywhere as a\n>>> complement to xmalloc (or maybe even just places where the errno\n>>> preservation seems useful), but that's one more thing to remember.\n>>\n>> An xfree() to go along with xmalloc()/xrealloc()/xcalloc()/xstrdup() would\n>> fit in nicely and might be easier to remember than free() after a while.\n>> Having to convert thousands of calls is unappealing, though.\n>\n> My biggest concern with it is that we'd have to remember to use it, and\n> there's not good compiler enforcement. But I guess coccinelle can help\n> us there.q\n\nYes, converting all free(3) calls is easy with Coccinelle, and the same\nsemantic patch can be used to enforce the use of xfree().  Still the\ninitial diff would be huge (2000+ changed lines in 300+ files).\n\n> My secondary concern is that it might make people think that xmalloc()\n> and xfree() are always paired, and thus you can do clever things in one\n> as long as the other matches it. But we sometimes free memory from\n> system routines like getline(). Maybe a scary comment would be enough?\n\nAmazing foresight!  Currently we only use getline(3) (which can act like\nrealloc(3)) in contrib/credential/, though.  Any others?\n\nThe \"x\" prefix doesn't promise exclusivity (hermetic seal?), but it\ncertainly suggests there are some shared feature between the allocator\nfunctions and xfree(), while they only have in common that the are\nwrapped.  We could call the latter errno_preserving_free() instead or so.\n\nI'm more worried about the overhead.  For a 0-1% slowdown (in the case of\ngit log) git_free() or xfree() give us accurate error numbers on all\nplatforms.  Non-misleading error codes are worth a lot (seen my share of\ncursed messages), but xfree() is only necessary in the error handling\ncode.  The happy path can call free(3) directly without harm.\n\nBut how to classify call sites accordingly?  It's easy for programmers\nonce they know it's necessary.  Is there a way in C, macro or Coccinelle\nto have our cake and only eat it if really needed?  I don't see any. :-|\n\n>> Found four places that did not expect free(3) to mess up their errno by\n>> running the test suite with that.  Patch below.\n>\n> These are perhaps worth fixing (though not if we come up with a\n> universal solution). But I'd be surprised if they are the only ones. By\n> its nature, this problem only manifests when there are actual errors,\n> and our test suite is mostly checking happy paths. So I'd assume there\n> are a ton of \"if (ret < 0) { free(foo); return -1; }\" spots that are\n> simply not exercised by the test suite at all.\n\nMakes sense.\n\nRené\n"},{"id":"493384","messageId":"20240424011137.GA1180766@coredump.intra.peff.net","threadId":"61270","inReplyTo":"10b7fff3-7a2c-4555-9210-8000aae43583@web.de","subject":"Re: free and errno, was Re: [PATCH] apply: replace mksnpath() with a mkpathdup() call","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-24T01:11:37Z","receivedAt":"2024-04-24T01:11:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 14, 2024 at 05:17:23PM +0200, René Scharfe wrote:\n\n> > In other words, it really depends on the contract of foo() with respect\n> > to errno. And I don't think there is a way in C to specify that\n> > contract in away that the compiler can understand.\n> \n> The context attribute of sparse could be used, I guess.  We'd have to\n> duplicate the declaration of all library functions, though, to tag them\n> with a may_have_changed_errno attribute.  And we'd need to clear it on\n> success, so we'd have to modify all callers.  On the flip side it would\n> allow detecting consecutive errno changes, e.g. by two I/O functions\n> with no error checking in between.  But the implementation effort seems\n> way too high.\n> \n> It would be easier if the error information was in one place instead of\n> one bit in the return value (NULL or less than 0) and the rest in errno.\n\nYeah. In a world where every failing function returned the equivalent of\n-errno, you could just do:\n\n  ret = xread(fd, ...); /* actually returns -errno! */\n  if (ret < 0) {\n\tclose(fd);\n\treturn ret; /* open's errno preserved via ret */\n  }\n\nWe could live in that world if we wrapped all of the syscalls with\nxread(), etc, that behaved that way. I don't know if it would run into\nweird gotchas, though, or if the result would simply look foreign to\nmost POSIX/C programmers.\n\nI also wonder if it might end up having the same 1% overhead that the\nfree() checks did, as it involves more juggling of stack values.\n\n> >> An xfree() to go along with xmalloc()/xrealloc()/xcalloc()/xstrdup() would\n> >> fit in nicely and might be easier to remember than free() after a while.\n> >> Having to convert thousands of calls is unappealing, though.\n> >\n> > My biggest concern with it is that we'd have to remember to use it, and\n> > there's not good compiler enforcement. But I guess coccinelle can help\n> > us there.q\n> \n> Yes, converting all free(3) calls is easy with Coccinelle, and the same\n> semantic patch can be used to enforce the use of xfree().  Still the\n> initial diff would be huge (2000+ changed lines in 300+ files).\n\nTrue. It's one more weird thing to remember, but for the most part we\nare there already with xmalloc().\n\n> > My secondary concern is that it might make people think that xmalloc()\n> > and xfree() are always paired, and thus you can do clever things in one\n> > as long as the other matches it. But we sometimes free memory from\n> > system routines like getline(). Maybe a scary comment would be enough?\n> \n> Amazing foresight!  Currently we only use getline(3) (which can act like\n> realloc(3)) in contrib/credential/, though.  Any others?\n\nHeh, I actually meant to say getdelim(), which obviously is closely\nrelated. There's only one call to it, but as it underlies\nstrbuf_getwholeline(), many strbufs will use it.\n\nAnd as you might have guessed, my \"amazing foresight\" came from being\nbitten by the same thing already. ;) Annoyed at how long git took to run\na large workload under massif, I once wrote a hacky peak-heap profiler\nthat wraps the system malloc(). Naturally it needs to match frees to\ntheir original allocations so we know how big each one was. Imagine my\nsurprise when I saw many frees without a matching allocation.\n\n> The \"x\" prefix doesn't promise exclusivity (hermetic seal?), but it\n> certainly suggests there are some shared feature between the allocator\n> functions and xfree(), while they only have in common that the are\n> wrapped.  We could call the latter errno_preserving_free() instead or so.\n\nThat's a mouthful, for sure. I think it is OK to suggest they are\nrelated as long as there is a comment in xfree() mentioning that we\nmight see pointers that didn't come from our x*() functions. That's\nwhere somebody would have to add code that violates the assumption.\n\n> I'm more worried about the overhead.  For a 0-1% slowdown (in the case of\n> git log) git_free() or xfree() give us accurate error numbers on all\n> platforms.  Non-misleading error codes are worth a lot (seen my share of\n> cursed messages), but xfree() is only necessary in the error handling\n> code.  The happy path can call free(3) directly without harm.\n> \n> But how to classify call sites accordingly?  It's easy for programmers\n> once they know it's necessary.  Is there a way in C, macro or Coccinelle\n> to have our cake and only eat it if really needed?  I don't see any. :-|\n\nI don't see a way either. I'd be willing to consider the 0-1% slowdown\nif it buys us simplicity, though. I'm also not even sure it's real.\nThere seemed to be a fairly large variation in results, and switching my\npowersaving functions around got me cases where the new code was\nactually faster. If we think there's a real benefit, we should make sure\nthe costs aren't just illusory.\n\n-Peff\n"}]}