Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

On Fri, Nov 24, 2017 at 02:54:50PM +0900, Junio C Hamano wrote:
> Max Kirillov  writes:
>> I hope I marked it correctly in the trailers.
> 
> It is probably more conventional to do it like so:
> 
> From: Florian Manschwetus 
> Date: 
> 
> Signed-off-by: Florian Manschwetus 
> [mk: fixed trivial build failures and stuff]
> Signed-off-by: Max Kirillov 

Thanks. Have done so

Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

On Sat, Nov 25, 2017 at 07:38:33PM -0500, Eric Sunshine wrote:
> On Sat, Nov 25, 2017 at 4:47 PM, Max Kirillov  wrote:
>> On Thu, Nov 23, 2017 at 08:30:39PM -0500, Eric Sunshine wrote:
 +static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char 
 **out)
>>>
>>> Wrong data type: s/size_t req_len/ssize_t req_len/
>>
>> Passing negative value to the function makes no sense. I
>> could add explicit type cast to make it clear. It should be
>> safe as site_t's range is bigger, and overflown
>> CONTENT_LENGTH results in die() at parsing (I have a test
>> which verifies it)
> 
> A concern with requesting size_t bytes is that, if it does read all
> bytes, that value can't necessarily be represented by the ssize_t
> returned from the function. Where would the cast be placed that you
> suggest? How do other git functions deal with this sort of situation?

Right... ok, let it be ssize_t

Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Eric Sunshine]

On Sat, Nov 25, 2017 at 4:47 PM, Max Kirillov  wrote:
> Thanks for the review. I saw only reaction of the Jeff in
> the original thread and though that it is ok otherwise. I'm
> fixing the things you mentioned.

The commentary (in which you talked about restoring the patch and
squashing) seemed to imply that this had been posted somewhere before,
but it wasn't marked as "v2" (or whatever attempt) and lacked a URL
pointing at the previous attempt, so it was difficult to judge.

> On Thu, Nov 23, 2017 at 08:30:39PM -0500, Eric Sunshine wrote:
>>> +static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char 
>>> **out)
>>
>> Wrong data type: s/size_t req_len/ssize_t req_len/
>
> Passing negative value to the function makes no sense. I
> could add explicit type cast to make it clear. It should be
> safe as site_t's range is bigger, and overflown
> CONTENT_LENGTH results in die() at parsing (I have a test
> which verifies it)

A concern with requesting size_t bytes is that, if it does read all
bytes, that value can't necessarily be represented by the ssize_t
returned from the function. Where would the cast be placed that you
suggest? How do other git functions deal with this sort of situation?

Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

Thanks for the review. I saw only reaction of the Jeff in
the original thread and though that it is ok otherwise. I'm
fixing the things you mentioned.

On Thu, Nov 23, 2017 at 08:30:39PM -0500, Eric Sunshine wrote:
>> +static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char 
>> **out)
> 
> Wrong data type: s/size_t req_len/ssize_t req_len/

Passing negative value to the function makes no sense. I
could add explicit type cast to make it clear. It should be
safe as site_t's range is bigger, and overflown
CONTENT_LENGTH results in die() at parsing (I have a test
which verifies it)

> Rather than writing an entirely new "read" function, how about just
> modifying the existing read_request() to optionally limit the read to
> a specified number of bytes?

I'll check it a bit separately.

-- 
Max

AW: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Florian Manschwetus]

Hi All,
thx Max for jumping in, I wasn't able to complete this due to serious lack of 
time.
Later I forgot it. Great to see that this finally made it.

Mit freundlichen Grüßen / With kind regards
Florian Manschwetus

E-Mail: manschwe...@cs-software-gmbh.de
Tel.: +49-(0)611-8908534
 
CS Software Concepts and Solutions GmbH
Geschäftsführer / Managing director: Dr. Werner Alexi 
Amtsgericht Wiesbaden HRB 10004 (Commercial registry)
Schiersteiner Straße 31
D-65187 Wiesbaden
Germany
Tel.: 0611/8908555

