On 2022-12-15 01:27, Ævar Arnfjörð Bjarmason wrote:
>
> On Mon, Dec 12 2022, Matthew John Cheetham via GitGitGadget wrote:
>
>> From: Matthew John Cheetham <mjcheetham@xxxxxxxxxxx>
>> [...]
>> /* 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)?
Sure! Good point.
>> + 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)....
>
>> +
>> + /*
>> + * 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.
That's a good point. I can move to using strbuf functions entirely.
> I haven't reviewed this in detail, maybe it's not easy or worth it
> here...
>
>> +
>> + 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?
Sure!
>> + 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?
Again, good point. I can rework this to pop and push a new, joined value.
>> + *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.
>
Again, a good point. Thanks for the suggestions. My next iteration will include
this.
Thanks,
Matthew
