Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 26, 2017, 22:08 UTC
Message-ID: <CAPig+cTuBRzoxtmp1_MQ3fUxC5LXpyFPAaEOL7z96e_Szfqn5w@mail.gmail.com>
In-Reply-To: <20171126193813.12531-2-max@max630.net>

On Sun, Nov 26, 2017 at 2:38 PM, Max Kirillov <max@max630.net> wrote:

Show 5 quoted lines

> [...]
> Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
> the whole input until EOF. If the variable is not defined, keep older behavior
> of reading until EOF because it is used to support chunked transfer-encoding.
> [...]

A few small comments below; with the possible exception of one, probably none worth a re-roll...

Show 12 quoted lines

> diff --git a/http-backend.c b/http-backend.c
> @@ -317,6 +317,54 @@ static ssize_t read_request(int fd, unsigned char **out)
> +static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **out)
> +{
> +       unsigned char *buf = NULL;
> +       ssize_t cnt = 0;
> +
> +       if (max_request_buffer < req_len) {
> +               die("request was larger than our maximum size (%lu): %lu;"
> +                           " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
> +                           max_request_buffer,
> +                           req_len);

Unsigned format conversion '%lu' doesn't seem correct for ssize_t.

Show 16 quoted lines

> +       }
> +
> +       if (req_len <= 0) {
> +               *out = NULL;
> +               return 0;
> +       }
> +
> +       buf = xmalloc(req_len);
> +       cnt = read_in_full(fd, buf, req_len);
> +       if (cnt < 0) {
> +               free(buf);
> +               return -1;
> +       } else {
> +               *out = buf;
> +               return cnt;
> +       }

This could have been written:

    if (cnt < 0) {
        free(buf);
        return -1;
    }
    *out = buf;
    return cnt;

but not worth a re-roll.

> +}
> +
> +static ssize_t env_content_length(void)

The caller of this function doesn't care how the content length is being determined -- whether it comes from an environment variable or is computed some other way; it cares only about the result. Having "env" in the name ties it to checking only the environment. A more generic name, such as get_content_length(), would help to decouple the API from the implementation.

Nevertheless, not worth a re-roll.

Show 5 quoted lines

> +{
> +       ssize_t val = -1;
> +       const char *str = getenv("CONTENT_LENGTH");
> +
> +       if (str && !git_parse_ssize_t(str, &val))

git_parse_ssize_t() does the right thing even when 'str' is NULL, so this condition could be simplified (but not worth a re-roll and may not improve clarity).

Show 7 quoted lines

> +               die("failed to parse CONTENT_LENGTH: %s", str);
> +       return val;
> +}
> +
> +static ssize_t read_request(int fd, unsigned char **out)
> +{
> +       ssize_t req_len = env_content_length();

Grabbing and parsing the value from the environment variable is effectively a one-liner, so env_content_length() could be dropped altogether, and instead (taking advantage of git_parse_ssize_t()'s proper NULL-handling):

    if (!git_parse_ssize_t(getenv(...), &req_len))
        die(...);

Not worth a re-roll.

Show 5 quoted lines

> +       if (req_len < 0)
> +               return read_request_eof(fd, out);
> +       else
> +               return read_request_fixed_len(fd, req_len, out);
> +}