Hi

> On 6 Apr 2024, at 12:32 AM, Sergey Kandaurov <pluknet@nginx.com> wrote:
>
> # HG changeset patch
> # User Sergey Kandaurov <pluknet@nginx.com>
> # Date 1712349104 -14400
> # Sat Apr 06 00:31:44 2024 +0400
> # Node ID 5fe21225ab3105aeea5e381a5d39ec1d3cfc04e6
> # Parent 47df39ea9e21c4359e0ca96dcc452f2bc8a82c44
> Detecting duplicate addresses in listen among different modules.
>
> This is used to gracefully reject the following configurations during
> syntax checking with a "duplicate ... address and port pair" error:
>
> http {
> server {
> listen 127.0.0.1:8080;
> ...
> }
> }
>
> stream {
> server {
> listen 127.0.0.1:8080;
> ...
> }
> }
>
> Also for wildcard addresses:
>
> stream {
> server {
> listen 8080;
> ...
> }
> }
>
> mail {
> server {
> listen 8080;
> ...
> }
> }
>
> Notably, this won't catch mixed wildcard and non-wildcard addresses as
> follows, where http server block with a specific address never accepts
> new connections.

Not sure what you mean mean by 'never accepts', but new connections surely
will be accepted by this server, just not directly. You probably mean there's
no designated listen socket for that address.

> The reason is that http will end up listening on the
> wildcard address, and stream is configured to listen on the specific
> address, which is technically a valid case.
>
> http {
> server {
> listen 127.0.0.1:8080;
> ...
> }
>
> server {
> listen 8080;
> ...
> }
> }
>
> stream {
> server {
> listen 127.0.0.1:8080;
> ...
> }
> }
>
> diff --git a/src/core/ngx_connection.c b/src/core/ngx_connection.c
> --- a/src/core/ngx_connection.c
> +++ b/src/core/ngx_connection.c
> @@ -21,10 +21,27 @@ ngx_create_listening(ngx_conf_t *cf, str
> socklen_t socklen)
> {
> size_t len;
> + ngx_uint_t i;
> ngx_listening_t *ls;
> struct sockaddr *sa;
> u_char text[NGX_SOCKADDR_STRLEN];
>
> + ls = cf->cycle->listening.elts;
> + for (i = 0; i < cf->cycle->listening.nelts; i++) {
> +
> + if (ngx_cmp_sockaddr(ls[i].sockaddr, ls[i].socklen,
> + sockaddr, socklen, 1)
> + == NGX_DECLINED)
> + {
> + continue;
> + }
> +
> + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> + "duplicate \"%V\" address and port pair",
> + &ls[i].addr_text);
> + return NULL;
> + }
> +
> ls = ngx_array_push(&cf->cycle->listening);
> if (ls == NULL) {
> return NULL;
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

I like the idea of fixing address collisions between modules. However, the patch does not fix the problem entirely and leaves
room for misconfigurations. IMHO we need to think how to properly fix this.

One way for fixing this is rejecting any overlapping listen addresses. We need to evaluate how likely it is that reasonable
configurations will be restricted by this. It doesn't seem likely to me.

----
Roman Arutyunyan
arut@nginx.com




_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel