Service for getaddrinfo by ki11roy · Pull Request #257 · c-ares/c-ares

ki11roy · 2019-05-19T23:31:41Z

This branch is a development of #232.
Some major changes are support of search domains and combining of ares_parse_a/aaaa_reply into ares__parse_into_addrinfo, though I haven't touched the original functions yet.
I had to introduce additional flag ARES_AI_NOSORT to disable connecting to mock addresses while sorting. My first idea was to use ares_socket_functions, but there is no "getsockname" mock, so flag seems a better variant.
Struct ares_addrinfo extended to keep ai_ttl and ai_cname_ttl.
The latest ares_getaddrinfo fixes are not actual anymore, because the code is removed.

coveralls · 2019-05-19T23:54:32Z

Coverage increased (+0.1%) to 89.573% when pulling 6426f1f on ki11roy:service_for_getaddrinfo into ba1aa06 on c-ares:master.

bradh352 · 2019-05-21T11:44:57Z

Before I do a full review on this, can you go ahead and clean up the legacy code? Think you referred to ares_parse_a/aaaa_reply, probably those just need to be made into wrappers right?

ki11roy · 2019-05-21T15:15:19Z

@bradh352 sure

ki11roy · 2019-05-21T22:57:49Z

I have tried and there is a problem. I need to add aliases to ares_addrinfo to completely replace the functions, but it seems too much, because there are already two extra fields ai_ttl and ai_cname_ttl (and ai_cname itself). So there are two separate parts which badly fit into the same struct.
I thought about moving from linked list as in classic addrinfo to more like something like good old hostent with cname, aliases, cname_ttl and a linked list of addrinfo with ttl’s. How about this?

bradh352 · 2019-05-22T00:09:53Z

Again, I wouldn't worry about standards conformance since we're already making modifications to the struct and it is namespaced. And memory these days is cheap, especially what we're talking about here.

I also think its probably silly for getaddrinfo to return technically less information than gethostbyname in any area at all.

My vote would be to make the ai_canonname an array structure holding the name and ttl

`
struct ares_addrinfo_cname {
char *name;
int ttl;
};

struct ares_addrinfo {
...
struct ares_addrinfo_cname *ai_canonname;
size_t ai_canonname_len;
...
};
`

ki11roy · 2019-05-22T23:07:00Z

I need to fix the tests and wrap the functions, finally.

bradh352 · 2019-05-22T23:24:26Z

Hmm, I'd think the cname + ttl struct would be an array since the recursive DNS server being queried could return multiple CNAMEs which are the aliases. I'm not sure why you have a char **aliases at all.

We might as well save the ttl for each if we're already saving the cname for each.

bradh352 · 2019-05-22T23:25:05Z

-  char                 *ai_canonname;
-  struct sockaddr      *ai_addr;
-  struct ares_addrinfo *ai_next;
+  struct ares_addrinfo_cname     cname;


make this an array, also need a length member for the array length. Or for consistency, I guess it could be a NULL-term linked list.

bradh352 · 2019-05-22T23:25:22Z

-  struct sockaddr      *ai_addr;
-  struct ares_addrinfo *ai_next;
+  struct ares_addrinfo_cname     cname;
+  char                       **aliases;


remove this

bradh352 · 2019-05-22T23:27:23Z

+          /* Take the min of the TTLs we see in the CNAME chain. */
+          if (ai->cname.ttl > rr_ttl)
+            ai->cname.ttl = rr_ttl;
+


no reason to record a minimum, instead record the ttl associated with the cname entry in the array

bradh352 · 2019-05-22T23:28:02Z

+          if (ai->cname.ttl > rr_ttl)
+            ai->cname.ttl = rr_ttl;
+
+          ares_free(ai->cname.name);


No need to free prior cname, should be a new array entry

ki11roy · 2019-05-23T01:21:21Z

