From: Omar Polo <op@omarpolo.com>
Subject: Re: gotwebd: don't use NULL pointer in *printf(3) calls
To: Mark Jamsek <mark@jamsek.com>
Cc: gameoftrees@openbsd.org
Date: Sun, 24 Nov 2024 15:09:41 +0100

On 2024/11/24 16:47:47 +1100, Mark Jamsek <mark@jamsek.com> wrote:
> The below diff avoids some undefined behaviour where we pass a NULL
> pointer to a series of printf(3) functions when the query string to
> gotwebd does not contain a path parameter. This was found thanks to
> a report from Mischa shared by stsp on IRC.
> 
> 
> commit 6c3c9861fa886ed16cba03b9d9df4744979dc300
> from: Mark Jamsek <mark@jamsek.dev>
> date: Sun Nov 24 05:14:25 2024 UTC
> 
> gotwebd: fix UB when path param is not in query
> 
> Don't pass NULL as a *printf(3) %s conversion specifier argument.
> If the path parameter is not defined, return repo not found error.

I'd prefer to bubble up the error checking.  The only way for dir to be
NULL in gotweb_load_got_path() is when the page is not INDEX and
qs->path was not given.  The other call uses the result of readdir().

ok?


diff /home/op/w/got
commit - 73675c3a673715141c3872d1f25eafaffb1ac0a5
path + /home/op/w/got
blob - 8cb8b818f4c6074b02022a654a94701232e3456b
file + gotwebd/gotweb.c
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -195,6 +195,11 @@ gotweb_process_request(struct request *c)
 	}
 
 	if (qs->action != INDEX) {
+		if (qs->path == NULL) {
+			error = got_error(GOT_ERR_BAD_QUERYSTRING);
+			goto err;
+		}
+
 		error = gotweb_load_got_path(&repo_dir, qs->path, c);
 		c->t->repo_dir = repo_dir;
 		if (error)
@@ -1069,9 +1074,6 @@ gotweb_load_got_path(struct repo_dir **rp, const char 
 	DIR *dt;
 	char *dir_test;
 
-	if (dir == NULL)
-		return got_error(GOT_ERR_NOT_GIT_REPO);
-
 	*rp = calloc(1, sizeof(**rp));
 	if (*rp == NULL)
 		return got_error_from_errno("calloc");