Re: [PATCH v4 1/8] http: read HTTP WWW-Authenticate response headers
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Dec 15, 2022, 09:27 UTC
- Message-ID
- <221215.861qp13tfh.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <b5b56ccd9419353a4bf5bc9d751a711af07d2197.1670880984.git.gitgitgadget@gmail.com>
On Mon, Dec 12 2022, Matthew John Cheetham via GitGitGadget wrote:
Show 14 quoted lines
> From: Matthew John Cheetham <mjcheetham@outlook.com>
> [...]
> /* Initialize a credential structure, setting all fields to empty. */
> diff --git a/http.c b/http.c
> index 8a5ba3f4776..c4e9cd73e14 100644
> --- a/http.c
> +++ b/http.c
> @@ -183,6 +183,82 @@ size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
> return nmemb;
> }
>
> +static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p)
> +{
> + size_t size = eltsize * nmemb;Just out of general paranoia: use st_mult() here, not "*" (checks for overflows)?
> + struct strvec *values = &http_auth.wwwauth_headers; > + struct strbuf buf = STRBUF_INIT; > + const char *val; > + const char *z = NULL;
Why NULL-init the "z" here, but not the "val"? Both look like they should be un-init'd. We also tend to call a throw-away char pointer "p", not "z", but anyway (more below)....
Show 30 quoted lines
> +
> + /*
> + * Header lines may not come NULL-terminated from libcurl so we must
> + * limit all scans to the maximum length of the header line, or leverage
> + * strbufs for all operations.
> + *
> + * In addition, it is possible that header values can be split over
> + * multiple lines as per RFC 2616 (even though this has since been
> + * deprecated in RFC 7230). A continuation header field value is
> + * identified as starting with a space or horizontal tab.
> + *
> + * The formal definition of a header field as given in RFC 2616 is:
> + *
> + * message-header = field-name ":" [ field-value ]
> + * field-name = token
> + * field-value = *( field-content | LWS )
> + * field-content = <the OCTETs making up the field-value
> + * and consisting of either *TEXT or combinations
> + * of token, separators, and quoted-string>
> + */
> +
> + strbuf_add(&buf, ptr, size);
> +
> + /* Strip the CRLF that should be present at the end of each field */
> + strbuf_trim_trailing_newline(&buf);
> +
> + /* Start of a new WWW-Authenticate header */
> + if (skip_iprefix(buf.buf, "www-authenticate:", &val)) {
> + while (isspace(*val))
> + val++;As we already have a "struct strbuf" here, maybe we can instead consistently use the strbuf functions, e.g. strbuf_ltrim() in this case.
I haven't reviewed this in detail, maybe it's not easy or worth it here...
Show 13 quoted lines
> +
> + strvec_push(values, val);
> + http_auth.header_is_last_match = 1;
> + goto exit;
> + }
> +
> + /*
> + * This line could be a continuation of the previously matched header
> + * field. If this is the case then we should append this value to the
> + * end of the previously consumed value.
> + */
> + if (http_auth.header_is_last_match && isspace(*buf.buf)) {
> + const char **v = values->v + values->nr - 1;It makes no difference to the compiler, but perhaps using []-indexing here is more idiomatic, for getting the nth member of this strvec?
> + char *append = xstrfmt("%s%.*s", *v, (int)(size - 1), ptr + 1);
> +
> + free((void*)*v);Is this reaching into the strvec & manually memory-managing it unavoidable, or can we use strvec_pop() etc?
Show 16 quoted lines
> + *v = append; > + > + goto exit; > + } > + > + /* This is the start of a new header we don't care about */ > + http_auth.header_is_last_match = 0; > + > + /* > + * If this is a HTTP status line and not a header field, this signals > + * a different HTTP response. libcurl writes all the output of all > + * response headers of all responses, including redirects. > + * We only care about the last HTTP request response's headers so clear > + * the existing array. > + */ > + if (skip_iprefix(buf.buf, "http/", &z))
...Don't you want to just skip this "z" variable altogether and use istarts_with() instead? All you seem to care about is whether it starts with it, not what the offset is.