Got your point, but for the aliases I need them to populate hostent structure in ares_parse_a_reply. I see no similar structure in regular addrinfo and it is not CNAME itself, but a label of CNAME as per RFC2181 section 10.1.1. So I wanted to have labels as aliases and one value (multiple values as you suggested) as CNAME (that part is unfinished).

ki11roy · 2019-06-16T22:22:25Z

Fixed the tests and replaced ares_parse_a/aaaa_reply with ares__parse_into_addrinfo. It is private, but considering #238 and #259 I think it is would be better to make it public?

There are some new structures, like:

/*
 * alias - label of the resource record.
 * name - value (canonical name) of the resource record.
 * See RFC2181 10.1.1. CNAME terminology.
 */
struct ares_addrinfo_cname {
  int                         ttl;
  char                       *alias;
  char                       *name;
  struct ares_addrinfo_cname *next;
};

I need the structure to populate hostent as it wants both aliases and canonical name in ares_parse_a/aaaa_record.

bradh352 · 2019-06-16T23:32:09Z

I'm inclined to merge this and deal with any tweaks or fallout later. It looks pretty good and provides a very nice implementation with more features (mainly ttls) than a native getaddrinfo which is nice.

@bagder any problems with this?

daviddrysdale · 2019-06-19T08:33:06Z

Looks like there's a Windows build failure with this (AppVeyor):

	cl.exe /nologo /MDd /D_DEBUG /Od /Zi /D_CRT_NONSTDC_NO_DEPRECATE /D_CRT_SECURE_NO_WARNINGS /RTCsu /DWIN32 /I. /I.. /I gmock-1.8.0 /W3 /EHsc /FD /DCARES_STATICLIB /Fo.\msvc\arestest\lib-debug\obj\ares-test-internal.obj /Fd.\msvc\arestest\lib-debug\obj\ /c .\ares-test-internal.cc
ares-test-internal.cc
C:\Program Files (x86)\Windows Kits\8.1\include\shared\winerror.h(26345) : warning C4005: 'NOERROR' : macro redefinition
        C:\projects\c-ares\nameser.h(144) : see previous definition of 'NOERROR'
.\ares-test-internal.cc(366) : error C2065: 'EnvValue' : undeclared identifier
.\ares-test-internal.cc(366) : error C2146: syntax error : missing ';' before identifier 'with_env'
.\ares-test-internal.cc(366) : error C3861: 'with_env': identifier not found
.\ares-test-internal.cc(386) : error C2065: 'EnvValue' : undeclared identifier
.\ares-test-internal.cc(386) : error C2146: syntax error : missing ';' before identifier 'with_env'
.\ares-test-internal.cc(386) : error C3861: 'with_env': identifier not found
.\ares-test-internal.cc(406) : error C2065: 'EnvValue' : undeclared identifier
.\ares-test-internal.cc(406) : error C2146: syntax error : missing ';' before identifier 'with_env'
.\ares-test-internal.cc(406) : error C3861: 'with_env': identifier not found
.\ares-test-internal.cc(426) : error C2065: 'EnvValue' : undeclared identifier
.\ares-test-internal.cc(426) : error C2146: syntax error : missing ';' before identifier 'with_env'
.\ares-test-internal.cc(426) : error C3861: 'with_env': identifier not found
.\ares-test-internal.cc(575) : warning C4267: 'argument' : conversion from 'size_t' to 'int', possible loss of data
NMAKE : fatal error

daviddrysdale · 2019-06-19T09:17:30Z

Also, I'm getting some OSS-Fuzz failure reports from this, in a couple of variants:

ares_parse_a_reply.c:160 has a WRITE stack buffer overflow – can we check all the count tests (e.g. l155) are correct?
ares_parse_aaaa_reply.c:156 has a READ heap buffer overflow.

ki11roy · 2019-06-19T11:22:13Z

@daviddrysdale thanks! i'll check it.

bradh352 · 2019-06-19T12:17:08Z

Looks like there's a Windows build failure with this (AppVeyor):

	cl.exe /nologo /MDd /D_DEBUG /Od /Zi /D_CRT_NONSTDC_NO_DEPRECATE /D_CRT_SECURE_NO_WARNINGS /RTCsu /DWIN32 /I. /I.. /I gmock-1.8.0 /W3 /EHsc /FD /DCARES_STATICLIB /Fo.\msvc\arestest\lib-debug\obj\ares-test-internal.obj /Fd.\msvc\arestest\lib-debug\obj\ /c .\ares-test-internal.cc
ares-test-internal.cc
C:\Program Files (x86)\Windows Kits\8.1\include\shared\winerror.h(26345) : warning C4005: 'NOERROR' : macro redefinition
        C:\projects\c-ares\nameser.h(144) : see previous definition of 'NOERROR'
.\ares-test-internal.cc(366) : error C2065: 'EnvValue' : undeclared identifier
.\ares-test-internal.cc(366) : error C2146: syntax error : missing ';' before identifier 'with_env'
.\ares-test-internal.cc(366) : error C3861: 'with_env': identifier not found
.\ares-test-internal.cc(386) : error C2065: 'EnvValue' : undeclared identifier
.\ares-test-internal.cc(386) : error C2146: syntax error : missing ';' before identifier 'with_env'
.\ares-test-internal.cc(386) : error C3861: 'with_env': identifier not found
.\ares-test-internal.cc(406) : error C2065: 'EnvValue' : undeclared identifier
.\ares-test-internal.cc(406) : error C2146: syntax error : missing ';' before identifier 'with_env'
.\ares-test-internal.cc(406) : error C3861: 'with_env': identifier not found
.\ares-test-internal.cc(426) : error C2065: 'EnvValue' : undeclared identifier
.\ares-test-internal.cc(426) : error C2146: syntax error : missing ';' before identifier 'with_env'
.\ares-test-internal.cc(426) : error C3861: 'with_env': identifier not found
.\ares-test-internal.cc(575) : warning C4267: 'argument' : conversion from 'size_t' to 'int', possible loss of data
NMAKE : fatal error

Looks like EnvValue is conditionalized against Windows for some reason. I've removed that condition and fixed the build.

ki11roy · 2019-06-20T00:51:05Z

Also, I'm getting some OSS-Fuzz failure reports from this, in a couple of variants:

ares_parse_a_reply.c:160 has a WRITE stack buffer overflow – can we check all the count tests (e.g. l155) are correct?

ares_parse_aaaa_reply.c:156 has a READ heap buffer overflow.

#262 should fix this one

Suspect problems were introduced by 7d3591e ("getaddrinfo enhancements (c-ares#257)"), and revolve around: - ares_parse_a_reply.c:160 : WRITE stack buffer overflow - ares_parse_aaaa_reply.c:156 : READ heap buffer overflow. Reproduce by building with ASAN enabled and running (e.g.): ./test/aresfuzz ./test/fuzzinput/clusterfuzz-5650695891451904

Suspect problems were introduced by 7d3591e ("getaddrinfo enhancements (c-ares#257)"), and include: - ares_parse_a_reply.c:170 : WRITE stack buffer overflow - ares_parse_aaaa_reply.c:170,172 : WRITE stack buffer overflow - ares_parse_a_reply:149 : leak - ares_parse_aaaa_reply.c:151 : leak Reproduce by building with ASAN enabled and running (e.g.): ./test/aresfuzz ./test/fuzzinput/clusterfuzz-5650695891451904

ChristianAmmer-zz · 2019-07-01T20:36:25Z

