Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Piotr Sikora]

Hey Maxim,

> I'm talking about trailers in general (though it's more about
> requests than responses).  Normal request (and response)
> processing in nginx assumes that headers are processed before the
> body, and adding trailers (which are headers "to be added later")
> to the picture are likely to have various security implications.

Let's step back a bit... I have no plans to change the processing
logic nor merge trailers with headers. Trailers are going to be
ignored (passed, but not processed) by NGINX, not discarded.

AFAIK, Apache does the same thing.

Basically, at this point, trailers act as metadata for the application
(browser, gRPC, 3rd-party NGINX module, etc.), with no HTTP semantics,
so there are no security implications for NGINX itself.

Best regards,
Piotr Sikora

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

Re: [PATCH 2 of 2] Core: remove NGX_TIME_T_SIZE

[Maxim Dounin]

Hello!

On Wed, Jul 13, 2016 at 11:00:06AM -0700, Piotr Sikora wrote:

> Hey Maxim,
> 
> > I don't think that size of time_t should be removed from module
> > signatures.  E.g., OpenBSD switched to 64-bit time_t on 32-bit
> > hosts a couple of years ago, and I would expect similar things to
> > happen on other platforms as well.  Signatures were designed to
> > prevent loading of incompatible modules in such cases.
> 
> I'm aware of that change... I don't have a way to test this, but I
> assume that modules wouldn't load even without NGX_TIME_T_SIZE being
> in the signature, because of the libc and/or kernel ABI bump.
> 
> Also, it's not clear to me why time_t is part of the signature, but
> ino_t and off_t (for example) aren't.

Because we don't have NGX_INO_T_SIZE/NGX_OFF_T_SIZE readily 
available and decided that adding them don't worth the effort.

> > It can be replaced with, e.g., NGX_TIME_T_LEN, but I don't see
> > reasoning behind these changes.  Are you trying to make it
> > possible to build nginx as a multiarchitecture binary?
> 
> Removal of values detected at the ./configure-time is one of the ways
> to enable cross-compilation, since the autotest binary cannot be run
> on the system running ./configure script.

For cross-compilation it might be more reasonable to use 
static/compile-time assertions in auto/types/sizeof instead.  
There were previous attempts to introduce such changes.

-- 
Maxim Dounin
http://nginx.org/

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

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Maxim Dounin]

Hello!

On Wed, Jul 13, 2016 at 10:25:57AM -0700, Piotr Sikora wrote:

> > So the question remains: are there any real-world use cases?  May
> > be someone will be able to provide some.
> 
> It's a chicken-and-egg problem. It's hard to use a feature that's not
> supported by one of the most popular web servers.

The feature was there for years before nginx became popular.  And 
no one prevents it to be used with other servers, including more 
popular ones.  The fact that there are no real-world use cases 
suggests that there is something wrong with the feature.

> > Without real-world use cases I don't think this should be added,
> > as in general trailers is quite external concept to how nginx
> > works and also may have various security implications.
> 
> Just to be clear, are you talking about HTTP/1.1 trailers or trailers
> in general? The patch also includes HTTP/2 trailers and it's not clear
> which one you don't like.

I'm talking about trailers in general (though it's more about 
requests than responses).  Normal request (and response) 
processing in nginx assumes that headers are processed before the 
body, and adding trailers (which are headers "to be added later") 
to the picture are likely to have various security implications.

> > Silently dropping trailers is what anyway happens if the client
> > doesn't support chunked encoding at all (e.g., uses HTTP/1.0).
> > And this is also what happens in your patch if there is no "TE:
> > trailers".
> 
> Yes, but then it happens for a reason (client doesn't support
> trailers) and not "just because".

"Transfer encoding used doesn't allow trailers" looks like a 
reason for me.

> > I think that whether to drop Content-Length and switch to chunked
> > encoding is highly use-case specific question.  In some cases it
> > may be appropriate, in some cases not, and in some cases one may
> > want to add trailers even without "TE: trailers".  So forcing
> > chunked encoding probably should be configured separately.  On the
> > other hand, it's very hard to decide something given there are no
> > real use cases known.
> 
> Why? I don't see anyone making a big deal out of forced chunked
> encoding with "gzip on".

