Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

deps: update c-ares to 1.23.0 #50800

Closed
wants to merge 2 commits into from
Closed

deps: update c-ares to 1.23.0 #50800

wants to merge 2 commits into from

Conversation

nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot commented Nov 18, 2023 •
edited
Loading

This is an automated update of c-ares to 1.23.0.

@nodejs-github-bot nodejs-github-bot added the dependencies Pull requests that update a dependency file. label Nov 18, 2023
@nodejs-github-bot
Copy link
Collaborator Author

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. needs-ci PRs that need a full CI run. labels Nov 18, 2023
@lpinca lpinca mentioned this pull request Nov 18, 2023
Closed
@lpinca
Copy link
Member

lpinca commented Nov 18, 2023

Depends on #50743.

@lpinca
Copy link
Member

lpinca commented Nov 19, 2023

Besides #50811, there is another issue here. It seems that calling dns.setServers() with an empty array no longer works as expected because

node/src/cares_wrap.cc

Line 1703 in e0bbe8f

int rv = ares_set_servers(channel->cares_channel(), nullptr);

returns an error after c-ares/c-ares@c8bd83a.

cc: @bradh352

@bradh352
Copy link
Contributor

What is the expected behavior here exactly? What's the use case for having zero servers configured? Previously if you did this in older c-ares versions, you'd make an inoperable channel ... and I wouldn't be surprised if you didn't get crashes with certain requests.

@lpinca
Copy link
Member

lpinca commented Nov 19, 2023

I'm not sure, the failing test was added 8 years ago. The code to handle the empty array special case, 10 years ago.

@lpinca
Copy link
Member

lpinca commented Nov 19, 2023 •
edited
Loading

I think it is used to reset servers. See also

node/test/parallel/test-dns.js

Lines 139 to 140 in 2059913

dns.setServers([]);
assert.deepStrictEqual(dns.getServers(), []);
.

@bradh352
Copy link
Contributor

yeah, that should have never been allowed. an ares channel with no servers won't do anything. I'll see if I can make it do something reasonable.

@bradh352
Copy link
Contributor

of course c-ares 1.22.1 is out now too :)

@lpinca
Copy link
Member

lpinca commented Nov 19, 2023

A public ares__destroy_servers_state() would be sufficient to fix the issue but if you have something more reasonable, then better.

@bradh352
Copy link
Contributor

I don't think its reasonable to try to zero out all servers for a library that needs servers. Even on initialization, if there is no system config, it will create a single server pointing to 127.0.0.1 as a last resort. I can't actually see any legitimate reason for nodejs to be attempting to do this at all and think its just something the test system thought might be a good idea.

@lpinca
Copy link
Member

lpinca commented Nov 19, 2023

It is also used in the wild https://github.com/mysql/mysql-connector-nodejs/blob/d6491ada6e50d21a9e2878acbca600d97a59daff/test/functional/default/connection/dns-srv.js#L180-L183 :(

