Hello!

On Wed, Jul 13, 2022 at 07:03:31PM +0400, Sergey Kandaurov wrote:

>
> > On 28 Jun 2022, at 20:25, Sergey Kandaurov <pluknet@nginx.com> wrote:
> >
> > # HG changeset patch
> > # User Ruslan Ermilov <ru@nginx.com>
> > # Date 1645589317 -10800
> > # Wed Feb 23 07:08:37 2022 +0300
> > # Node ID 04e314eb6b4d20a48c5d7bab0609e1b03b51b406
> > # Parent fecd73db563fb64108f7669eca419badb2aba633
> > The "ipv4=" parameter of the "resolver" directive.
> >
> > When set to "off", only IPv6 addresses will be resolved, and no
> > A queries are ever sent (ticket #2196).
> >
> > diff -r fecd73db563f -r 04e314eb6b4d src/core/ngx_resolver.c
> > --- a/src/core/ngx_resolver.c Tue Jun 21 17:25:37 2022 +0300
> > +++ b/src/core/ngx_resolver.c Wed Feb 23 07:08:37 2022 +0300
> > @@ -157,6 +157,8 @@ ngx_resolver_create(ngx_conf_t *cf, ngx_
> > cln->handler = ngx_resolver_cleanup;
> > cln->data = r;
> >
> > + r->ipv4 = 1;
> > +
> > ngx_rbtree_init(&r->name_rbtree, &r->name_sentinel,
> > ngx_resolver_rbtree_insert_value);
> >
> > @@ -225,6 +227,23 @@ ngx_resolver_create(ngx_conf_t *cf, ngx_
> > }
> >
> > #if (NGX_HAVE_INET6)
> > + if (ngx_strncmp(names[i].data, "ipv4=", 5) == 0) {
> > +
> > + if (ngx_strcmp(&names[i].data[5], "on") == 0) {
> > + r->ipv4 = 1;
> > +
> > + } else if (ngx_strcmp(&names[i].data[5], "off") == 0) {
> > + r->ipv4 = 0;
> > +
> > + } else {
> > + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> > + "invalid parameter: %V", &names[i]);
> > + return NULL;
> > + }
> > +
> > + continue;
> > + }
> > +
> > if (ngx_strncmp(names[i].data, "ipv6=", 5) == 0) {
> >
> > if (ngx_strcmp(&names[i].data[5], "on") == 0) {
> > @@ -273,6 +292,14 @@ ngx_resolver_create(ngx_conf_t *cf, ngx_
> > }
> > }
> >
> > +#if (NGX_HAVE_INET6)
> > + if (r->ipv4 + r->ipv6 == 0) {
> > + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> > + "\"ipv4\" and \"ipv6\" cannot both be \"off\"");
> > + return NULL;
> > + }
> > +#endif
> > +
> > if (n && r->connections.nelts == 0) {
> > ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, "no name servers defined");
> > return NULL;
> > @@ -836,7 +863,7 @@ ngx_resolve_name_locked(ngx_resolver_t *
> > r->last_connection = 0;
> > }
> >
> > - rn->naddrs = (u_short) -1;
> > + rn->naddrs = r->ipv4 ? (u_short) -1 : 0;
> > rn->tcp = 0;
> > #if (NGX_HAVE_INET6)
> > rn->naddrs6 = r->ipv6 ? (u_short) -1 : 0;
> > @@ -1263,7 +1290,7 @@ ngx_resolver_send_query(ngx_resolver_t *
> > rec->log.action = "resolving";
> > }
> >
> > - if (rn->naddrs == (u_short) -1) {
> > + if (rn->query && rn->naddrs == (u_short) -1) {
>
> It should be safe to revert this condition:
> for PTR and SRV queries, rn->query is always set;
> for A queries, it is additionally protected with rn->naddrs,
> which by itself is set to (u_short) -1 only for r->ipv4 == 1.
> See below for rationale.

Wouldn't it be better to keep the rn->query check, simply to keep
the code in line with the r->query6 case below?

Note that we anyway check rn->query at least in
ngx_resolver_process_a().

> > rc = rn->tcp ? ngx_resolver_send_tcp_query(r, rec, rn->query, rn->qlen)
> > : ngx_resolver_send_udp_query(r, rec, rn->query, rn->qlen);
> >
> > @@ -1765,10 +1792,13 @@ ngx_resolver_process_response(ngx_resolv
> > q = ngx_queue_next(q))
> > {
> > rn = ngx_queue_data(q, ngx_resolver_node_t, queue);
> > - qident = (rn->query[0] << 8) + rn->query[1];
> > -
> > - if (qident == ident) {
> > - goto dns_error_name;
> > +
> > + if (rn->query) {
> > + qident = (rn->query[0] << 8) + rn->query[1];
> > +
> > + if (qident == ident) {
> > + goto dns_error_name;
> > + }
>
> This one also looks save to revert.
> This code is used to check ident match for the FORMERR case.
> For ipv4=off case, with the below part reverted, both rn->query
> and rn->query6 will look at the same address, so on ident mismatch
> both checks for rn->query and rn->query6 just duplicate each other.

Same here.

> > }
> >
> > #if (NGX_HAVE_INET6)
> > @@ -3645,7 +3675,7 @@ ngx_resolver_create_name_query(ngx_resol
> > len = sizeof(ngx_resolver_hdr_t) + nlen + sizeof(ngx_resolver_qs_t);
> >
> > #if (NGX_HAVE_INET6)
> > - p = ngx_resolver_alloc(r, r->ipv6 ? len * 2 : len);
> > + p = ngx_resolver_alloc(r, len * (r->ipv4 + r->ipv6));
> > #else
> > p = ngx_resolver_alloc(r, len);
> > #endif
> > @@ -3654,23 +3684,28 @@ ngx_resolver_create_name_query(ngx_resol
> > }
> >
> > rn->qlen = (u_short) len;
> > - rn->query = p;
> > +
> > + if (r->ipv4) {
> > + rn->query = p;
> > + }
>
> It turns out that doing conditional allocation prevents from
> memory deallocation using "ngx_resolver_free(r, rn->query);" idiom.
> Reverting this part and accompanying changes for rn->query
> seems to be enough to fix this.
> See above for more details, and the patch below.
>
> >
> > #if (NGX_HAVE_INET6)
> > if (r->ipv6) {
> > - rn->query6 = p + len;
> > + rn->query6 = r->ipv4 ? (p + len) : p;
> > }
> > #endif
> >
> > query = (ngx_resolver_hdr_t *) p;
> >
> > - ident = ngx_random();
> > -
> > - ngx_log_debug2(NGX_LOG_DEBUG_CORE, r->log, 0,
> > - "resolve: \"%V\" A %i", name, ident & 0xffff);
> > -
> > - query->ident_hi = (u_char) ((ident >> 8) & 0xff);
> > - query->ident_lo = (u_char) (ident & 0xff);
> > + if (r->ipv4) {
> > + ident = ngx_random();
> > +
> > + ngx_log_debug2(NGX_LOG_DEBUG_CORE, r->log, 0,
> > + "resolve: \"%V\" A %i", name, ident & 0xffff);
> > +
> > + query->ident_hi = (u_char) ((ident >> 8) & 0xff);
> > + query->ident_lo = (u_char) (ident & 0xff);
> > + }
> >
> > /* recursion query */
> > query->flags_hi = 1; query->flags_lo = 0;
> > @@ -3731,7 +3766,9 @@ ngx_resolver_create_name_query(ngx_resol
> >
> > p = rn->query6;
> >
> > - ngx_memcpy(p, rn->query, rn->qlen);
> > + if (r->ipv4) {
> > + ngx_memcpy(p, rn->query, rn->qlen);
> > + }
> >
> > query = (ngx_resolver_hdr_t *) p;
> >
> > diff -r fecd73db563f -r 04e314eb6b4d src/core/ngx_resolver.h
> > --- a/src/core/ngx_resolver.h Tue Jun 21 17:25:37 2022 +0300
> > +++ b/src/core/ngx_resolver.h Wed Feb 23 07:08:37 2022 +0300
> > @@ -175,8 +175,10 @@ struct ngx_resolver_s {
> > ngx_queue_t srv_expire_queue;
> > ngx_queue_t addr_expire_queue;
> >
> > + unsigned ipv4:1;
> > +
> > #if (NGX_HAVE_INET6)
> > - ngx_uint_t ipv6; /* unsigned ipv6:1; */
> > + unsigned ipv6:1;
> > ngx_rbtree_t addr6_rbtree;
> > ngx_rbtree_node_t addr6_sentinel;
> > ngx_queue_t addr6_resend_queue;
>
> # HG changeset patch
> # User Sergey Kandaurov <pluknet@nginx.com>
> # Date 1657724523 -14400
> # Wed Jul 13 19:02:03 2022 +0400
> # Node ID 61fa6c872c85b54ce41af8748ffde933dbaae47e
> # Parent 2a77754cd9feae752152e8eef7e5c506dd0186d6
> Resolver: fixed memory leak for the "ipv4=off" case.
>
> This change partially reverts 2a77754cd9fe to properly free rn->query.
>
> diff -r 2a77754cd9fe -r 61fa6c872c85 src/core/ngx_resolver.c
> --- a/src/core/ngx_resolver.c Tue Jul 12 21:44:02 2022 +0400
> +++ b/src/core/ngx_resolver.c Wed Jul 13 19:02:03 2022 +0400
> @@ -1290,7 +1290,7 @@ ngx_resolver_send_query(ngx_resolver_t *
> rec->log.action = "resolving";
> }
>
> - if (rn->query && rn->naddrs == (u_short) -1) {
> + if (rn->naddrs == (u_short) -1) {
> rc = rn->tcp ? ngx_resolver_send_tcp_query(r, rec, rn->query, rn->qlen)
> : ngx_resolver_send_udp_query(r, rec, rn->query, rn->qlen);
>
> @@ -1792,13 +1792,10 @@ ngx_resolver_process_response(ngx_resolv
> q = ngx_queue_next(q))
> {
> rn = ngx_queue_data(q, ngx_resolver_node_t, queue);
> -
> - if (rn->query) {
> - qident = (rn->query[0] << 8) + rn->query[1];
> -
> - if (qident == ident) {
> - goto dns_error_name;
> - }
> + qident = (rn->query[0] << 8) + rn->query[1];
> +
> + if (qident == ident) {
> + goto dns_error_name;
> }
>
> #if (NGX_HAVE_INET6)
> @@ -3684,10 +3681,7 @@ ngx_resolver_create_name_query(ngx_resol
> }
>
> rn->qlen = (u_short) len;
> -
> - if (r->ipv4) {
> - rn->query = p;
> - }
> + rn->query = p;
>
> #if (NGX_HAVE_INET6)
> if (r->ipv6) {

See above for comments, otherwise looks good.

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org