Gzip drops Content-Length because it's not possible to know it in 
advice.  And does so only for some responses, with various 
configuration options available to prevent this happening for 
clients who can't handle it and/or for responses where it is not 
beneficial.

-- 
Maxim Dounin
http://nginx.org/

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

Re: [PATCH] Core: reorder #ifdef in .h file to match order in .c file

[Maxim Dounin]

Hello!

On Wed, Jul 13, 2016 at 10:29:04AM -0700, Piotr Sikora wrote:

> Hey Maxim,
> 
> > Current order matches one used in #if in the same .h file, and I
> > don't think this order needs to be changed.
> 
> Hmm? Are we looking at the same files?
> 
> http://hg.nginx.org/nginx/file/tip/src/os/unix/ngx_setaffinity.h:
> 
> #if (NGX_HAVE_SCHED_SETAFFINITY)
> ...
> #elif (NGX_HAVE_CPUSET_SETAFFINITY)
> ...
> #endif
> 
> http://hg.nginx.org/nginx/file/tip/src/os/unix/ngx_setaffinity.c:
> 
> #if (NGX_HAVE_CPUSET_SETAFFINITY)
> 
> #elif (NGX_HAVE_SCHED_SETAFFINITY)
> 
> #endif
> 
> The order is different... And yes, I know that's nitpicking.

I wrote "in #if in the same .h file", that is,
http://hg.nginx.org/nginx/file/tip/src/os/unix/ngx_setaffinity.h#l10:

#if (NGX_HAVE_SCHED_SETAFFINITY || NGX_HAVE_CPUSET_SETAFFINITY)
...
#if (NGX_HAVE_SCHED_SETAFFINITY)
...
#elif (NGX_HAVE_CPUSET_SETAFFINITY)
...
#endif
...

-- 
Maxim Dounin
http://nginx.org/

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

Re: [PATCH 2 of 2] Core: remove NGX_TIME_T_SIZE

[Piotr Sikora]

Hey Maxim,

> I don't think that size of time_t should be removed from module
> signatures.  E.g., OpenBSD switched to 64-bit time_t on 32-bit
> hosts a couple of years ago, and I would expect similar things to
> happen on other platforms as well.  Signatures were designed to
> prevent loading of incompatible modules in such cases.

I'm aware of that change... I don't have a way to test this, but I
assume that modules wouldn't load even without NGX_TIME_T_SIZE being
in the signature, because of the libc and/or kernel ABI bump.

Also, it's not clear to me why time_t is part of the signature, but
ino_t and off_t (for example) aren't.

> It can be replaced with, e.g., NGX_TIME_T_LEN, but I don't see
> reasoning behind these changes.  Are you trying to make it
> possible to build nginx as a multiarchitecture binary?

Removal of values detected at the ./configure-time is one of the ways
to enable cross-compilation, since the autotest binary cannot be run
on the system running ./configure script.

Best regards,
Piotr Sikora

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

Re: [PATCH] Core: reorder #ifdef in .h file to match order in .c file

[Piotr Sikora]

Hey Maxim,

> Current order matches one used in #if in the same .h file, and I
> don't think this order needs to be changed.

Hmm? Are we looking at the same files?

http://hg.nginx.org/nginx/file/tip/src/os/unix/ngx_setaffinity.h:

#if (NGX_HAVE_SCHED_SETAFFINITY)
...
#elif (NGX_HAVE_CPUSET_SETAFFINITY)
...
#endif

http://hg.nginx.org/nginx/file/tip/src/os/unix/ngx_setaffinity.c:

#if (NGX_HAVE_CPUSET_SETAFFINITY)

#elif (NGX_HAVE_SCHED_SETAFFINITY)

#endif

The order is different... And yes, I know that's nitpicking.

Best regards,
Piotr Sikora

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

Re: [PATCH] Core: detect valid error numbers at run-time

