Re: [PATCH] HTTP/2: make http2 server support http1

[Haitao Lv]

Hello, here is another patch(more sample) according Maxim Dounin advice.

Please offer your comment. Thanks.

diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
index 89cfe77a..71bc7b59 100644
--- a/src/http/ngx_http_request.c
+++ b/src/http/ngx_http_request.c
@@ -17,6 +17,10 @@ static ssize_t 
ngx_http_read_request_header(ngx_http_request_t *r);
 static ngx_int_t ngx_http_alloc_large_header_buffer(ngx_http_request_t *r,
 ngx_uint_t request_line);
 
+#if (NGX_HTTP_V2)
+static void ngx_http_wait_v2_preface_handler(ngx_event_t *rev);
+#endif
+
 static ngx_int_t ngx_http_process_header_line(ngx_http_request_t *r,
 ngx_table_elt_t *h, ngx_uint_t offset);
 static ngx_int_t ngx_http_process_unique_header_line(ngx_http_request_t *r,
@@ -321,7 +325,7 @@ ngx_http_init_connection(ngx_connection_t *c)
 
 #if (NGX_HTTP_V2)
 if (hc->addr_conf->http2) {
-rev->handler = ngx_http_v2_init;
+rev->handler = ngx_http_wait_v2_preface_handler;
 }
 #endif
 
@@ -377,6 +381,108 @@ ngx_http_init_connection(ngx_connection_t *c)
 }
 
 
+#if (NGX_HTTP_V2)
+static void
+ngx_http_wait_v2_preface_handler(ngx_event_t *rev)
+{
+size_t size;
+ssize_tn;
+ngx_buf_t *b;
+ngx_connection_t  *c;
+
+c = rev->data;
+
+ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0,
+"http wait h2 preface handler");
+
+if (rev->timedout) {
+ngx_log_error(NGX_LOG_INFO, c->log, NGX_ETIMEDOUT, "client timed out");
+ngx_http_close_connection(c);
+return;
+}
+
+if (c->close) {
+ngx_http_close_connection(c);
+return;
+}
+
+size = 5 /* strlen("PRI *") */;
+
+b = c->buffer;
+
+if (b == NULL) {
+b = ngx_create_temp_buf(c->pool, size);
+if (b == NULL) {
+ngx_http_close_connection(c);
+return;
+}
+
+c->buffer = b;
+
+} else if (b->start == NULL) {
+
+b->start = ngx_palloc(c->pool, size);
+if (b->start == NULL) {
+ngx_http_close_connection(c);
+return;
+}
+
+b->pos = b->start;
+b->last = b->start;
+b->end = b->last + size;
+}
+
+n = c->recv(c, b->last, size);
+
+if (n == NGX_AGAIN) {
+
+if (!rev->timer_set) {
+ngx_add_timer(rev, c->listening->post_accept_timeout);
+ngx_reusable_connection(c, 1);
+}
+
+if (ngx_handle_read_event(rev, 0) != NGX_OK) {
+ngx_http_close_connection(c);
+return;
+}
+
+/*
+ * We are trying to not hold c->buffer's memory for an idle connection.
+ */
+
+if (ngx_pfree(c->pool, b->start) == NGX_OK) {
+b->start = NULL;
+}
+
+return;
+}
+
+if (n == NGX_ERROR) {
+ngx_http_close_connection(c);
+return;
+}
+
+if (n == 0) {
+ngx_log_error(NGX_LOG_INFO, c->log, 0,
+  "client closed connection");
+ngx_http_close_connection(c);
+return;
+}
+
+b->last += n;
+
+if (b->last == b->end) {
+if (ngx_strncmp(b->start, "PRI *", 5) == 0) {
+ngx_http_v2_init_with_buf(rev, b);
+} else {
+rev->handler = ngx_http_wait_request_handler;
+ngx_http_wait_request_handler(rev);
+}
+}
+}
+#endif
+
+
 static void
 ngx_http_wait_request_handler(ngx_event_t *rev)
 {
@@ -430,6 +536,21 @@ ngx_http_wait_request_handler(ngx_event_t *rev)
 b->pos = b->start;
 b->last = b->start;
 b->end = b->last + size;
+} else {
+
+p = ngx_palloc(c->pool, size);
+if (p == NULL) {
+ngx_http_close_connection(c);
+return;
+}
+
+n = b->last - b->start;
+ngx_memcpy(p, b->start, n);
+
+b->start = p;
+b->pos = b->start;
+b->last = b->start + n;
+b->end = b->last + size;
 }
 
 n = c->recv(c, b->last, size);
diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c
index d9df0f90..a990c96f 100644
--- a/src/http/v2/ngx_http_v2.c
+++ b/src/http/v2/ngx_http_v2.c
@@ -231,6 +231,14 @@ static ngx_http_v2_parse_header_t  
ngx_http_v2_parse_headers[] = {
 void
 ngx_http_v2_init(ngx_event_t *rev)
 {
+ngx_http_v2_init_with_buf(rev, NULL);
+}
+
+
+void
+ngx_http_v2_init_with_buf(ngx_event_t *rev, ngx_buf_t *buf)
+{
+size_t size;
 ngx_connection_t  *c;
 ngx_pool_cleanup_t*cln;
 ngx_http_connection_t *hc;
@@ -262,6 +270,17 @@ ngx_http_v2_init(ngx_event_t *rev)
 return;
 }
 
+if (buf != NULL) {
+size = buf->last - buf->start;
+
+if (size > h2mcf->recv_buffer_size) {
+size = h2mcf->recv_buffer_size;
+}
+
+ngx_memcpy(h2mcf->recv_buffer, buf->start, size);
+h2c->state.buffer_used = size;
+}
+
 h2c->connection = c;
 

[nginx] HTTP/2: unknown frames now logged at info level.

[Maxim Dounin]

details:   http://hg.nginx.org/nginx/rev/e80930e5e422
branches:  
changeset: 7225:e80930e5e422
user:  Maxim Dounin 
date:  Mon Mar 05 21:35:13 2018 +0300
description:
HTTP/2: unknown frames now logged at info level.

diffstat:

 src/http/v2/ngx_http_v2.c |  4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diffs (14 lines):

diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c
--- a/src/http/v2/ngx_http_v2.c
+++ b/src/http/v2/ngx_http_v2.c
@@ -783,8 +783,8 @@ ngx_http_v2_state_head(ngx_http_v2_conne
type, h2c->state.flags, h2c->state.length, h2c->state.sid);
 
 if (type >= NGX_HTTP_V2_FRAME_STATES) {
-ngx_log_debug1(NGX_LOG_DEBUG_HTTP, h2c->connection->log, 0,
-   "http2 frame with unknown type %ui", type);
+ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
+  "client sent frame with unknown type %ui", type);
 return ngx_http_v2_state_skip(h2c, pos, end);
 }
 
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

[nginx] Style.

[Maxim Dounin]

details:   http://hg.nginx.org/nginx/rev/1ab290cf5267
branches:  
changeset: 7224:1ab290cf5267
user:  Maxim Dounin 
date:  Mon Mar 05 21:35:08 2018 +0300
description:
Style.

diffstat:

 src/stream/ngx_stream_log_module.c |  3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diffs (13 lines):

