From: Tracey Emery Subject: Re: gotwebd: fix double Content-type To: Omar Polo Cc: gameoftrees@openbsd.org Date: Thu, 28 Jul 2022 08:19:52 -0600 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