Adding "Service for ares_getaddrinfo" also changed the order search domains for T_A and T_AAAA requests are tried (If I'm not wrong).

Before:
T_AAAA for host.suffix1 and parallel T_A for host.suffix1
T_AAAA for host.suffix2 and parallel T_A for host.suffix2

Now:
T_AAAA for host.suffix1
T_AAAA for host.suffix2
T_A for host.suffix1
T_A for host.suffix2

This slows down the entire lookup. In comparison to this, glibc uses parallel searches with the intention of not slowing down.

bradh352 · 2019-07-10T12:16:19Z

@ChristianAmmer I agree that queries for ipv4 & ipv6 should be done in parallel

ki11roy · 2019-07-10T16:45:04Z

As far as I remember the initial purpose of that code was to fixing the incorrect behavior, which was done in mostly invasive way. So I just fixed it the other way. Everything above this is an optimization. I do agree with the statement about the parallel queries, but in my opinion the first step is fixing the code (its done) and the second is implementing the optimization. I will think on how to return it in the most painless way.

@ki11roy

* Service support has been added to getaddrinfo. * ares_parse_a/aaaa_record now share code with the addrinfo parser. * Private ares_addrinfo structure with useful extensions such as ttls (including cname ttls), as well as the ability to list multiple cnames in chain of lookups Work By: Andrew Selivanov @ki11roy

ChristianAmmer-zz · 2019-11-06T19:22:36Z

Hi @bradh352 @ki11roy
I was working on a related issue today, this remembered me on this one. To not forget it, I created #285.

@ki11roy

* Service support has been added to getaddrinfo. * ares_parse_a/aaaa_record now share code with the addrinfo parser. * Private ares_addrinfo structure with useful extensions such as ttls (including cname ttls), as well as the ability to list multiple cnames in chain of lookups Work By: Andrew Selivanov @ki11roy

@mmomtchev

If there are more ttls returned than the maximum provided by the requestor, then the *naddrttls response would be larger than the actual number of elements in the addrttls array. This bug could lead to invalid memory accesses in applications using c-ares. This behavior appeared to break with PR #257 Fixes: #371 Reported By: Momtchil Momtchev (@mmomtchev) Fix By: Brad House (@bradh352)

@mmomtchev

Original commit message: If there are more ttls returned than the maximum provided by the requestor, then the *naddrttls response would be larger than the actual number of elements in the addrttls array. This bug could lead to invalid memory accesses in applications using c-ares. This behavior appeared to break with PR c-ares/c-ares#257 Fixes: c-ares/c-ares#371 Reported By: Momtchil Momtchev (@mmomtchev) Fix By: Brad House (@bradh352) Refs: https://github.com/nodejs/node/issues/36063 Signed-off-by: Michael Dawson <[email protected]> CVE-ID: CVE-2020-8277 PR-URL: nodejs-private/node-private#231 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Beth Griggs <[email protected]>

@mmomtchev

If there are more ttls returned than the maximum provided by the requestor, then the *naddrttls response would be larger than the actual number of elements in the addrttls array. This bug could lead to invalid memory accesses in applications using c-ares. This behavior appeared to break with PR c-ares#257 Fixes: c-ares#371 Reported By: Momtchil Momtchev (@mmomtchev) Fix By: Brad House (@bradh352)

@mmomtchev

Original commit message: If there are more ttls returned than the maximum provided by the requestor, then the *naddrttls response would be larger than the actual number of elements in the addrttls array. This bug could lead to invalid memory accesses in applications using c-ares. This behavior appeared to break with PR c-ares/c-ares#257 Fixes: c-ares/c-ares#371 Reported By: Momtchil Momtchev (@mmomtchev) Fix By: Brad House (@bradh352) Refs: https://github.com/nodejs/node/issues/36063 Signed-off-by: Michael Dawson <[email protected]> CVE-ID: CVE-2020-8277 PR-URL: nodejs-private/node-private#231 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Beth Griggs <[email protected]>

@mmomtchev

Original commit message: If there are more ttls returned than the maximum provided by the requestor, then the *naddrttls response would be larger than the actual number of elements in the addrttls array. This bug could lead to invalid memory accesses in applications using c-ares. This behavior appeared to break with PR c-ares/c-ares#257 Fixes: c-ares/c-ares#371 Reported By: Momtchil Momtchev (@mmomtchev) Fix By: Brad House (@bradh352) Refs: https://github.com/nodejs/node/issues/36063 Signed-off-by: Michael Dawson <[email protected]> CVE-ID: CVE-2020-8277 PR-URL: nodejs-private/node-private#231 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Beth Griggs <[email protected]>

ki11roy added 9 commits April 11, 2019 21:57

getaddrinfo with service

bf23054

more code moved

0dac3f9

more code moved

9b17efb

parse_into_addrinfo and other changes

c3e58af

sort all the results

e5c6532

remove extra

3adfcfa

Merge branch 'master' into service_for_getaddrinfo

b9e0ec9

fix hostname leak

9c66f92

fix analyzer

3a30657

ki11roy mentioned this pull request May 19, 2019

Support for ares getaddrinfo #232

Closed

Merge branch 'master' into service_for_getaddrinfo

c573235

ki11roy mentioned this pull request May 21, 2019

Add TTL in ares_gethostbyname callback #259

Closed

switch to alternative ares_addrinfo, tests are broken

99eb7f2

bradh352 reviewed May 22, 2019

View reviewed changes

parse partially implemented, tests are broken

e2a74aa

yxue mentioned this pull request Jun 7, 2019

dns: adjust DNS refresh rate respecting to record TTL envoyproxy/envoy#6975

Merged

got parse to work

6426f1f

bradh352 merged commit 7d3591e into c-ares:master Jun 18, 2019

This was referenced Jun 18, 2019

Some member variables of ares_addrinfo not set when using ares_getaddrinfo. #247

Closed

Cannot parse TTL of a CNAME query #238

Closed

daviddrysdale mentioned this pull request Jun 20, 2019

Add fuzzer input files that trigger problems #263

Closed

ChristianAmmer-zz mentioned this pull request Nov 6, 2019

Regression in ares_getaddrinfo on the A and AAAA query order. #285

Closed

shmilylty mentioned this pull request Mar 9, 2020

It's exciting that the c-ares library supports TTL! saghul/pycares#123

Closed

bradh352 mentioned this pull request Nov 12, 2020

ares_parse_a_reply / ares_parse_aaaa_reply seem to have new behavior #371

Closed

dimbleby mentioned this pull request Oct 26, 2023

ESERVICE is undocumented #586

Closed

Conversation

ki11roy commented May 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented May 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradh352 commented May 21, 2019

Uh oh!

ki11roy commented May 21, 2019

Uh oh!

ki11roy commented May 21, 2019

Uh oh!

bradh352 commented May 22, 2019

Uh oh!

ki11roy commented May 22, 2019

Uh oh!

bradh352 commented May 22, 2019

Uh oh!

bradh352 May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradh352 May 22, 2019

Choose a reason for hiding this comment

Uh oh!

bradh352 May 22, 2019

Choose a reason for hiding this comment

Uh oh!

bradh352 May 22, 2019

Choose a reason for hiding this comment

Uh oh!

ki11roy commented May 23, 2019

Uh oh!

ki11roy commented Jun 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradh352 commented Jun 16, 2019

Uh oh!

daviddrysdale commented Jun 19, 2019

Uh oh!

daviddrysdale commented Jun 19, 2019

Uh oh!

ki11roy commented Jun 19, 2019

Uh oh!

bradh352 commented Jun 19, 2019

Uh oh!

ki11roy commented Jun 20, 2019

Uh oh!

ChristianAmmer-zz commented Jul 1, 2019

Uh oh!

bradh352 commented Jul 10, 2019

Uh oh!

ki11roy commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChristianAmmer-zz commented Nov 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ki11roy commented May 19, 2019 •

edited

Loading

coveralls commented May 19, 2019 •

edited

Loading

bradh352 May 22, 2019 •

edited

Loading

ki11roy commented Jun 16, 2019 •

edited

Loading

ki11roy commented Jul 10, 2019 •

edited

Loading