On Wed, Jul 27, 2022 at 11:13:04AM +0200, Omar Polo wrote:
> Hello,
> 
> when rendering a blob, gotwebd prints the Content-Type header twice.
> Since gotweb_render_content_type_file also adds the empty line to
> terminate the headers part of the HTTP reply, the second Content-Type is
> sent as part of the blob.
> 
> this arises from gotweb_process_request unconditionally setting the
> content type to text/plain while got_output_file_blob (called next)
> already does so.
> 
> in got_output_file_blob there is also a typo in the content type:
> unfortunately "text/text" doesn't seem to exist, so change it to
> "text/plain".
> 
> While here i couldn't help not to tweak to fcgi_gen{_binary,}_response,
> isbinary and to the last part of the loop in got_output_file_blob.
> 
> I've noticed that fcgi_gen_response and the _binary variant are
> practically the same function, with the exception that fcgi_gen_response
> implicitly calls strlen and quits if the length is zero.  So, why not
> dedup the code?
> 
> I'm also adding a check in fcgi_gen_binary_response to bail out if len
> is zero, because got_output_file_blob may call it with a zero length and
> the other function had this optimization anyway.
> 
> now, since fcgi_gen_binary_response and fcgi_gen_response are
> fundamentally the same function, i think we can simplify the last part
> of the loop in got_output_file_blob.
> 
> the change to isbinary is just because i thought it would read better
> with a memchr and because it's shorter, but i'm fine with dropping it if
> the explicit loop is preferred.
> 
>  ---
> 
> There is still another issue is gotwebd that I've found and still
> haven't tracked down.  it doesn't dump all the blob somehow.  Here's an
> example:
> 
> https://git.omarpolo.com/?index_page=&path=amused.git&action=blob&commit=3fc7366684dd5a216deed3c8d7c69c1792022d5c&folder=&file=amused.c
> 

Weird. Will look into this after you get this diff in. ok.

> the blob is always truncated at `main_send_player' and some gibberish is
> added at the end apparently.  I'm running gotwebd from the latest commit
> in main as of now plus below diff applied, but this bug was there even
> without my diff.
> 
>  ---
> 
> ok?
> 
> diff /home/op/w/got
> commit - 615e455c6bdddbd4e59d5d0dece41eb9953b6336
> path + /home/op/w/got
> blob - 1581cf938674fc59b27476119bfcc54b5b4417b5
> file + gotwebd/fcgi.c
> --- gotwebd/fcgi.c
> +++ gotwebd/fcgi.c
> @@ -311,7 +311,7 @@ fcgi_gen_binary_response(struct request *c, const uint
>  	if (c->sock->client_status == CLIENT_DISCONNECT)
>  		return -1;
>  
> -	if (data == NULL)
> +	if (data == NULL || len == 0)
>  		return 0;
>  
>  	if ((resp = calloc(1, sizeof(struct fcgi_response))) == NULL) {
> @@ -342,43 +342,9 @@ fcgi_gen_binary_response(struct request *c, const uint
>  int
>  fcgi_gen_response(struct request *c, const char *data)
>  {
> -	struct fcgi_response *resp;
> -	struct fcgi_record_header *header;
> -	ssize_t n = 0;
> -	int i;
> -
> -	if (c->sock->client_status == CLIENT_DISCONNECT)
> -		return -1;
> -
> -	if (data == NULL)
> +	if (data == NULL || *data == '\0')
>  		return 0;
> -
> -	if (strlen(data) == 0)
> -		return 0;
> -
> -	if ((resp = calloc(1, sizeof(struct fcgi_response))) == NULL) {
> -		log_warn("%s: cannot calloc fcgi_response", __func__);
> -		return -1;
> -	}
> -
> -	header = (struct fcgi_record_header*) resp->data;
> -	header->version = 1;
> -	header->type = FCGI_STDOUT;
> -	header->id = htons(c->id);
> -	header->padding_len = 0;
> -	header->reserved = 0;
> -
> -	for (i = 0; i < strlen(data); i++) {
> -		resp->data[i+8] = data[i];
> -		n++;
> -	}
> -
> -	header->content_len = htons(n);
> -	resp->data_pos = 0;
> -	resp->data_len = n + sizeof(struct fcgi_record_header);
> -	fcgi_send_response(c, resp);
> -
> -	return 0;
> +	return fcgi_gen_binary_response(c, data, strlen(data));
>  }
>  
>  void
> blob - e8a77c36de8a9bb990ee17fc8764af9390f1915c
> file + gotwebd/got_operations.c
> --- gotwebd/got_operations.c
> +++ gotwebd/got_operations.c
> @@ -55,12 +55,7 @@ static const struct got_error *got_gotweb_blame_cb(voi
>  static int
>  isbinary(const uint8_t *buf, size_t n)
>  {
> -	size_t i;
> -
> -	for (i = 0; i < n; i++)
> -		if (buf[i] == 0)
> -			return 1;
> -	return 0;
> +	return memchr(buf, '\0', n) != NULL;
>  }
>  
>  
> @@ -1201,8 +1196,7 @@ got_output_file_blob(struct request *c)
>  	struct got_reflist_head refs;
>  	struct got_blob_object *blob = NULL;
>  	char *path = NULL, *in_repo_path = NULL;
> -	int obj_type, set_mime = 0, type = 0, fd = -1;
> -	char *buf_output = NULL;
> +	int obj_type, set_mime = 0, fd = -1;
>  	size_t len, hdrlen;
>  	const uint8_t *buf;
>  
> @@ -1274,32 +1268,18 @@ got_output_file_blob(struct request *c)
>  					    error->msg);
>  					goto done;
>  				}
> -				type = 0;
>  			} else {
>  				error = gotweb_render_content_type(c,
> -				  "text/text");
> +				  "text/plain");
>  				if (error) {
>  					log_warnx("%s: %s", __func__,
>  					    error->msg);
>  					goto done;
>  				}
> -				type = 1;
>  			}
>  		}
>  		set_mime = 1;
> -		if (type) {
> -			buf_output = calloc(len - hdrlen + 1,
> -			    sizeof(*buf_output));
> -			if (buf_output == NULL) {
> -				error = got_error_from_errno("calloc");
> -				goto done;
> -			}
> -			memcpy(buf_output, buf, len - hdrlen);
> -			fcgi_gen_response(c, buf_output);
> -			free(buf_output);
> -			buf_output = NULL;
> -		} else
> -			fcgi_gen_binary_response(c, buf, len - hdrlen);
> +		fcgi_gen_binary_response(c, buf, len - hdrlen);
>  
>  		hdrlen = 0;
>  	} while (len != 0);
> @@ -1310,7 +1290,6 @@ done:
>  		error = got_error_from_errno("close");
>  	if (blob)
>  		got_object_blob_close(blob);
> -	free(buf_output);
>  	free(in_repo_path);
>  	free(commit_id);
>  	free(path);
> blob - c30fdedd0e6b808f3a0607b931d83f55c2e7b317
> file + gotwebd/gotweb.c
> --- gotwebd/gotweb.c
> +++ gotwebd/gotweb.c
> @@ -177,11 +177,6 @@ gotweb_process_request(struct request *c)
>  		error = got_get_repo_commits(c, 1);
>  		if (error)
>  			goto done;
> -		error = gotweb_render_content_type(c, "text/plain");
> -		if (error) {
> -			log_warnx("%s: %s", __func__, error->msg);
> -			goto err;
> -		}
>  		error = got_output_file_blob(c);
>  		if (error) {
>  			log_warnx("%s: %s", __func__, error->msg);
> 

-- 

Tracey Emery