From: Tracey Emery <tracey@traceyemery.net>
Subject: Re: gotwebd: drop needless qs NULL check
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Thu, 22 Sep 2022 07:41:11 -0600

On Wed, Sep 21, 2022 at 08:00:07PM +0200, Omar Polo wrote:
> spotted while doing something else.  If qs is NULL we would have
> already crashed and a couple of lines before we do dereference it.
> 
> Every time we hit gotweb_render_header we have already gone thru a
> (successfull) gotweb_init_querystring, otherwise we would have `goto
> err' in gotweb_process_request.
> 
> got diff -w with extra context for ease of review:
> 
> 	--- gotwebd/gotweb.c
> 	+++ gotwebd/gotweb.c
> 	@@ -740,11 +740,10 @@ gotweb_render_header(struct request *c)
> 	 	    c->script_name, srv->logo,
> 	 	    qs->index_page, srv->site_link);
> 	 	if (r == -1)
> 	 		goto done;
> 	 
> 	-	if (qs != NULL) {
> 	 		if (qs->path != NULL) {
> 	 			char *epath;
> 	 
> 	 			if (fcgi_printf(c, " / ") == -1)
> 	 				goto done;
> 	@@ -793,11 +792,10 @@ gotweb_render_header(struct request *c)
> 	 			}
> 	 
> 	 			if (fcgi_printf(c, " / %s", action) == -1)
> 	 				goto done;
> 	 		}
> 	-	}
> 	 
> 	 	fcgi_printf(c, "</div>\n"	/* #site_path */
> 	 	    "</div>\n"			/* #site_link */
> 	 	    "<div id='content'>\n");
> 
> 
> and the real diff for got patch :)
> 
> (admittedly only build-tested.)

Yes, ok.

> 
> diff /home/op/w/gwd-template
> commit - 611e5fc2074d428e17f920dc595496af4dd0dc77
> path + /home/op/w/gwd-template
> blob - aafa0224a0f81173ac396f4607674dc80127339a
> file + gotwebd/gotweb.c
> --- gotwebd/gotweb.c
> +++ gotwebd/gotweb.c
> @@ -742,59 +742,57 @@ gotweb_render_header(struct request *c)
>  	if (r == -1)
>  		goto done;
>  
> -	if (qs != NULL) {
> -		if (qs->path != NULL) {
> -			char *epath;
> +	if (qs->path != NULL) {
> +		char *epath;
>  
> -			if (fcgi_printf(c, " / ") == -1)
> -				goto done;
> +		if (fcgi_printf(c, " / ") == -1)
> +			goto done;
>  
> -			err = gotweb_escape_html(&epath, qs->path);
> -			if (err)
> -				return err;
> -			r = gotweb_link(c, &(struct gotweb_url){
> -				.action = SUMMARY,
> -				.index_page = -1,
> -				.page = -1,
> -				.path = qs->path,
> -			    }, "%s", epath);
> -			free(epath);
> -			if (r == -1)
> -				goto done;
> -		}
> -		if (qs->action != INDEX) {
> -			const char *action = "";
> +		err = gotweb_escape_html(&epath, qs->path);
> +		if (err)
> +			return err;
> +		r = gotweb_link(c, &(struct gotweb_url){
> +			    .action = SUMMARY,
> +			    .index_page = -1,
> +			    .page = -1,
> +			    .path = qs->path,
> +		    }, "%s", epath);
> +		free(epath);
> +		if (r == -1)
> +			goto done;
> +	}
> +	if (qs->action != INDEX) {
> +		const char *action = "";
>  
> -			switch (qs->action) {
> -			case BLAME:
> -				action = "blame";
> -				break;
> -			case BRIEFS:
> -				action = "briefs";
> -				break;
> -			case COMMITS:
> -				action = "commits";
> -				break;
> -			case DIFF:
> -				action = "diff";
> -				break;
> -			case SUMMARY:
> -				action = "summary";
> -				break;
> -			case TAG:
> -				action = "tag";
> -				break;
> -			case TAGS:
> -				action = "tags";
> -				break;
> -			case TREE:
> -				action = "tree";
> -				break;
> -			}
> -
> -			if (fcgi_printf(c, " / %s", action) == -1)
> -				goto done;
> +		switch (qs->action) {
> +		case BLAME:
> +			action = "blame";
> +			break;
> +		case BRIEFS:
> +			action = "briefs";
> +			break;
> +		case COMMITS:
> +			action = "commits";
> +			break;
> +		case DIFF:
> +			action = "diff";
> +			break;
> +		case SUMMARY:
> +			action = "summary";
> +			break;
> +		case TAG:
> +			action = "tag";
> +			break;
> +		case TAGS:
> +			action = "tags";
> +			break;
> +		case TREE:
> +			action = "tree";
> +			break;
>  		}
> +
> +		if (fcgi_printf(c, " / %s", action) == -1)
> +			goto done;
>  	}
>  
>  	fcgi_printf(c, "</div>\n"	/* #site_path */
> 

-- 

Tracey Emery