{"thread":{"id":"52576","subject":"[PATCH] run-command: avoid undefined behavior in exists_in_PATH","startedAt":"2020-01-07T01:36:56Z","lastAt":"2020-01-08T10:29:06Z","messageCount":11,"participants":["brian m. carlson","Jonathan Nieder","Bryan Turner","Jeff King","Junio C Hamano","Miriam R.","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"389310","messageId":"20200107013640.1821227-1-sandals@crustytoothpaste.net","threadId":"52576","inReplyTo":null,"subject":"[PATCH] run-command: avoid undefined behavior in exists_in_PATH","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-01-07T01:36:40Z","receivedAt":"2020-01-07T01:36:56Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"In this function, we free the pointer we get from locate_in_PATH and\nthen check whether it's NULL. However, this is undefined behavior if\nthe pointer is non-NULL, since the C standard no longer permits us to\nuse a valid pointer after freeing it.\n\nThe only case in which the C standard would permit this to be defined\nbehavior is if r were NULL, since it states that in such a case \"no\naction occurs\" as a result of calling free.\n\nIt's easy to suggest that this is not likely to be a problem, but we\nknow that GCC does aggressively exploit the fact that undefined\nbehavior can never occur to optimize and rewrite code, even when that's\ncontrary to the expectations of the programmer. It is, in fact, very\ncommon for it to omit NULL pointer checks, just as we have here.\n\nSince it's easy to fix, let's do so, and avoid a potential headache in\nthe future.\n\nNoticed-by: Miriam R. \nSigned-off-by: brian m. carlson \n---\n run-command.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 9942f120a9..f5e1149f9b 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -213,8 +213,9 @@ static char *locate_in_PATH(const char *file)\n static int exists_in_PATH(const char *file)\n {\n \tchar *r = locate_in_PATH(file);\n+\tint found = r != NULL;\n \tfree(r);\n-\treturn r != NULL;\n+\treturn found;\n }\n \n int sane_execvp(const char *file, char * const argv[])\n"},{"id":"389313","messageId":"20200107020425.GG92456@google.com","threadId":"52576","inReplyTo":"20200107013640.1821227-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-01-07T02:04:25Z","receivedAt":"2020-01-07T02:04:30Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"brian m. carlson wrote:\n\n> In this function, we free the pointer we get from locate_in_PATH and\n> then check whether it's NULL. However, this is undefined behavior if\n> the pointer is non-NULL, since the C standard no longer permits us to\n> use a valid pointer after freeing it.\n[...]\n> Noticed-by: Miriam R. \n> Signed-off-by: brian m. carlson \n> ---\n> run-command.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n\nReviewed-by: Jonathan Nieder \n\nThis API that forces the caller to deal with the allocated result when\nthey never asked for it seems a bit inconvenient. Should we clean it up\na little? Something like this (on top):\n\n-- >8 --\nSubject: run-command: let caller pass in buffer to locate_in_PATH\n\nInstead of returning a buffer that the caller is responsible for\nfreeing, use a strbuf output parameter to record the path to the\nsearched-for program.\n\nThis makes ownership a little easier to reason about, since the owning\ncode declares the buffer. It's a good habit to follow because it\nallows buffer reuse when calling such a function in a loop.\n\nIt also allows the caller exists_in_PATH that does not care about the\npath to the command to be slightly simplified, by allowing a NULL\noutput parameter that means that locate_in_PATH should take care of\nallocating and freeing its temporary buffer.\n\nSigned-off-by: Jonathan Nieder \n---\n run-command.c | 51 +++++++++++++++++++++++++++++----------------------\n 1 file changed, 29 insertions(+), 22 deletions(-)\n\ndiff --git i/run-command.c w/run-command.c\nindex f5e1149f9b..a6dc38396a 100644\n--- i/run-command.c\n+++ w/run-command.c\n@@ -170,52 +170,57 @@ int is_executable(const char *name)\n * The caller should ensure that file contains no directory\n * separators.\n *\n- * Returns the path to the command, as found in $PATH or NULL if the\n- * command could not be found. The caller inherits ownership of the memory\n- * used to store the resultant path.\n+ * Returns a boolean indicating whether the command was found in $PATH.\n+ * The path to the command is recorded in the strbuf 'out', if supplied.\n *\n * This should not be used on Windows, where the $PATH search rules\n * are more complicated (e.g., a search for \"foo\" should find\n * \"foo.exe\").\n */\n-static char *locate_in_PATH(const char *file)\n+static int locate_in_PATH(const char *file, struct strbuf *out)\n {\n \tconst char *p = getenv(\"PATH\");\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tif (!p || !*p)\n-\t\treturn NULL;\n+\tif (!out)\n+\t\tout = &buf;\n+\n+\tif (!p || !*p) {\n+\t\tstrbuf_reset(out);\n+\t\tstrbuf_release(&buf);\n+\t\treturn 0;\n+\t}\n \n \twhile (1) {\n \t\tconst char *end = strchrnul(p, ':');\n \n-\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_reset(out);\n \n \t\t/* POSIX specifies an empty entry as the current directory. */\n \t\tif (end != p) {\n-\t\t\tstrbuf_add(&buf, p, end - p);\n-\t\t\tstrbuf_addch(&buf, '/');\n+\t\t\tstrbuf_add(out, p, end - p);\n+\t\t\tstrbuf_addch(out, '/');\n \t\t}\n-\t\tstrbuf_addstr(&buf, file);\n+\t\tstrbuf_addstr(out, file);\n \n-\t\tif (is_executable(buf.buf))\n-\t\t\treturn strbuf_detach(&buf, NULL);\n+\t\tif (is_executable(out->buf)) {\n+\t\t\tstrbuf_release(&buf);\n+\t\t\treturn 1;\n+\t\t}\n \n \t\tif (!*end)\n \t\t\tbreak;\n \t\tp = end + 1;\n \t}\n \n+\tstrbuf_reset(out);\n \tstrbuf_release(&buf);\n-\treturn NULL;\n+\treturn 0;\n }\n \n static int exists_in_PATH(const char *file)\n {\n-\tchar *r = locate_in_PATH(file);\n-\tint found = r != NULL;\n-\tfree(r);\n-\treturn found;\n+\treturn locate_in_PATH(file, NULL);\n }\n \n int sane_execvp(const char *file, char * const argv[])\n@@ -427,15 +432,17 @@ static int prepare_cmd(struct argv_array *out, const struct child_process *cmd)\n \t * directly.\n \t */\n \tif (!strchr(out->argv[1], '/')) {\n-\t\tchar *program = locate_in_PATH(out->argv[1]);\n-\t\tif (program) {\n-\t\t\tfree((char *)out->argv[1]);\n-\t\t\tout->argv[1] = program;\n-\t\t} else {\n+\t\tstruct strbuf program = STRBUF_INIT;\n+\n+\t\tif (!locate_in_PATH(out->argv[1], &program)) {\n \t\t\targv_array_clear(out);\n+\t\t\tstrbuf_release(&program);\n \t\t\terrno = ENOENT;\n \t\t\treturn -1;\n \t\t}\n+\n+\t\tfree((char *)out->argv[1]);\n+\t\tout->argv[1] = strbuf_detach(&program, NULL);\n \t}\n \n \treturn 0;\n"},{"id":"389314","messageId":"20200107021611.GK6570@camp.crustytoothpaste.net","threadId":"52576","inReplyTo":"20200107020425.GG92456@google.com","subject":"Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-01-07T02:16:11Z","receivedAt":"2020-01-07T02:16:18Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-01-07 at 02:04:25, Jonathan Nieder wrote:\n> brian m. carlson wrote:\n> \n> > In this function, we free the pointer we get from locate_in_PATH and\n> > then check whether it's NULL. However, this is undefined behavior if\n> > the pointer is non-NULL, since the C standard no longer permits us to\n> > use a valid pointer after freeing it.\n> [...]\n> > Noticed-by: Miriam R. \n> > Signed-off-by: brian m. carlson \n> > ---\n> > run-command.c | 3 ++-\n> > 1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> Reviewed-by: Jonathan Nieder \n> \n> This API that forces the caller to deal with the allocated result when\n> they never asked for it seems a bit inconvenient. Should we clean it up\n> a little? Something like this (on top):\n> \n> -- >8 --\n> Subject: run-command: let caller pass in buffer to locate_in_PATH\n> \n> Instead of returning a buffer that the caller is responsible for\n> freeing, use a strbuf output parameter to record the path to the\n> searched-for program.\n> \n> This makes ownership a little easier to reason about, since the owning\n> code declares the buffer. It's a good habit to follow because it\n> allows buffer reuse when calling such a function in a loop.\n> \n> It also allows the caller exists_in_PATH that does not care about the\n> path to the command to be slightly simplified, by allowing a NULL\n> output parameter that means that locate_in_PATH should take care of\n> allocating and freeing its temporary buffer.\n\nSure, I think this is a nice improvement. The user can reuse the buffer\nin a loop if they want by resetting it, which as you point out would be\nconvenient (and potentially more efficient). And we're already using\none internally, so there's no reason not to just pass one in.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"389316","messageId":"CAGyf7-GpCSaBauuKunDxBcZbcHU2sYkrt3+BpdF20dN8ZuPZgw@mail.gmail.com","threadId":"52576","inReplyTo":"20200107020425.GG92456@google.com","subject":"Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2020-01-07T03:40:13Z","receivedAt":"2020-01-07T03:40:27Z","isPatch":true,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Mon, Jan 6, 2020 at 6:04 PM Jonathan Nieder wrote:\n>\n> brian m. carlson wrote:\n>\n> > In this function, we free the pointer we get from locate_in_PATH and\n> > then check whether it's NULL. However, this is undefined behavior if\n> > the pointer is non-NULL, since the C standard no longer permits us to\n> > use a valid pointer after freeing it.\n> [...]\n> > Noticed-by: Miriam R. \n> > Signed-off-by: brian m. carlson \n> > ---\n> > run-command.c | 3 ++-\n> > 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> Reviewed-by: Jonathan Nieder \n>\n> This API that forces the caller to deal with the allocated result when\n> they never asked for it seems a bit inconvenient. Should we clean it up\n> a little? Something like this (on top):\n>\n> -- >8 --\n> Subject: run-command: let caller pass in buffer to locate_in_PATH\n>\n> Instead of returning a buffer that the caller is responsible for\n> freeing, use a strbuf output parameter to record the path to the\n> searched-for program.\n>\n> This makes ownership a little easier to reason about, since the owning\n> code declares the buffer. It's a good habit to follow because it\n> allows buffer reuse when calling such a function in a loop.\n>\n> It also allows the caller exists_in_PATH that does not care about the\n> path to the command to be slightly simplified, by allowing a NULL\n> output parameter that means that locate_in_PATH should take care of\n> allocating and freeing its temporary buffer.\n>\n> Signed-off-by: Jonathan Nieder \n> ---\n> run-command.c | 51 +++++++++++++++++++++++++++++----------------------\n> 1 file changed, 29 insertions(+), 22 deletions(-)\n>\n> diff --git i/run-command.c w/run-command.c\n> index f5e1149f9b..a6dc38396a 100644\n> --- i/run-command.c\n> +++ w/run-command.c\n> @@ -170,52 +170,57 @@ int is_executable(const char *name)\n> * The caller should ensure that file contains no directory\n> * separators.\n> *\n> - * Returns the path to the command, as found in $PATH or NULL if the\n> - * command could not be found. The caller inherits ownership of the memory\n> - * used to store the resultant path.\n> + * Returns a boolean indicating whether the command was found in $PATH.\n> + * The path to the command is recorded in the strbuf 'out', if supplied.\n> *\n> * This should not be used on Windows, where the $PATH search rules\n> * are more complicated (e.g., a search for \"foo\" should find\n> * \"foo.exe\").\n> */\n> -static char *locate_in_PATH(const char *file)\n> +static int locate_in_PATH(const char *file, struct strbuf *out)\n> {\n> const char *p = getenv(\"PATH\");\n> struct strbuf buf = STRBUF_INIT;\n>\n> - if (!p || !*p)\n> - return NULL;\n> + if (!out)\n> + out = &buf;\n> +\n> + if (!p || !*p) {\n> + strbuf_reset(out);\n\nSince the while loop and this block both call strbuf_reset(out);, is\nthere a reason not to do:\nif (out)\n strbuf_reset(out);\n} else {\n out = &buf;\n}\nabove? The loop below would still need its own reset, but it could do\nthat when it decides to loop.\n\n> + strbuf_release(&buf);\n> + return 0;\n> + }\n>\n> while (1) {\n> const char *end = strchrnul(p, ':');\n>\n> - strbuf_reset(&buf);\n> + strbuf_reset(out);\n\nThis reset would be removed\n\n>\n> /* POSIX specifies an empty entry as the current directory. */\n> if (end != p) {\n> - strbuf_add(&buf, p, end - p);\n> - strbuf_addch(&buf, '/');\n> + strbuf_add(out, p, end - p);\n> + strbuf_addch(out, '/');\n> }\n> - strbuf_addstr(&buf, file);\n> + strbuf_addstr(out, file);\n>\n> - if (is_executable(buf.buf))\n> - return strbuf_detach(&buf, NULL);\n> + if (is_executable(out->buf)) {\n> + strbuf_release(&buf);\n> + return 1;\n> + }\n>\n\nWe'd call strbuf_reset(out); here instead, before we break or loop.\n\n> if (!*end)\n> break;\n> p = end + 1\n> }\n>\n> + strbuf_reset(out);\n\nAnd we could leave this one out; if we make it here, we'd know the\nbuffer was already reset.\n\n> strbuf_release(&buf);\n> - return NULL;\n> + return 0;\n> }\n>\n> static int exists_in_PATH(const char *file)\n> {\n> - char *r = locate_in_PATH(file);\n> - int found = r != NULL;\n> - free(r);\n> - return found;\n> + return locate_in_PATH(file, NULL);\n> }\n>\n> int sane_execvp(const char *file, char * const argv[])\n> @@ -427,15 +432,17 @@ static int prepare_cmd(struct argv_array *out, const struct child_process *cmd)\n> * directly.\n> */\n> if (!strchr(out->argv[1], '/')) {\n> - char *program = locate_in_PATH(out->argv[1]);\n> - if (program) {\n> - free((char *)out->argv[1]);\n> - out->argv[1] = program;\n> - } else {\n> + struct strbuf program = STRBUF_INIT;\n> +\n> + if (!locate_in_PATH(out->argv[1], &program)) {\n> argv_array_clear(out);\n> + strbuf_release(&program);\n> errno = ENOENT;\n> return -1;\n> }\n> +\n> + free((char *)out->argv[1]);\n> + out->argv[1] = strbuf_detach(&program, NULL);\n> }\n>\n> return 0;\n\nJust a thought. (Pardon the noise from the peanut gallery!)\n\nBest regards,\nBryan Turner\n"},{"id":"389317","messageId":"CAGyf7-G_YKtyKgAA+LLqbvh2Tuq+qmVVH=yz9=kfC5uWXHMr_Q@mail.gmail.com","threadId":"52576","inReplyTo":"CAGyf7-GpCSaBauuKunDxBcZbcHU2sYkrt3+BpdF20dN8ZuPZgw@mail.gmail.com","subject":"Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2020-01-07T03:41:30Z","receivedAt":"2020-01-07T03:41:43Z","isPatch":true,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Mon, Jan 6, 2020 at 7:40 PM Bryan Turner wrote:\n>\n> On Mon, Jan 6, 2020 at 6:04 PM Jonathan Nieder wrote:\n> >\n> > brian m. carlson wrote:\n> >\n> > > In this function, we free the pointer we get from locate_in_PATH and\n> > > then check whether it's NULL. However, this is undefined behavior if\n> > > the pointer is non-NULL, since the C standard no longer permits us to\n> > > use a valid pointer after freeing it.\n> > [...]\n> > > Noticed-by: Miriam R. \n> > > Signed-off-by: brian m. carlson \n> > > ---\n> > > run-command.c | 3 ++-\n> > > 1 file changed, 2 insertions(+), 1 deletion(-)\n> >\n> > Reviewed-by: Jonathan Nieder \n> >\n> > This API that forces the caller to deal with the allocated result when\n> > they never asked for it seems a bit inconvenient. Should we clean it up\n> > a little? Something like this (on top):\n> >\n> > -- >8 --\n> > Subject: run-command: let caller pass in buffer to locate_in_PATH\n> >\n> > Instead of returning a buffer that the caller is responsible for\n> > freeing, use a strbuf output parameter to record the path to the\n> > searched-for program.\n> >\n> > This makes ownership a little easier to reason about, since the owning\n> > code declares the buffer. It's a good habit to follow because it\n> > allows buffer reuse when calling such a function in a loop.\n> >\n> > It also allows the caller exists_in_PATH that does not care about the\n> > path to the command to be slightly simplified, by allowing a NULL\n> > output parameter that means that locate_in_PATH should take care of\n> > allocating and freeing its temporary buffer.\n> >\n> > Signed-off-by: Jonathan Nieder \n> > ---\n> > run-command.c | 51 +++++++++++++++++++++++++++++----------------------\n> > 1 file changed, 29 insertions(+), 22 deletions(-)\n> >\n> > diff --git i/run-command.c w/run-command.c\n> > index f5e1149f9b..a6dc38396a 100644\n> > --- i/run-command.c\n> > +++ w/run-command.c\n> > @@ -170,52 +170,57 @@ int is_executable(const char *name)\n> > * The caller should ensure that file contains no directory\n> > * separators.\n> > *\n> > - * Returns the path to the command, as found in $PATH or NULL if the\n> > - * command could not be found. The caller inherits ownership of the memory\n> > - * used to store the resultant path.\n> > + * Returns a boolean indicating whether the command was found in $PATH.\n> > + * The path to the command is recorded in the strbuf 'out', if supplied.\n> > *\n> > * This should not be used on Windows, where the $PATH search rules\n> > * are more complicated (e.g., a search for \"foo\" should find\n> > * \"foo.exe\").\n> > */\n> > -static char *locate_in_PATH(const char *file)\n> > +static int locate_in_PATH(const char *file, struct strbuf *out)\n> > {\n> > const char *p = getenv(\"PATH\");\n> > struct strbuf buf = STRBUF_INIT;\n> >\n> > - if (!p || !*p)\n> > - return NULL;\n> > + if (!out)\n> > + out = &buf;\n> > +\n> > + if (!p || !*p) {\n> > + strbuf_reset(out);\n>\n> Since the while loop and this block both call strbuf_reset(out);, is\n> there a reason not to do:\n> if (out)\n> strbuf_reset(out);\n> } else {\n> out = &buf;\n> }\n> above? The loop below would still need its own reset, but it could do\n> that when it decides to loop.\n\nIgnore my incorrect braces; force of habit. I meant:\nif (out)\n strbuf_reset(out);\nelse\n out = &buf;\n\n>\n> > + strbuf_release(&buf);\n> > + return 0;\n> > + }\n> >\n> > while (1) {\n> > const char *end = strchrnul(p, ':');\n> >\n> > - strbuf_reset(&buf);\n> > + strbuf_reset(out);\n>\n> This reset would be removed\n>\n> >\n> > /* POSIX specifies an empty entry as the current directory. */\n> > if (end != p) {\n> > - strbuf_add(&buf, p, end - p);\n> > - strbuf_addch(&buf, '/');\n> > + strbuf_add(out, p, end - p);\n> > + strbuf_addch(out, '/');\n> > }\n> > - strbuf_addstr(&buf, file);\n> > + strbuf_addstr(out, file);\n> >\n> > - if (is_executable(buf.buf))\n> > - return strbuf_detach(&buf, NULL);\n> > + if (is_executable(out->buf)) {\n> > + strbuf_release(&buf);\n> > + return 1;\n> > + }\n> >\n>\n> We'd call strbuf_reset(out); here instead, before we break or loop.\n>\n> > if (!*end)\n> > break;\n> > p = end + 1\n> > }\n> >\n> > + strbuf_reset(out);\n>\n> And we could leave this one out; if we make it here, we'd know the\n> buffer was already reset.\n>\n> > strbuf_release(&buf);\n> > - return NULL;\n> > + return 0;\n> > }\n> >\n> > static int exists_in_PATH(const char *file)\n> > {\n> > - char *r = locate_in_PATH(file);\n> > - int found = r != NULL;\n> > - free(r);\n> > - return found;\n> > + return locate_in_PATH(file, NULL);\n> > }\n> >\n> > int sane_execvp(const char *file, char * const argv[])\n> > @@ -427,15 +432,17 @@ static int prepare_cmd(struct argv_array *out, const struct child_process *cmd)\n> > * directly.\n> > */\n> > if (!strchr(out->argv[1], '/')) {\n> > - char *program = locate_in_PATH(out->argv[1]);\n> > - if (program) {\n> > - free((char *)out->argv[1]);\n> > - out->argv[1] = program;\n> > - } else {\n> > + struct strbuf program = STRBUF_INIT;\n> > +\n> > + if (!locate_in_PATH(out->argv[1], &program)) {\n> > argv_array_clear(out);\n> > + strbuf_release(&program);\n> > errno = ENOENT;\n> > return -1;\n> > }\n> > +\n> > + free((char *)out->argv[1]);\n> > + out->argv[1] = strbuf_detach(&program, NULL);\n> > }\n> >\n> > return 0;\n>\n> Just a thought. (Pardon the noise from the peanut gallery!)\n>\n> Best regards,\n> Bryan Turner\n"},{"id":"389349","messageId":"20200107110145.GA1073219@coredump.intra.peff.net","threadId":"52576","inReplyTo":"20200107013640.1821227-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-07T11:01:45Z","receivedAt":"2020-01-07T11:01:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 07, 2020 at 01:36:40AM +0000, brian m. carlson wrote:\n\n> In this function, we free the pointer we get from locate_in_PATH and\n> then check whether it's NULL. However, this is undefined behavior if\n> the pointer is non-NULL, since the C standard no longer permits us to\n> use a valid pointer after freeing it.\n> \n> The only case in which the C standard would permit this to be defined\n> behavior is if r were NULL, since it states that in such a case \"no\n> action occurs\" as a result of calling free.\n> \n> It's easy to suggest that this is not likely to be a problem, but we\n> know that GCC does aggressively exploit the fact that undefined\n> behavior can never occur to optimize and rewrite code, even when that's\n> contrary to the expectations of the programmer. It is, in fact, very\n> common for it to omit NULL pointer checks, just as we have here.\n\nOK, I agree it makes sense to be on the safe side here (and the patch is\nobviously the right fix).\n\n> Noticed-by: Miriam R. \n> Signed-off-by: brian m. carlson \n\nI think Miriam actually posted the same patch in her initial email:\n\n https://lore.kernel.org/git/CAN7CjDDBA0ZoCG9aaQf5rg3gxqny=EjR6v6jE1mnxvUJQSF_0Q@mail.gmail.com/\n\nI don't know how we want to handle authorship.\n\n-Peff\n"},{"id":"389350","messageId":"20200107110850.GB1073219@coredump.intra.peff.net","threadId":"52576","inReplyTo":"20200107020425.GG92456@google.com","subject":"Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-07T11:08:50Z","receivedAt":"2020-01-07T11:08:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 06, 2020 at 06:04:25PM -0800, Jonathan Nieder wrote:\n\n> -- >8 --\n> Subject: run-command: let caller pass in buffer to locate_in_PATH\n> \n> Instead of returning a buffer that the caller is responsible for\n> freeing, use a strbuf output parameter to record the path to the\n> searched-for program.\n> \n> This makes ownership a little easier to reason about, since the owning\n> code declares the buffer. It's a good habit to follow because it\n> allows buffer reuse when calling such a function in a loop.\n> \n> It also allows the caller exists_in_PATH that does not care about the\n> path to the command to be slightly simplified, by allowing a NULL\n> output parameter that means that locate_in_PATH should take care of\n> allocating and freeing its temporary buffer.\n>\n> Signed-off-by: Jonathan Nieder \n> ---\n> run-command.c | 51 +++++++++++++++++++++++++++++----------------------\n> 1 file changed, 29 insertions(+), 22 deletions(-)\n\nI dunno. Now the rules inside locate_in_PATH() are more complicated, and\nwe have an unusual boolean return from the function.\n\nI admit I don't overly care either way, as there are literally two\ncallers (and not likely to be more). So I'd probably just leave it\nalone, but I'm not opposed to the patch if people think it's a good\nidea.\n\n-Peff\n"},{"id":"389396","messageId":"xmqqzhez5hlq.fsf@gitster-ct.c.googlers.com","threadId":"52576","inReplyTo":"20200107110145.GA1073219@coredump.intra.peff.net","subject":"Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-07T16:58:09Z","receivedAt":"2020-01-07T16:58:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jan 07, 2020 at 01:36:40AM +0000, brian m. carlson wrote:\n>\n>> In this function, we free the pointer we get from locate_in_PATH and\n>> then check whether it's NULL. However, this is undefined behavior if\n>> the pointer is non-NULL, since the C standard no longer permits us to\n>> use a valid pointer after freeing it.\n>> \n>> The only case in which the C standard would permit this to be defined\n>> behavior is if r were NULL, since it states that in such a case \"no\n>> action occurs\" as a result of calling free.\n>> \n>> It's easy to suggest that this is not likely to be a problem, but we\n>> know that GCC does aggressively exploit the fact that undefined\n>> behavior can never occur to optimize and rewrite code, even when that's\n>> contrary to the expectations of the programmer. It is, in fact, very\n>> common for it to omit NULL pointer checks, just as we have here.\n>\n> OK, I agree it makes sense to be on the safe side here (and the patch is\n> obviously the right fix).\n>\n>> Noticed-by: Miriam R. \n>> Signed-off-by: brian m. carlson \n>\n> I think Miriam actually posted the same patch in her initial email:\n>\n> https://lore.kernel.org/git/CAN7CjDDBA0ZoCG9aaQf5rg3gxqny=EjR6v6jE1mnxvUJQSF_0Q@mail.gmail.com/\n>\n> I don't know how we want to handle authorship.\n\nI think the explanation in the log message has as much value as, if\nnot more than, the actual patch text, in this case. Noticed-by: may\nbe striking the right balance.\n\nThanks, all.\n"},{"id":"389444","messageId":"20200108024732.GL6570@camp.crustytoothpaste.net","threadId":"52576","inReplyTo":"20200107110145.GA1073219@coredump.intra.peff.net","subject":"Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-01-08T02:47:32Z","receivedAt":"2020-01-08T02:47:38Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-01-07 at 11:01:45, Jeff King wrote:\n> > Noticed-by: Miriam R. \n> > Signed-off-by: brian m. carlson \n> \n> I think Miriam actually posted the same patch in her initial email:\n> \n> https://lore.kernel.org/git/CAN7CjDDBA0ZoCG9aaQf5rg3gxqny=EjR6v6jE1mnxvUJQSF_0Q@mail.gmail.com/\n> \n> I don't know how we want to handle authorship.\n\nI don't feel strongly about holding authorship; I'm happy to have her\nname on it instead of mine since she did propose a solution. I just\ncare that we fix it.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"389458","messageId":"CAN7CjDCv2uYNaHEtQv6Zco33-Cba1Fn2by87NU_3vwytZHy_WA@mail.gmail.com","threadId":"52576","inReplyTo":"20200108024732.GL6570@camp.crustytoothpaste.net","subject":"Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-08T09:15:37Z","receivedAt":"2020-01-08T09:15:49Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"El mié., 8 ene. 2020 a las 3:47, brian m. carlson\n() escribió:\n>\n> On 2020-01-07 at 11:01:45, Jeff King wrote:\n> > > Noticed-by: Miriam R. \n> > > Signed-off-by: brian m. carlson \n> >\n> > I think Miriam actually posted the same patch in her initial email:\n> >\n> > https://lore.kernel.org/git/CAN7CjDDBA0ZoCG9aaQf5rg3gxqny=EjR6v6jE1mnxvUJQSF_0Q@mail.gmail.com/\n> >\n> > I don't know how we want to handle authorship.\n>\n> I don't feel strongly about holding authorship; I'm happy to have her\n> name on it instead of mine since she did propose a solution. I just\n> care that we fix it.\n> --\nHi,\nmy mentor (Christian Couder ) was who\ndetected the problem and sent me the solution, I only asked the\ncommunity. I think his name should be in the patch instead of mine.\n\nBest,\nMiriam\n> brian m. carlson: Houston, Texas, US\n> OpenPGP: https://keybase.io/bk2204\n"},{"id":"389463","messageId":"CAP8UFD246PpsysJd8s37g+BjSu4NNerW_ozEMtkP9iGCmUbupQ@mail.gmail.com","threadId":"52576","inReplyTo":"CAN7CjDCv2uYNaHEtQv6Zco33-Cba1Fn2by87NU_3vwytZHy_WA@mail.gmail.com","subject":"Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-01-08T10:28:51Z","receivedAt":"2020-01-08T10:29:06Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jan 8, 2020 at 10:16 AM Miriam R. wrote:\n>\n> El mié., 8 ene. 2020 a las 3:47, brian m. carlson\n> () escribió:\n\n> > > I think Miriam actually posted the same patch in her initial email:\n> > >\n> > > https://lore.kernel.org/git/CAN7CjDDBA0ZoCG9aaQf5rg3gxqny=EjR6v6jE1mnxvUJQSF_0Q@mail.gmail.com/\n> > >\n> > > I don't know how we want to handle authorship.\n> >\n> > I don't feel strongly about holding authorship; I'm happy to have her\n> > name on it instead of mine since she did propose a solution. I just\n> > care that we fix it.\n\n> my mentor (Christian Couder ) was who\n> detected the problem and sent me the solution, I only asked the\n> community. I think his name should be in the patch instead of mine.\n\nI am ok with not having my name anywhere and leaving the patch as is.\n"}]}