"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Omar Polo <op@omarpolo.com>
Subject:
gotwebd: fix double Content-type
To:
gameoftrees@openbsd.org
Cc:
Tracey Emery <tracey@traceyemery.net>
Date:
Wed, 27 Jul 2022 11:13:04 +0200

Download raw body.

Thread
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

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