> On 14 Jul 2022, at 17:12, Maxim Dounin <mdou...@mdounin.ru> wrote:
>
> Hello!
>
> On Wed, Jul 13, 2022 at 07:03:31PM +0400, Sergey Kandaurov wrote:
>
>>
>>> On 28 Jun 2022, at 20:25, Sergey Kandaurov <pluk...@nginx.com> wrote:
>>>
>>> # HG changeset patch
>>> # User Ruslan Ermilov <r...@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?
I agree in general, keeping checks should not affect behaviour,
while giving consistent codepath potentially useful in future.
>
> Note that we anyway check rn->query at least in
> ngx_resolver_process_a().
I believe it was made in the same sense of keeping the code in line.
>
>>> 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.
Ok.
>
>>> }
>>>
>>> #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 <pluk...@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.
I intend to commit this later today,
keeping it here for the reference.
# HG changeset patch
# User Sergey Kandaurov <pluk...@nginx.com>
# Date 1657808624 -14400
# Thu Jul 14 18:23:44 2022 +0400
# Node ID 06c74eef1cc3a249b73a4edb189e32d453b32fe8
# 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 06c74eef1cc3 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 Thu Jul 14 18:23:44 2022 +0400
@@ -3684,10 +3684,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) {
--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-le...@nginx.org