[Piotr Sikora]

Hey Maxim,

> This looks utterly wrong.  You are removing proper support for
> platforms with sys_nerr (including Linux and FreeBSD) and use
> instead the workaround designed to somehow work on other
> platforms.

Use of sys_nerr is deprecated, so it's not a perfect solution,
but fair enough, I'll rework this...

Best regards,
Piotr Sikora

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

Re: [PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[Piotr Sikora]

Hey Maxim,

> So the question remains: are there any real-world use cases?  May
> be someone will be able to provide some.

It's a chicken-and-egg problem. It's hard to use a feature that's not
supported by one of the most popular web servers.

> Without real-world use cases I don't think this should be added,
> as in general trailers is quite external concept to how nginx
> works and also may have various security implications.

Just to be clear, are you talking about HTTP/1.1 trailers or trailers
in general? The patch also includes HTTP/2 trailers and it's not clear
which one you don't like.

> Silently dropping trailers is what anyway happens if the client
> doesn't support chunked encoding at all (e.g., uses HTTP/1.0).
> And this is also what happens in your patch if there is no "TE:
> trailers".

Yes, but then it happens for a reason (client doesn't support
trailers) and not "just because".

> I think that whether to drop Content-Length and switch to chunked
> encoding is highly use-case specific question.  In some cases it
> may be appropriate, in some cases not, and in some cases one may
> want to add trailers even without "TE: trailers".  So forcing
> chunked encoding probably should be configured separately.  On the
> other hand, it's very hard to decide something given there are no
> real use cases known.

Why? I don't see anyone making a big deal out of forced chunked
encoding with "gzip on".

Best regards,
Piotr Sikora

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

[njs] Syntax error messages are more verbose and have line number.

[Igor Sysoev]

details:   http://hg.nginx.org/njs/rev/07d2be75a7db
branches:  
changeset: 120:07d2be75a7db
user:  Igor Sysoev 
date:  Wed Jul 13 13:56:12 2016 +0300
description:
Syntax error messages are more verbose and have line number.

diffstat:

 njs/njs_builtin.c |2 +
 njs/njs_generator.c   |   66 +++--
 njs/njs_lexer.c   |   23 -
 njs/njs_nonrecursive_parser.c |2 +-
 njs/njs_parser.c  |  187 -
 njs/njs_parser.h  |   16 +++
 njs/njs_parser_expression.c   |   35 ++-
 njs/njs_regexp.c  |   89 +++
 njs/njs_regexp.h  |3 +-
 njs/njs_string.c  |   60 +
 njs/njs_string.h  |2 -
 njs/njs_vm.c  |   24 +-
 njs/njs_vm.h  |7 +-
 njs/njscript.c|3 +-
 njs/test/njs_unit_test.c  |  100 +-
 15 files changed, 442 insertions(+), 177 deletions(-)

diffs (truncated from 1568 to 1000 lines):

diff -r 5e7e498eb90d -r 07d2be75a7db njs/njs_builtin.c
--- a/njs/njs_builtin.c Mon Jul 11 15:24:29 2016 +0300
+++ b/njs/njs_builtin.c Wed Jul 13 13:56:12 2016 +0300
@@ -19,6 +19,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #include 
 #include 
 #include 
diff -r 5e7e498eb90d -r 07d2be75a7db njs/njs_generator.c
--- a/njs/njs_generator.c   Mon Jul 11 15:24:29 2016 +0300
+++ b/njs/njs_generator.c   Wed Jul 13 13:56:12 2016 +0300
@@ -20,6 +20,13 @@
 #include 
 #include 
 #include 
+#include 
+
+
+typedef enum {
+NJS_GENERATOR_ERROR_ILLEGAL_CONTINUE = 0,
+NJS_GENERATOR_ERROR_ILLEGAL_BREAK,
+} njs_generator_error_t;
 
 
 static nxt_int_t njs_generator(njs_vm_t *vm, njs_parser_t *parser,
@@ -117,6 +124,8 @@ static nxt_noinline nxt_int_t njs_genera
 static nxt_noinline nxt_int_t njs_generator_index_release(njs_vm_t *vm,
 njs_parser_t *parser, njs_index_t index);
 nxt_inline nxt_bool_t njs_generator_is_constant(njs_parser_node_t *node);
+static nxt_int_t njs_generator_error(njs_vm_t *vm, njs_parser_node_t *node,
+njs_generator_error_t err);
 
 
 static const nxt_str_t  no_label = { 0, NULL };
@@ -1067,19 +1076,20 @@ njs_generate_continue_statement(njs_vm_t
 {
 njs_vmcode_jump_t   *jump;
 njs_parser_patch_t  *patch;
-
-if (parser->block == NULL) {
-vm->exception = _exception_syntax_error;
-return NXT_ERROR;
+njs_parser_block_t  *block;
+
+for (block = parser->block; block != NULL; block = block->next) {
+if (block->type == NJS_PARSER_LOOP) {
+goto found;
+}
 }
 
+return njs_generator_error(vm, node, NJS_GENERATOR_ERROR_ILLEGAL_CONTINUE);
+
+found:
+
 /* TODO: LABEL */
 
-if (parser->block->type != NJS_PARSER_LOOP) {
-vm->exception = _exception_syntax_error;
-return NXT_ERROR;
-}
-
 patch = nxt_mem_cache_alloc(vm->mem_cache_pool, 
sizeof(njs_parser_patch_t));
 
 if (nxt_fast_path(patch != NULL)) {
@@ -1105,12 +1115,20 @@ njs_generate_break_statement(njs_vm_t *v
 {
 njs_vmcode_jump_t   *jump;
 njs_parser_patch_t  *patch;
-
-if (parser->block == NULL) {
-vm->exception = _exception_syntax_error;
-return NXT_ERROR;
+njs_parser_block_t  *block;
+
+for (block = parser->block; block != NULL; block = block->next) {
+if (block->type == NJS_PARSER_LOOP
+|| block->type == NJS_PARSER_SWITCH)
+{
+goto found;
+}
 }
 
+return njs_generator_error(vm, node, NJS_GENERATOR_ERROR_ILLEGAL_BREAK);
+
+found:
+
 /* TODO: LABEL: loop and switch may have label, block must have label. */
 
 patch = nxt_mem_cache_alloc(vm->mem_cache_pool, 
sizeof(njs_parser_patch_t));
@@ -2466,3 +2484,25 @@ njs_generator_is_constant(njs_parser_nod
 return (node->token >= NJS_TOKEN_FIRST_CONST
 && node->token <= NJS_TOKEN_LAST_CONST);
 }
+
+
+static nxt_int_t
+njs_generator_error(njs_vm_t *vm, njs_parser_node_t *node,
+njs_generator_error_t err)
+{
+uint32_t size;
+const char   *msg;
+u_char   buf[NJS_EXCEPTION_BUF_LENGTH];
+
+static const char  *errors[] = {
+"SyntaxError: Illegal continue statement in %u",
+"SyntaxError: Illegal break statement in %u",
+};
+
+msg = errors[err];
+
+size = snprintf((char *) buf, NJS_EXCEPTION_BUF_LENGTH,
+msg, node->token_line);
+
+return njs_vm_throw_exception(vm, buf, size);
+}
diff -r 5e7e498eb90d -r 07d2be75a7db njs/njs_lexer.c
--- a/njs/njs_lexer.c   Mon Jul 11 15:24:29 2016 +0300
+++ b/njs/njs_lexer.c   Wed Jul 13 13:56:12 2016 +0300
@@ -47,7 +47,7 @@ static const uint8_t  njs_tokens[256]  n
 NJS_TOKEN_ILLEGAL,   NJS_TOKEN_ILLEGAL,
 /* \t */NJS_TOKEN_ILLEGAL,   NJS_TOKEN_SPACE,
 /* \n */NJS_TOKEN_LINE_END,  NJS_TOKEN_ILLEGAL,
-/* \r */NJS_TOKEN_ILLEGAL,