> -Ursprüngliche Nachricht-
> Von: Junio C Hamano [mailto:gits...@pobox.com]
> Gesendet: Freitag, 24. November 2017 06:55
> An: Max Kirillov
> Cc: Jeff King; Florian Manschwetus; Chris Packham; Konstantin Khomoutov;
> git@vger.kernel.org
> Betreff: Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified
> by rfc3875
> 
> Max Kirillov <m...@max630.net> writes:
> 
> > http-backend reads whole input until EOF. However, the RFC 3875
> > specifies that a script must read only as many bytes as specified by
> > CONTENT_LENGTH environment variable. This causes hang under
> IIS/Windows, for example.
> >
> > Make http-backend read only CONTENT_LENGTH bytes, if it's defined,
> > rather than the whole input until EOF. If the varibale is not defined,
> > keep older behavior of reading until EOF because it is used to support
> chunked transfer-encoding.
> >
> > Signed-off-by: Florian Manschwetus <manschwetus@cs-software-
> gmbh.de>
> > Authored-by: Florian Manschwetus <manschwetus@cs-software-
> gmbh.de>
> > Fixed-by: Max Kirillov <m...@max630.net>
> > Signed-off-by: Max Kirillov <m...@max630.net>
> > ---
> > ...
> > I hope I marked it correctly in the trailers.
> 
> It is probably more conventional to do it like so:
> 
> From: Florian Manschwetus <manschwe...@cs-software-gmbh.de>
> Date: 
> 
> http-backend reads whole input until EOF. However, the RFC 3875...
> ... chunked transfer-encoding.
> 
> Signed-off-by: Florian Manschwetus <manschwetus@cs-software-
> gmbh.de>
> [mk: fixed trivial build failures and stuff]
> Signed-off-by: Max Kirillov <m...@max630.net>
> ---
> 
> >
> > +/*
> > + * replacement for original read_request, now renamed to
> > +read_request_eof,
> > + * honoring given content_length (req_len),
> > + * provided by new wrapper function read_request  */
> 
> I agree with Eric's suggestion.  In-code comment is read by those who have
> the current code, without knowing/caring what it used to be.  "It used to do
> this, but replace it with this new thing because..." is a valuable thing to 
> record
> in the log message, but not here.

Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Junio C Hamano]

Max Kirillov  writes:

> http-backend reads whole input until EOF. However, the RFC 3875 specifies
> that a script must read only as many bytes as specified by CONTENT_LENGTH
> environment variable. This causes hang under IIS/Windows, for example.
>
> Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
> the whole input until EOF. If the varibale is not defined, keep older behavior
> of reading until EOF because it is used to support chunked transfer-encoding.
>
> Signed-off-by: Florian Manschwetus 
> Authored-by: Florian Manschwetus 
> Fixed-by: Max Kirillov 
> Signed-off-by: Max Kirillov 
> ---
> ...
> I hope I marked it correctly in the trailers.

It is probably more conventional to do it like so:

From: Florian Manschwetus 
Date: 

http-backend reads whole input until EOF. However, the RFC 3875...
... chunked transfer-encoding.

Signed-off-by: Florian Manschwetus 
[mk: fixed trivial build failures and stuff]
Signed-off-by: Max Kirillov 
---

>  
> +/*
> + * replacement for original read_request, now renamed to read_request_eof,
> + * honoring given content_length (req_len),
> + * provided by new wrapper function read_request
> + */

I agree with Eric's suggestion.  In-code comment is read by those
who have the current code, without knowing/caring what it used to
be.  "It used to do this, but replace it with this new thing
because..." is a valuable thing to record in the log message, but
not here.

Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Eric Sunshine]

On Thu, Nov 23, 2017 at 6:45 PM, Max Kirillov <m...@max630.net> wrote:
> [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

The "RFC" seems to be missing from the subject line of this unpolished patch.

> http-backend reads whole input until EOF. However, the RFC 3875 specifies
> that a script must read only as many bytes as specified by CONTENT_LENGTH
> environment variable. This causes hang under IIS/Windows, for example.

By "_this_ causes a hang", I presume you mean "not respecting
CONTENT_LENGTH causes a hang"? Perhaps that could be spelled out
explicitly.

> Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
> the whole input until EOF. If the varibale is not defined, keep older behavior

s/varibale/variable/

> of reading until EOF because it is used to support chunked transfer-encoding.
>
> Signed-off-by: Florian Manschwetus <manschwe...@cs-software-gmbh.de>
> Authored-by: Florian Manschwetus <manschwe...@cs-software-gmbh.de>
> Fixed-by: Max Kirillov <m...@max630.net>
> Signed-off-by: Max Kirillov <m...@max630.net>
> ---
> diff --git a/http-backend.c b/http-backend.c
> @@ -317,6 +317,76 @@ static ssize_t read_request(int fd, unsigned char **out)
> +/*
> + * replacement for original read_request, now renamed to read_request_eof,
> + * honoring given content_length (req_len),
> + * provided by new wrapper function read_request
> + */

This comment has value only to someone who knew what the code was like
before this change, and it merely repeats what is already implied by
the commit message, rather than providing any valuable information
about this new function itself. Therefore, it should be dropped.

> +static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char 
> **out)

Wrong data type: s/size_t req_len/ssize_t req_len/

Also: s/fix/fixed/

> +{
> +   unsigned char *buf = NULL;
> +   size_t len = 0;
> +
> +   /* check request size */

Comment merely repeats what code says, thus has no value. Please drop.

> +   if (max_request_buffer < req_len) {
> +   die("request was larger than our maximum size (%lu);"
> +   " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
> +   max_request_buffer);

This error message neglects to say what the request size was. Such
information would be useful given that it suggests bumping
GIT_HTTP_MAX_REQUEST_BUFFER to a larger value.

> +   }
> +
> +   if (req_len <= 0) {
> +   *out = NULL;
> +   return 0;
> +   }
> +
> +   /* allocate buffer */

Drop valueless comment.

> +   buf = xmalloc(req_len);
> +
> +

Style: Too many blank lines.

> +   while (1) {
> +   ssize_t cnt;
> +
> +   cnt = read_in_full(fd, buf + len, req_len - len);
> +   if (cnt < 0) {
> +   free(buf);
> +   return -1;
> +   }
> +
> +   /* partial read from read_in_full means we hit EOF */
> +   len += cnt;
> +   if (len < req_len) {
> +   /* TODO request incomplete?? */
> +   /* maybe just remove this block and condition along 
> with the loop, */
> +   /* if read_in_full is prooven reliable */

s/prooven/proven/

> +   *out = buf;
> +   return len;
> +   } else {
> +   /* request complete */
> +   *out = buf;
> +   return len;
> +
> +   }
> +   }

What is the purpose of the while(1) loop? Every code path inside the
loop returns, so it will never execute more than once. Likewise, why
is 'len' needed?

Rather than writing an entirely new "read" function, how about just
modifying the existing read_request() to optionally limit the read to
a specified number of bytes?

> +}
> +
> +/**
> + * wrapper function, whcih determines based on CONTENT_LENGTH value,

s/whcih/which/

Also, the placement of commas needs some attention.

> + * to
> + * - use old behaviour of read_request, to read until EOF
> + * => read_request_eof(...)
> + * - just read CONTENT_LENGTH-bytes, when provided
> + * => read_request_fix_len(...)
> + */

When talking about "old behavior", this comment is repeating
information more suitable to the commit message (and effectively
already covered there); information which only has value to someone
who knew what the old code/behavior was like. The rest of this comment
is merely repeating what the code itself already says, thus adds no
value, so should be dropped.

> +static ssize_t read_request(int fd, unsigned char **out)
> +{
> +   /* get request size */

Drop valueless comment.

> +   ssize_t req_len = git_env_ssize_t("CONTENT_LENGTH", -1);
> +   if (req_len < 0)
> +   return read_request_eof(fd, out);
> +   else
> +   return read_request_fix_len(fd, req_len, out);
> +}

[PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

http-backend reads whole input until EOF. However, the RFC 3875 specifies
that a script must read only as many bytes as specified by CONTENT_LENGTH
environment variable. This causes hang under IIS/Windows, for example.

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

Signed-off-by: Florian Manschwetus 
Authored-by: Florian Manschwetus 
Fixed-by: Max Kirillov 
Signed-off-by: Max Kirillov 
---
Hi

I came across this issue, and I think is should be good to restore the patch.
It is basically same but I squashed them, fixed the thing you mentioned and
also some trivial build failures (null -> NULL and missing return from the 
wrapper).
I hope I marked it correctly in the trailers.
 config.c   |  8 +++
 config.h   |  1 +
 http-backend.c | 72 +-
 3 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 231f9a750b..925bb65dfa 100644
--- a/config.c
+++ b/config.c
@@ -1525,6 +1525,14 @@ unsigned long git_env_ulong(const char *k, unsigned long 
val)
return val;
 }
 
+ssize_t git_env_ssize_t(const char *k, ssize_t val)
+{
+   const char *v = getenv(k);
+   if (v && !git_parse_ssize_t(v, ))
+   die("failed to parse %s", k);
+   return val;
+}
+
 int git_config_system(void)
 {
return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
diff --git a/config.h b/config.h
index 0352da117b..947695c304 100644
--- a/config.h
+++ b/config.h
@@ -74,6 +74,7 @@ extern int git_config_rename_section_in_file(const char *, 
const char *, const c
 extern const char *git_etc_gitconfig(void);
 extern int git_env_bool(const char *, int);
 extern unsigned long git_env_ulong(const char *, unsigned long);
+extern ssize_t git_env_ssize_t(const char *, ssize_t);
 extern int git_config_system(void);
 extern int config_error_nonbool(const char *);
 #if defined(__GNUC__)
diff --git a/http-backend.c b/http-backend.c
index 519025d2c3..317b99b87c 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -280,7 +280,7 @@ static struct rpc_service *select_service(struct strbuf 
*hdr, const char *name)
  * hit max_request_buffer we die (we'd rather reject a
  * maliciously large request than chew up infinite memory).
  */
-static ssize_t read_request(int fd, unsigned char **out)
+static ssize_t read_request_eof(int fd, unsigned char **out)
 {
size_t len = 0, alloc = 8192;
unsigned char *buf = xmalloc(alloc);
@@ -317,6 +317,76 @@ static ssize_t read_request(int fd, unsigned char **out)
}
 }
 
+/*
+ * replacement for original read_request, now renamed to read_request_eof,
+ * honoring given content_length (req_len),
+ * provided by new wrapper function read_request
+ */
+static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char 
**out)
+{
+   unsigned char *buf = NULL;
+   size_t len = 0;
+
+   /* check request size */
+   if (max_request_buffer < req_len) {
+   die("request was larger than our maximum size (%lu);"
+   " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+   max_request_buffer);
+   }
+
+   if (req_len <= 0) {
+   *out = NULL;
+   return 0;
+   }
+
+   /* allocate buffer */
+   buf = xmalloc(req_len);
+
+
+   while (1) {
+   ssize_t cnt;
+
+   cnt = read_in_full(fd, buf + len, req_len - len);
+   if (cnt < 0) {
+   free(buf);
+   return -1;
+   }
+
+   /* partial read from read_in_full means we hit EOF */
+   len += cnt;
+   if (len < req_len) {
+   /* TODO request incomplete?? */
+   /* maybe just remove this block and condition along 
with the loop, */
+   /* if read_in_full is prooven reliable */
+   *out = buf;
+   return len;
+   } else {
+   /* request complete */
+   *out = buf;
+   return len;
+   
+   }
+   }
+}
+
+/**
+ * wrapper function, whcih determines based on CONTENT_LENGTH value,
+ * to
+ * - use old behaviour of read_request, to read until EOF
+ * => read_request_eof(...)
+ * - just read CONTENT_LENGTH-bytes, when provided
+ * => read_request_fix_len(...)
+ */
+static ssize_t read_request(int fd, unsigned char **out)
+{
+   /* get request size */
+   ssize_t req_len = git_env_ssize_t("CONTENT_LENGTH", -1);
+   if (req_len < 0)
+   return read_request_eof(fd, out);
+   else
+   return