bradh352 added a commit to c-ares/c-ares that referenced this pull request Nov 19, 2023
@bradh352
ares_set_servers_*() should allow an empty server list
320cefe 
For historic reasons, we have users depending on ares_set_servers_*()
to return ARES_SUCCESS when passing no servers and actually *clear*
the server list.  It appears they do this for test cases to simulate
DNS unavailable or similar.  Presumably they could achieve the same
effect in other ways (point to localhost on a port that isn't in use).
But it seems like this might be wide-spread enough to cause headaches
so we just will document and test for this behavior, clearly it hasn't
caused "issues" for anyone with the old behavior.

See: nodejs/node#50800

Fix By: Brad House (@bradh352)
@bradh352
Copy link
Contributor

bradh352 commented Nov 19, 2023 •
edited
Loading

Of course it is ... ugh. Ok, next c-ares release will restore the prior behavior. I've added docs for this behavior and test cases in c-ares with comments so it doesn't get reverted again in the future.

c-ares/c-ares@320cefe

@lpinca
Copy link
Member

lpinca commented Nov 19, 2023

Thank you.

@nodejs-github-bot nodejs-github-bot changed the title deps: update c-ares to 1.22.0 deps: update c-ares to 1.22.1 Nov 26, 2023
@nodejs-github-bot nodejs-github-bot changed the title deps: update c-ares to 1.22.1 deps: update c-ares to 1.23.0 Dec 3, 2023
@lpinca lpinca force-pushed the actions/tools-update-c-ares branch 2 times, most recently from 458d5b9 to 14b5230 Compare December 3, 2023 06:48
@lpinca lpinca force-pushed the actions/tools-update-c-ares branch from 14b5230 to 3626884 Compare December 3, 2023 07:24
@lpinca lpinca added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Dec 3, 2023
@lpinca lpinca mentioned this pull request Dec 3, 2023
Closed
@lpinca
Copy link
Member

lpinca commented Dec 3, 2023 •
edited
Loading

The failing test (parallel/test-dns-resolveany-bad-ancount.js) hangs because, to use c-ares defaults, we pass -1 as timeout, but after c-ares/c-ares@3b10e57 this no longer works as expected. It is now casted to an unsigned int.

bradh352 added a commit to c-ares/c-ares that referenced this pull request Dec 3, 2023
@bradh352
ares_init_options() with invalid options values should unset the option
c982bf4 
Apparently nodejs is relying on the above behavior for legacy reasons.  Add
sanity checks to the various optmask parameters where it makes sense.

See nodejs/node#50800

Fix By: Brad House (@bradh352)
@bradh352
Copy link
Contributor

bradh352 commented Dec 3, 2023

Hmm, well, that's unexpected. The optmask parameter is set to indicate you are setting a specific value and not using the default, but then you set a value of -1 that just so happened to work in prior versions to force it to use the default value anyhow because the value provided was invalid.

I've added sanity checks in c-ares/c-ares@c982bf4 that should restore the prior behavior (albeit in a slightly different way).

bradh352 added a commit to c-ares/c-ares that referenced this pull request Dec 4, 2023
@bradh352
ares_init_options() with invalid options values should unset the option
2656c38 
Apparently nodejs is relying on the above behavior for legacy reasons.  Add
sanity checks to the various optmask parameters where it makes sense.

See nodejs/node#50800

Fix By: Brad House (@bradh352)
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Dec 7, 2023
@0-wiz-0
libcares: update to 1.23.0.
5dd4594 
Version 1.23.0 (28 Nov 2023)

GitHub (28 Nov 2023)
- [Brad House brought this change]

  1.23.0 release prep (#641)

Brad House (28 Nov 2023)
- add missing manpage to distribution list

- clang-format

- remove a simply

- fix doc typo

- ares_init_options with ARES_OPT_UDP_PORT wrong byte order

  Regression from c-ares 1.19.1, ARES_OPT_UDP_PORT and ARES_OPT_TCP_PORT are
  specified from the user in host-byte order, but there was a regression that
  caused it to be read as if it was network byte order.

  Fixes Bug: #640
  Reported By: @Flow86
  Fix By: Brad House (@bradh352)

- fix ares_threadsafety() prototype

GitHub (28 Nov 2023)
- [Brad House brought this change]

  Basic Thread Safety (#636)

  c-ares does not have any concept of thread-safety. It has always been 100% up to the implementor to ensure they never call c-ares from more than one thread at a time. This patch adds basic thread-safety support, which can be disabled at compile time if not desired. It uses a single recursive mutex per channel, which should be extremely quick when uncontested so overhead should be minimal.

  Fixes Bug: #610

  Also sets the stage to implement #611

  Fix By: Brad House (@bradh352)

- [petrvh brought this change]

  ares_getaddrinfo(): do not use search domains if ARES_FLAG_NOSEARCH is set (#638)

  c-ares init options defines a flag ARES_FLAG_NOSEARCH that is supposed to prevent search using configured domain suffixes, however when using ares_getaddrinfo() the flag was ignored and domain suffixes were used anyway.

  Configuring zero domains to search also does not work (if ndomains == 0 default domain search list is loaded regardless of the flag ARES_OPT_DOMAINS being set).

  This change adds a check for the ARES_FLAG_NOSEARCH in as_is_only() function that is used by ares_getaddrinfo() to decide if to try to query next possible name ( next_dns_lookup() )

  Fix By: @petrvh

Brad House (25 Nov 2023)
- Fix MacOS version test

  It appears that the Issue #454 wasn't really fixed for some reason.  This commit should fix the detection.

  Fix By: Brad House (@bradh352)

Daniel Stenberg (24 Nov 2023)
- CI: codespell

  Closes #635

GitHub (24 Nov 2023)
- [Christian Clauss brought this change]

  Fix typos discovered by codespell (#634)

  % `codespell --ignore-words-list="aas,aci,acter,atleast,contentss,firey,fo,sais,seh,statics"`
  * https://pypi.org/project/codespell

  Fix By: Christian Clauss (@cclauss)

Brad House (22 Nov 2023)
- environment is meant as an override for sysconfig

GitHub (22 Nov 2023)
- [Ignat brought this change]

  Support attempts and timeout options from resolv.conf (#632)

  c-ares parses only antique version of options for timeout and number of retries from resolv.conf (`retrans` and `retry` are missing in modern documentation https://man7.org/linux/man-pages/man5/resolv.conf.5.html).

  I add support of `attempts` and `timeout` options

  Fix By: Ignat (@Kontakter)

- [Brad House brought this change]

  more precise timeout calculation (#633)

  The timeout calculation was occurring with millisecond precision, but on some systems, there is microsecond precision which could mean we'd tell a user a timeout time prior to the actual timeout.

  Fixes Bug: #631
  Fix By: Brad House (@bradh352)

- [Christian Clauss brought this change]

  INSTALL.md: Fix typo (#630)

  Fix By: Christian Clauss (@cclauss)

Brad House (19 Nov 2023)
- SonarCloud: fix minor codesmells

- fix test case regression due to missing parens

- now that warnings are enabled on test cases, clear a bunch of warnings

- CMake: CXXFLAGS environment wasn't being read because C++ compiler was enabled after settings warnings.

- fix additional windows test warnings

- cleanup some Windows warnings in test

- clang-format

GitHub (19 Nov 2023)
- [Brad House brought this change]

  Fix Windows UWP (Store) building and add to CI/CD (#627)

  When building for UWP (WindowsStore), additional headers are needed and some functions are not available. This also adds AppVeyor CI/CD support to catch these issues in the future.

  Fix By: Deal (@halx99) and Brad House (@bradh352)

Brad House (19 Nov 2023)
- ares_set_servers_*() should allow an empty server list

  For historic reasons, we have users depending on ares_set_servers_*()
  to return ARES_SUCCESS when passing no servers and actually *clear*
  the server list.  It appears they do this for test cases to simulate
  DNS unavailable or similar.  Presumably they could achieve the same
  effect in other ways (point to localhost on a port that isn't in use).
  But it seems like this might be wide-spread enough to cause headaches
  so we just will document and test for this behavior, clearly it hasn't
  caused "issues" for anyone with the old behavior.

  See: nodejs/node#50800

  Fix By: Brad House (@bradh352)

GitHub (19 Nov 2023)
- [Brad House brought this change]

  Query Cache support (#625)

  This PR implements a query cache at the lowest possible level, the actual dns request and response messages.  Only successful and `NXDOMAIN` responses are cached. The lowest TTL in the response message determines the cache validity period for the response, and is capped at the configuration value for `qcache_max_ttl`.  For `NXDOMAIN` responses, the SOA record is evaluated.

  For a query to match the cache, the opcode, flags, and each question's class, type, and name are all evaluated.  This is to prevent matching a cached entry for a subtly different query (such as if the RD flag is set on one request and not another).

  For things like ares_getaddrinfo() or ares_search() that may spawn multiple queries, each individual message received is cached rather than the overarching response.  This makes it possible for one query in the sequence to be purged from the cache while others still return cached results which means there is no chance of ever returning stale data.

  We have had a lot of user requests to return TTLs on all the various parsers like `ares_parse_caa_reply()`, and likely this is because they want to implement caching mechanisms of their own, thus this PR should solve those issues as well.

  Due to the internal data structures we have these days, this PR is less than 500 lines of new code.

  Fixes #608

  Fix By: Brad House (@bradh352)
@bradh352
Copy link
Contributor

c-ares 1.24.0 has been released with this fix.

@lpinca
Copy link
Member

lpinca commented Dec 18, 2023

I close this and manually trigger the action to open a new PR with the updated version.

@lpinca lpinca closed this Dec 18, 2023
@lpinca lpinca deleted the actions/tools-update-c-ares branch December 18, 2023 19:17
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Dec 30, 2023
@0-wiz-0
libcares: update to 1.24.0.
40b4588 
Version 1.24.0 (16 Dec 2023)

GitHub (16 Dec 2023)
- [Brad House brought this change]

  ares_cancel() could trigger callback with wrong response code (#663)

  When doing ares_gethostbyname() or ares_getaddrinfo() with AF_UNSPEC, if ares_cancel() was called after one address class was returned but before the other address class, it would return ARES_SUCCESS rather than ARES_ECANCELLED.

  Test case has been added for this specific condition.

  Fixes Bug: #662
  Fix By: Brad House (@bradh352)

- [Brad House brought this change]

  rand: allow fallback from OS (#661)

  getrandom() can fail with ENOSYS if the libc supports the function but the kernel does not.

  Fixes Bug: #660
  Fix By: Brad House (@bradh352)

- [Brad House brought this change]

  1.24.0 release prep (#657)

Brad House (11 Dec 2023)
- reference alternative to ares_getsock() in docs

- tag some functions as deprecated in docs

- Coverity: fix allocation size as reported in new code

- remove dead code: ares_iphlpapi.h

- remove dead code: bitncmp

GitHub (9 Dec 2023)
- [Brad House brought this change]

  Use external GoogleTest instead of bundling it (#655)

  GoogleTest should be unbundled. Google changed their guidance a few years back and modern versions of google test cannot build the bundling code file.

  This PR also updates to use C++14 as is required by modern GoogleTest versions.

  Fixes Bug: #506
  Fix By: Brad House (@bradh352)

Brad House (8 Dec 2023)
- use IF_NAMESIZE instead of IFNAMSIZ to avoid warning

- remove redundant cast

- clang-format and fix one warning

GitHub (8 Dec 2023)
- [Brad House brought this change]

  Clean up some Windows-only warnings (#654)

  Windows was emitting some warnings due to datatype differences.

  Fix By: Brad House (@bradh352)

- [Brad House brought this change]

  Rewrite sortlist hand parser for memory safety and bugs (#653)

  The parser for the sortlist has been rewritten to use the ares__buf_*() functions. This also resolves some known bugs in accepting invalid sortlist entries which should have caused parse failures.

  Fixes Bug: #501
  Fix By: Brad House (@bradh352)

Brad House (8 Dec 2023)
- enhance timeout test case to make sure it will re-use a previously downed server

- enhance timeout test case

- SonarCloud: make const

GitHub (7 Dec 2023)
- [Brad House brought this change]

  increment failures on timeout (#651)

  As of c-ares 1.22.0, server timeouts were erroneously not incrementing server failures meaning the server in use wouldn't rotate.  There was apparently never a test case for this condition.

  This PR fixes the bug and adds a test case to ensure it behaves properly.

  Fixes Bug: #650
  Fix By: Brad House (@bradh352)

- [Brad House brought this change]

  Windows UBSAN tests (#649)

  Fix UBSAN error, and enable UBSAN testing in AppVeyor.

  Fixes Bug #648
  Fix By: Gisle Vanem (@gvanem)

- [Brad House brought this change]

  Support ipv6 link-local servers and %iface syntax (#646)

  Some environments may send router advertisements on a link setting their link-local (fe80::/10) address as a valid DNS server to the remote system.  This will cause a DNS entry to be created like `fe80::1%iface`, since all link-local network interfaces are technically part of the same /10 subnet, it must be told what interface to send packets through explicitly if there are multiple physical interfaces.

  This PR adds support for the %iface modifier when setting DNS servers via `/etc/resolv.conf` as well as via `ares_set_servers_csv()`.

  For MacOS and iOS it is assumed that libresolve will set the `sin6_scope_id` and should be supported, but my test systems don't seem to read the Router Advertisement for RDNSS link-local.  Specifying the link-local dns server on MacOS via adig has been tested and confirmed working.

  For Windows, this is similar to MacOS in that the system doesn't seem to honor the RDNSS RA, but specifying manually has been tested to work.

  At this point, Android support does not exist.

  Fixes Bug #462
  Supersedes PR #463

  Fix By: Brad House (@bradh352) and Serhii Purik (@sergvpurik)

Brad House (4 Dec 2023)
- silence openwatcom warning due to qcache_max_ttl being unsigned

- ares__round_up_pow2() work around bogus warning

  On 32bit systems, a codeblock that would intentionally never
  be executed was emitting a warning.  Rework the code to
  prevent the warning.  More code, no behavior difference, but
  keeps people from complaining about the warning...

  Fixes Bug: #645
  Fix By: Brad House (@bradh352)

- try to move AC_USE_SYSTEM_EXTENSIONS

- Enable system extensions

  Certain defines are needed on some systems to enable functionality like
  pthread recursive mutexes.

  Fixes #644
  Fix By: Brad House (@bradh352)

- ares_init_options() with invalid options values should unset the option

  Apparently nodejs is relying on the above behavior for legacy reasons.  Add
  sanity checks to the various optmask parameters where it makes sense.

  See nodejs/node#50800

  Fix By: Brad House (@bradh352)

- SonarCloud: silence bogus reported error

- clang-format

GitHub (2 Dec 2023)
- [Brad House brought this change]

  Nameserver parsing: replace another hand-written parser (#643)

  This replaces the nameserver parsing code with code that use ares__buf_*() in the name of memory safety.

  Fix By: Brad House (@bradh352)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nodejs-github-bot @lpinca @bradh352