Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jan 7, 2020, 02:04 UTC
Message-ID: <20200107020425.GG92456@google.com>
In-Reply-To: <20200107013640.1821227-1-sandals@crustytoothpaste.net>

brian m. carlson wrote:

> In this function, we free the pointer we get from locate_in_PATH and
> then check whether it's NULL.  However, this is undefined behavior if
> the pointer is non-NULL, since the C standard no longer permits us to
> use a valid pointer after freeing it.

[...]

Show 5 quoted lines

> Noticed-by: Miriam R. <mirucam@gmail.com>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  run-command.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

This API that forces the caller to deal with the allocated result when they never asked for it seems a bit inconvenient. Should we clean it up a little? Something like this (on top):

-- >8 --
Subject: run-command: let caller pass in buffer to locate_in_PATH

Instead of returning a buffer that the caller is responsible for freeing, use a strbuf output parameter to record the path to the searched-for program.

This makes ownership a little easier to reason about, since the owning code declares the buffer. It's a good habit to follow because it allows buffer reuse when calling such a function in a loop.

It also allows the caller exists_in_PATH that does not care about the path to the command to be slightly simplified, by allowing a NULL output parameter that means that locate_in_PATH should take care of allocating and freeing its temporary buffer.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 run-command.c | 51 +++++++++++++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 22 deletions(-)

diff --git i/run-command.c w/run-command.c
index f5e1149f9b..a6dc38396a 100644
--- i/run-command.c
+++ w/run-command.c
@@ -170,52 +170,57 @@ int is_executable(const char *name)
  * The caller should ensure that file contains no directory
  * separators.
  *
- * Returns the path to the command, as found in $PATH or NULL if the
- * command could not be found.  The caller inherits ownership of the memory
- * used to store the resultant path.
+ * Returns a boolean indicating whether the command was found in $PATH.
+ * The path to the command is recorded in the strbuf 'out', if supplied.
  *
  * This should not be used on Windows, where the $PATH search rules
  * are more complicated (e.g., a search for "foo" should find
  * "foo.exe").
  */
-static char *locate_in_PATH(const char *file)
+static int locate_in_PATH(const char *file, struct strbuf *out)
 {
 	const char *p = getenv("PATH");
 	struct strbuf buf = STRBUF_INIT;
 
-	if (!p || !*p)
-		return NULL;
+	if (!out)
+		out = &buf;
+
+	if (!p || !*p) {
+		strbuf_reset(out);
+		strbuf_release(&buf);
+		return 0;
+	}
 
 	while (1) {
 		const char *end = strchrnul(p, ':');
 
-		strbuf_reset(&buf);
+		strbuf_reset(out);
 
 		/* POSIX specifies an empty entry as the current directory. */
 		if (end != p) {
-			strbuf_add(&buf, p, end - p);
-			strbuf_addch(&buf, '/');
+			strbuf_add(out, p, end - p);
+			strbuf_addch(out, '/');
 		}
-		strbuf_addstr(&buf, file);
+		strbuf_addstr(out, file);
 
-		if (is_executable(buf.buf))
-			return strbuf_detach(&buf, NULL);
+		if (is_executable(out->buf)) {
+			strbuf_release(&buf);
+			return 1;
+		}
 
 		if (!*end)
 			break;
 		p = end + 1;
 	}
 
+	strbuf_reset(out);
 	strbuf_release(&buf);
-	return NULL;
+	return 0;
 }
 
 static int exists_in_PATH(const char *file)
 {
-	char *r = locate_in_PATH(file);
-	int found = r != NULL;
-	free(r);
-	return found;
+	return locate_in_PATH(file, NULL);
 }
 
 int sane_execvp(const char *file, char * const argv[])
@@ -427,15 +432,17 @@ static int prepare_cmd(struct argv_array *out, const struct child_process *cmd)
 	 * directly.
 	 */
 	if (!strchr(out->argv[1], '/')) {
-		char *program = locate_in_PATH(out->argv[1]);
-		if (program) {
-			free((char *)out->argv[1]);
-			out->argv[1] = program;
-		} else {
+		struct strbuf program = STRBUF_INIT;
+
+		if (!locate_in_PATH(out->argv[1], &program)) {
 			argv_array_clear(out);
+			strbuf_release(&program);
 			errno = ENOENT;
 			return -1;
 		}
+
+		free((char *)out->argv[1]);
+		out->argv[1] = strbuf_detach(&program, NULL);
 	}
 
 	return 0;