From: Stefan Sperling <stsp@stsp.name>
Subject: Re: Got relies on non-POSIX basename(3), dirname(3)
To: Christian Weisgerber <naddy@mips.inka.de>
Cc: gameoftrees@openbsd.org
Date: Fri, 18 Sep 2020 20:02:50 +0200

On Fri, Sep 18, 2020 at 05:44:43PM -0000, Christian Weisgerber wrote:
> On 2020-09-18, Stefan Sperling <stsp@stsp.name> wrote:
> 
> > Could you add basename/dirname overrides to the freebsd version which
> > copy the string internally before use?
> 
> I didn't want to worry about strdup() error handling, so I just
> took the OpenBSD basename.c and dirname.c.

Fair enough.

> > Otherwise I don't think we could do much else but go through the basename
> > and dirname calls one-by-one to determine whether they rely on the string
> > to remain unmodified. Where a modifiable string is required, an easy fix
> > would be to run strdup() first, I guess?
> 
> Sooner or later this is going to rear its head when the prototypes
> get fixed and compiler warnings about the loss of const show up...

So the long-term plan is to adjust OpenBSD's basename?

The only potential problem I see is that hot code paths such as the
tree object parser could end up calling malloc/free more often, which
could hurt performance on OpenBSD. We could use fixed-size buffers on
the stack where that matters.

Using the first match found via grep as one example, in the code below
both arguments to basename are re-used later on and the code expects
these strings to retain their original content. So we must call strdup()
before handing either of them to basename(3).

This also plugs a memory leak ('cwd') in error cases.

ok?

diff 5874ea87557e324b6e847a5dbed5a1b4b9efe92e /home/stsp/src/got
blob - fff8cb02da0feb500622e5a857ec563f0947bc8d
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -2408,6 +2408,7 @@ cmd_checkout(int argc, char *argv[])
 	const char *path_prefix = "";
 	const char *branch_name = GOT_REF_HEAD;
 	char *commit_id_str = NULL;
+	char *cwd = NULL, *path = NULL;
 	int ch, same_path_prefix, allow_nonempty = 0;
 	struct got_pathlist_head paths;
 	struct got_checkout_progress_arg cpa;
@@ -2445,7 +2446,7 @@ cmd_checkout(int argc, char *argv[])
 		err(1, "pledge");
 #endif
 	if (argc == 1) {
-		char *cwd, *base, *dotgit;
+		char *base, *dotgit;
 		repo_path = realpath(argv[0], NULL);
 		if (repo_path == NULL)
 			return got_error_from_errno2("realpath", argv[0]);
@@ -2454,30 +2455,26 @@ cmd_checkout(int argc, char *argv[])
 			error = got_error_from_errno("getcwd");
 			goto done;
 		}
-		if (path_prefix[0]) {
-			base = basename(path_prefix);
-			if (base == NULL) {
-				error = got_error_from_errno2("basename",
-				    path_prefix);
-				goto done;
-			}
-		} else {
-			base = basename(repo_path);
-			if (base == NULL) {
-				error = got_error_from_errno2("basename",
-				    repo_path);
-				goto done;
-			}
+		if (path_prefix[0])
+			path = strdup(path_prefix);
+		else
+			path = strdup(repo_path);
+		if (path == NULL) {
+			error = got_error_from_errno("strdup");
+			goto done;
 		}
+		base = basename(path);
+		if (base == NULL) {
+			error = got_error_from_errno2("basename", path);
+			goto done;
+		}
 		dotgit = strstr(base, ".git");
 		if (dotgit)
 			*dotgit = '\0';
 		if (asprintf(&worktree_path, "%s/%s", cwd, base) == -1) {
 			error = got_error_from_errno("asprintf");
-			free(cwd);
 			goto done;
 		}
-		free(cwd);
 	} else if (argc == 2) {
 		repo_path = realpath(argv[0], NULL);
 		if (repo_path == NULL) {
@@ -2595,6 +2592,8 @@ done:
 	free(commit_id_str);
 	free(repo_path);
 	free(worktree_path);
+	free(cwd);
+	free(path);
 	return error;
 }