Omar Polo <op@omarpolo.com> wrote:
> 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?

Yes, makes sense. This is better.

ok for me! Thanks, op :)

> 
> 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");


-- 
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68