diff --git a/src/stream/ngx_stream_log_module.c 
b/src/stream/ngx_stream_log_module.c
--- a/src/stream/ngx_stream_log_module.c
+++ b/src/stream/ngx_stream_log_module.c
@@ -868,7 +868,8 @@ ngx_stream_log_json_variable(ngx_stream_
 
 
 static size_t
-ngx_stream_log_unescaped_variable_getlen(ngx_stream_session_t *s, uintptr_t 
data)
+ngx_stream_log_unescaped_variable_getlen(ngx_stream_session_t *s,
+uintptr_t data)
 {
 ngx_stream_variable_value_t  *value;
 
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH] HTTP/2: make http2 server support http1

[Maxim Dounin]

Hello!

On Mon, Mar 05, 2018 at 11:52:57PM +0800, Haitao Lv wrote:

[...]

> > Overall, the patch looks like a hack and introduces too much
> > complexity for this feature.  While I understand the reasoning,
> > the proposed implementation cannot be accepted.
> 
> Could you clarify that whether is this feature not accepted or this patch?
> 
> If this feature is not needed, I will terminate this thread.
> 
> If this patch only looks like a hack, would you like offer any advice to write
> code with good smell?

We've previously discussed this with Valentin, and our position is 
as follows:

- The feature itself (autodetection between HTTP/2 and HTTP/1.x 
  protocols) might be usable, and we can consider adding it if 
  there will be a good and simple enough patch.  (Moreover, we 
  think that this probably should be the default if "listen ... 
  http2" is configured - that is, no "http1" option.)

- The patch suggested certainly doesn't meet the above criteria, 
  and it does not look like it can be fixed.

We don't know if a good and simple enough implementation is at all 
possible though.  One of the possible approaches was already 
proposed by Valentin (detect HTTP/2 or HTTP/1.x before starting 
processing, may be similar to how we handle http-to-https 
requests), but it's now immediately clear if it will work or not.  
Sorry, but please don't expect any of us to provide further 
guidance.

-- 
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH] HTTP/2: make http2 server support http1

[Haitao Lv]

Hello wbr,

Thank you for reviewing this patch.

> On Mar 5, 2018, at 23:06, Valentin V. Bartenev  wrote:
> 
> On Monday 05 March 2018 11:11:08 Haitao Lv wrote:
> [..]
>>> @@ -145,10 +152,6 @@ ngx_http_parse_request_line(ngx_http_request_t *r, 
>>> ngx_buf_t *b)
>>>case sw_start:
>>>r->request_start = p;
>>> 
>>> -if (ch == CR || ch == LF) {
>>> -break;
>>> -}
>>> -
>> 
>> I think Nginx should not allow any leading \r or \n. HTTP client should 
>> never send this chars
>> before the request line. Support this feature makes the buffer management 
>> more harder.
> 
> Support for this feature is required.  See the notes from Ruslan.
> Some clients send additional empty line between keepalive requests.

Yes, this is a mistake. I did not read the RFC carefully. Sorry again. But this 
change isn't needed.

> 
> [..]
>>> +#if (NGX_HTTP_V2)
>>> +case sw_h2_preface:
>>> +
>>> +if (ch == *n++) {
>> 
>> I introduce a new sw_h2_preface state. In this state, the parser will check 
>> the h2 preface char by char.
>> 
>> The parser only check at most 20 chars "* HTTP/2.0\r\n\r\nSM\r\n\r\n". So 
>> the buffer will not be fulfilled.
>> 
> 
> The recv() operation gets buffer size as an argument.
> 
> Thus, the buffer can be fully filled with all the data copied from
> a socket buffer.  HTTP/2 clients usually don't send preface alone.
> They can send other data with the preface. 

Yes, the buffer could be full filled. HTTP/2 client will send more frames after 
the h2 preface, this is
why I introduce the ngx_http_v2_init_after_preface(ngx_event_t *rev, ngx_buf_t 
*buf).

All the remaining data in the buffer will be copied to the h2mcf->recv_buffer 
and be processed by the HTTP/2 ngx_http_v2_state_head handler.

So this will not breaks the HTTP/2 handshake.

However, if we received data is longer than the h2mcf->recv_buffer_size, the 
current patch will yield a
bad request response. 

A bad case is the client_header_buffer_size(default 1k) set larger than the 
http2_recv_buffer_size(default 265k), we will may fully filled the header_in 
buffer but cannot be copied into the h2mcf->recv_buffer,
which will cause the HTTP/2 handshake failed. But simple way to workaround this 
issue is to change the
http2_recv_buffer_size to or larger than client_header_buffer_size. It is a 
hack.

> 
> [..]
>>> +#if (NGX_HTTP_V2)
>>> +static void
>>> +ngx_http_process_h2_preface(ngx_event_t *rev)
>>> +{
>>> +size_t  len;
>>> +ngx_connection_t   *c;
>>> +ngx_http_request_t *r;
>>> +ngx_http_connection_t  *hc;
>>> +ngx_http_v2_main_conf_t*h2mcf;
>>> +
>>> +c = rev->data;
>>> +r = c->data;
>>> +hc = r->http_connection;
>>> +
>>> +ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0,
>>> +"http process h2 preface");
>>> +
>>> +r->header_in->pos = r->header_in->start + 24;
>>> +
>>> +len = r->header_in->last - r->header_in->pos;
>>> +
>>> +h2mcf = ngx_http_get_module_main_conf(hc->conf_ctx, 
>>> ngx_http_v2_module);
>>> +
>>> +if (len <= h2mcf->recv_buffer_size) {
>> 
>> We check the recv buffer size before do the ngx_http_v2_init_after_preface.
>> 
> [..]
> 
> See above.  It's a bad idea to decline request with a "client sent
> invalid h2 preface" message in error log just because client has
> sent more HTTP/2 frames right after the preface.

In this path, all the valid HTTP/2 handshake will never generate a error log.

> 
> Overall, the patch looks like a hack and introduces too much
> complexity for this feature.  While I understand the reasoning,
> the proposed implementation cannot be accepted.

Could you clarify that whether is this feature not accepted or this patch?

If this feature is not needed, I will terminate this thread.

If this patch only looks like a hack, would you like offer any advice to write
code with good smell?

Thank you.

> 
> Protocol detection before starting processing HTTP/1 or HTTP/2
> could be a better way to go.
> 
>  wbr, Valentin V. Bartenev
> 
> ___
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel



___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH] HTTP/2: make http2 server support http1

[Valentin V. Bartenev]

On Monday 05 March 2018 11:11:08 Haitao Lv wrote:
[..]
> > @@ -145,10 +152,6 @@ ngx_http_parse_request_line(ngx_http_request_t *r, 
> > ngx_buf_t *b)
> > case sw_start:
> > r->request_start = p;
> > 
> > -if (ch == CR || ch == LF) {
> > -break;
> > -}
> > -
> 
> I think Nginx should not allow any leading \r or \n. HTTP client should never 
> send this chars
> before the request line. Support this feature makes the buffer management 
> more harder.

Support for this feature is required.  See the notes from Ruslan.
Some clients send additional empty line between keepalive requests.


[..]
> > +#if (NGX_HTTP_V2)
> > +case sw_h2_preface:
> > +
> > +if (ch == *n++) {
> 
> I introduce a new sw_h2_preface state. In this state, the parser will check 
> the h2 preface char by char.
> 
> The parser only check at most 20 chars "* HTTP/2.0\r\n\r\nSM\r\n\r\n". So the 
> buffer will not be fulfilled.
> 

The recv() operation gets buffer size as an argument.

Thus, the buffer can be fully filled with all the data copied from
a socket buffer.  HTTP/2 clients usually don't send preface alone.
They can send other data with the preface. 


[..]
> > +#if (NGX_HTTP_V2)
> > +static void
> > +ngx_http_process_h2_preface(ngx_event_t *rev)
> > +{
> > +size_t  len;
> > +ngx_connection_t   *c;
> > +ngx_http_request_t *r;
> > +ngx_http_connection_t  *hc;
> > +ngx_http_v2_main_conf_t*h2mcf;
> > +
> > +c = rev->data;
> > +r = c->data;
> > +hc = r->http_connection;
> > +
> > +ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0,
> > +"http process h2 preface");
> > +
> > +r->header_in->pos = r->header_in->start + 24;
> > +
> > +len = r->header_in->last - r->header_in->pos;
> > +
> > +h2mcf = ngx_http_get_module_main_conf(hc->conf_ctx, 
> > ngx_http_v2_module);
> > +
> > +if (len <= h2mcf->recv_buffer_size) {
> 
> We check the recv buffer size before do the ngx_http_v2_init_after_preface.
> 
[..]

See above.  It's a bad idea to decline request with a "client sent
invalid h2 preface" message in error log just because client has
sent more HTTP/2 frames right after the preface.

Overall, the patch looks like a hack and introduces too much
complexity for this feature.  While I understand the reasoning,
the proposed implementation cannot be accepted.

Protocol detection before starting processing HTTP/1 or HTTP/2
could be a better way to go.

  wbr, Valentin V. Bartenev

___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH] HTTP/2: make http2 server support http1

[Haitao Lv]

> On Mar 5, 2018, at 17:02, Ruslan Ermilov  wrote:
> 
> On Mon, Mar 05, 2018 at 11:11:08AM +0800, Haitao Lv wrote:
> [...]
>>> @@ -145,10 +152,6 @@ ngx_http_parse_request_line(ngx_http_request_t *r, 
>>> ngx_buf_t *b)
>>>case sw_start:
>>>r->request_start = p;
>>> 
>>> -if (ch == CR || ch == LF) {
>>> -break;
>>> -}
>>> -
>> 
>> I think Nginx should not allow any leading \r or \n. HTTP client should 
>> never send this chars
>> before the request line. Support this feature makes the buffer management 
>> more harder.
> 
> https://tools.ietf.org/html/rfc2616#section-4.1
> 
> In the interest of robustness, servers SHOULD ignore any empty
> line(s) received where a Request-Line is expected. In other words, if
> the server is reading the protocol stream at the beginning of a
> message and receives a CRLF first, it should ignore the CRLF.
> 
> 
> https://tools.ietf.org/html/rfc7230#section-3.5
> 
> In the interest of robustness, a server that is expecting to receive
> and parse a request-line SHOULD ignore at least one empty line (CRLF)
> received prior to the request-line.

Thank you, it is a mistake.

> ___
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel



___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH] HTTP/2: make http2 server support http1

[Ruslan Ermilov]

On Mon, Mar 05, 2018 at 11:11:08AM +0800, Haitao Lv wrote:
[...]
> > @@ -145,10 +152,6 @@ ngx_http_parse_request_line(ngx_http_request_t *r, 
> > ngx_buf_t *b)
> > case sw_start:
> > r->request_start = p;
> > 
> > -if (ch == CR || ch == LF) {
> > -break;
> > -}
> > -
> 
> I think Nginx should not allow any leading \r or \n. HTTP client should never 
> send this chars
> before the request line. Support this feature makes the buffer management 
> more harder.

https://tools.ietf.org/html/rfc2616#section-4.1

In the interest of robustness, servers SHOULD ignore any empty
line(s) received where a Request-Line is expected. In other words, if
the server is reading the protocol stream at the beginning of a
message and receives a CRLF first, it should ignore the CRLF.


https://tools.ietf.org/html/rfc7230#section-3.5

In the interest of robustness, a server that is expecting to receive
and parse a request-line SHOULD ignore at least one empty line (CRLF)
received prior to the request-line.
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: HTTP/2: used bitwise operation to replace division and modulo.

[Ruslan Ermilov]

On Sun, Mar 04, 2018 at 10:36:43PM -0800, tokers wrote:
> # HG changeset patch
> # User Alex Zhang 
> # Date 1520229178 -28800
> #  Mon Mar 05 13:52:58 2018 +0800
> # Node ID 03ecef37a93d541e55802393636c101a4da14550
> # Parent  265c29b0b8b8c54b1c623268481ed85324ce3c79
> HTTP/2: used bitwise operation to replace division and modulo.
> 
> Signed-off-by: Alex Zhang 

No, thanks.

- there is no difference in assembler output even with -O0,
  with either GCC or Clang, and the author likes it the way
  it's written;

- this would require parenthesis around the & operator on
  the LHS of ==.

- the patch is incomplete.

Also, please fix your MUA so that it doesn't break patches
by wrapping lines.

> diff -r 265c29b0b8b8 -r 03ecef37a93d src/http/v2/ngx_http_v2.c
> --- a/src/http/v2/ngx_http_v2.c Thu Mar 01 11:42:55 2018 +0300
> +++ b/src/http/v2/ngx_http_v2.c Mon Mar 05 13:52:58 2018 +0800
> @@ -1094,7 +1094,7 @@
> "depends on %ui excl:%ui weight:%ui",
> h2c->state.sid, depend, excl, weight);
> 
> -if (h2c->state.sid % 2 == 0 || h2c->state.sid <= h2c->last_sid) {
> +if (h2c->state.sid & 0x1 == 0 || h2c->state.sid <= h2c->last_sid) {
>  ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
>"client sent HEADERS frame with incorrect identifier
> "
>"%ui, the last was %ui", h2c->state.sid,
> h2c->last_sid);
> diff -r 265c29b0b8b8 -r 03ecef37a93d src/http/v2/ngx_http_v2_filter_module.c
> --- a/src/http/v2/ngx_http_v2_filter_module.c   Thu Mar 01 11:42:55 2018
> +0300
> +++ b/src/http/v2/ngx_http_v2_filter_module.c   Mon Mar 05 13:52:58 2018
> +0800
> @@ -1149,8 +1149,8 @@
>  value -= prefix;
> 
>  while (value >= 128) {
> -*pos++ = value % 128 + 128;
> -value /= 128;
> +*pos++ = (value & 0x7f) + 128;
> +value >>= 7;
>  }
> 
>  *pos++ = (u_char) value;

> ___
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel


-- 
Ruslan Ermilov
Assume stupidity not malice
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel