The previous implementation would redirect a query to a failed server
based on a timeout and random chance per query. This could lead to
issues of having to deal with server timeout scenarios when the server
isn't back online yet causing latency issues. Instead, we should
continue to use the known good servers for the query itself, but spawn a
second query with the same question to a different downed server. That
query will be able to be processed in the background and potentially
bring the server back online.
Also, when using the `rotate` option, servers were previously chosen at
random from the complete list. This PR changes that to choose only from
the servers that share the same highest priority.
Authored-By: Brad House (@bradh352)
Due to running containerized tests we had to cut and paste the `TEST_P`
macro definition in `googletest/include/gtest/gtest-param-test.h`, and
modify it as it isn't designed to be wrapped in any way. Unfortunately
this tends to change from release to release, usually in minor ways ...
but also google test specifically doesn't advertise its own version so
it can be hard to work around.
Lets try to fix compatibility with google test 1.15
Fixes#873
Authored-By: Brad House (@bradh352)
At some point in time, internal non-public functions were prefixed with
`ares__` where as functions that may become public were prefixed with
just `ares_`. This was never very consistent. Organizing the code better
typically provides more benefit in this way, which we've made great
progress on.
All non-static symbols must contain the `ares_` prefix, and that should
be more than sufficient in preventing any sort of issues.
Authored-By: Brad House (@bradh352)
Though the fuzzing tests are in the tests directory, we should move
the FUZZING.md file itself to the toplevel to make it more visible.
Signed-off-by: Brad House (@bradh352)
We modified the include path with recent changes, that isn't good for
OSS-Fuzz. Lets make the path relative for including some semi-public
headers to allow OSS-Fuzz to build again.
Fix By: Brad House (@bradh352)
The test case changes introduced a memory leak in the test itself
(not in c-ares). Fix this memory leak. Also move prior fuzzing
information into the new fuzzing document.
Fix By: Brad House (@bradh352)
The coverage of the fuzzer is fairly low as it was only testing the
parsing, not trying to read the data from the datastructure or writing
it back out as a DNS message. Lets extend, and also provide a
FUZZING.md to help people new to fuzzing get up and running.
Authored-By: Brad House (@bradh352)
We recently turned on a feature of SonarCloud to notify when the
ISO C limit of 31 character identifiers had been exceeded. This
commit fixes all non-public instances of such violations.
Authored-By: Brad House (@bradh352)
The legacy c-ares parsers such as ares_parse_txt_reply() and
ares_parse_srv_reply() historically all used different logic and
parsing.
As of c-ares 1.21.0 these are simply wrappers around a single parser, and
simply convert the parsed DNS response into the data structures the legacy
parsers used which is a small amount of code and not likely going to vary
based on the input data.
Instead, these days, it makes more sense to test the new parser directly
instead of calling it 10 or 11 times with the same input data to speed up
the number of iterations per second the fuzzer can perform.
We are keeping this legacy fuzzer test for historic reasons or if someone
finds them of use hidden behind a define of USE_LEGACY_PARSERS.
Authored-By: Brad House (@bradh352)
During compiling using MSVC and Ninja, its not uncommon to see errors
such as:
```
C:\projects\c-ares\test\ares_queryloop.c: fatal error C1041: cannot open program database 'C:\projects\c-ares\build\bin\vc140.pdb'; if multiple CL.EXE write to the same .PDB file, please use /FS
```
We see this as build failures in our AppVeyor runs.
The suggestion of using `/FS` is misleading as it already exists in the
compiler flags (and previous attempts to append them again were wrong).
The real issue I believe is that different output targets are being
compiled in parallel, but trying to use the same pdb file, but clearly
shouldn't be since they're different targets.
This CMake issue may be related:
https://gitlab.kitware.com/cmake/cmake/-/issues/20188
Authored-By: Brad House (@bradh352)
* ares__buf_split_str: Ability to split a buf object into a C array of C
strings
* ares__array_insertdata_{at,last,first}: copy data provided into array
member as a helper
* ares_free_array: Free a normal C array with custom callback to free
each element
Authored-By: Brad House (@bradh352)
systemd-resolved will return `ESERVFAIL` or `EREFUSED` by default on
single label domain names. See
https://github.com/systemd/systemd/issues/34101
They've basically labeled this as a non-issue even though it appears to
be in violation of the RFCs for downstream systems being able to support
negative caching. I haven't tested with the suggested
`ResolveUnicastSingleLabel=yes`, but that's off by default so its
unlikely people even knows this exists.
Since systemd is very prevalent these days, we really don't have much of
a choice but to work around their design decisions. This PR implements
this support, but *only* on single label names as it likely isn't valid
otherwise. It also adds test cases to confirm this behavior.
Fixes#852
Authored-By: Brad House (@bradh352)
As per #852 searching is failing, partially it is due to the ndots value
not defaulting to a proper value on linux, and partially due to
systemd-resolved returning the wrong error codes.
This PR fixes the first issue and adds containerized test cases to
validate the behavior and prevent issues in the future.
Reported-By: Hans-Christian Egtvedt (@egtvedt) and Mikael Lindemann(@mikaellindemann)
Authored-By: Brad House (@bradh352)
The tools like `adig` and `ahost` can take advantage of some of the
library functions within c-ares. We can't really split these library
functions into a helper library without possibly breaking users.
Technically if we could guarantee they rely on pkg-config or cmake
helpers we could make it work ... we can revisit that in the future,
maybe if we make a c-ares 2.0 where people might expect more breakage
(but I still wouldn't want to break API/ABI).
So what this does is it just exports some symbols in headers that aren't
distributed so end users aren't meant to use the symbols, but any
utilities or tests built by c-ares can. It does clutter up some of the
namespace, but all these symbols are guaranteed to be prefixed with
`ares_` so there shouldn't ever be symbol clashes due to this for end
users and its not so many symbols that it should affect any load times.
There will be **zero** API/ABI guarantees for these symbols. They can
change from release to release, this is ok since they are not public.
I'm not entirely thrilled with doing this solution, but I want to avoid
thing like hand-written parsers, such as is used in #856.
Authored-By: Brad House (@bradh352)
If a blank DNS name is used, the DNS query cache would fail due to an
invalid sanity check. This can be legitimate such as:
adig -t SOA .
This fixes that situation as well as a few other spots that were
uncovered and adds a test case to validate the behavior to ensure
it won't regress in the future.
Fixes#858
Reported-By: Nodar Chkuaselidze (@nodech)
Authored-By: Brad House (@bradh352)
On CI systems that can be overloaded, things like usleep() and select()
may not closely honor their timeouts, and can often be a multiple of the
requested timeout. Some tests, out of necessity need to rely on accurate
timing in order to test timeout conditions so this means test failures
when the skew is too large. Short of increasing timeouts to a point that
would make tests take an unreasonable amount of time, the alternative is
to make the OS honor the requested timeout more accurately.
On MacOS this means to set a realtime thread priority for the tests.
Other projects like libuv do this same thing. The code is taken from:
https://developer.apple.com/library/archive/technotes/tn2169/_index.html
Authored-By: Brad House (@bradh352)
GitHub actions supports running tests on various docker containers, move
Ubuntu 20.04 and Alpine tests to containers. Also move iOS testing to
GitHub actions since that runs on MacOS which is supported.
This should take additional load off of Cirrus-CI which consumes credits
like crazy. This leaves only FreeBSD and Linux ARM testing on Cirrus-CI.
Authored-By: Brad House (@bradh352)
Create a new data structure for a basic growable indexable array,
supporting the features you'd normally expect such as insert (at, last,
first), remove (at, last, first), and get (at, last, first). Internally
all data is stored in an appropriately sized array that can directly be
returned to the caller as a C array of the data type provided. The array
grows by powers of two, and has optimizations for head and tail
removals.
Array modifications can be risky (e.g. wrong reallocation sizes,
mis-sized memory moves, out-of-bounds access), so it makes sense to have
standardized code that is well tested.
The arrays used by the dns record parser / writer have been converted to
the new array data structure as have a few other instances.
Authored-By: Brad House (@bradh352)
TCP Fast Open (TFO) allows TCP connection establishment in 0-RTT when a
client and server have previously communicated. The SYN packet will also
contain the initial data packet from the client to the server. This
means there should be virtually no slowdown over UDP when both sides
support TCP FastOpen, which is unfortunately not always the case. For
instance, `1.1.1.1` appears to support TFO, however `8.8.8.8` does not.
This implementation supports Linux, Android, FreeBSD, MacOS, and iOS.
While Windows does have support for TCP FastOpen it does so via
completion APIs only, and that can't be used with polling APIs like used
by every other OS. We could implement it in the future if desired for
those using `ARES_OPT_EVENT_THREAD`, but it would probably require
adopting IOCP completely on Windows.
Sysctls are required to be set appropriately:
- Linux: `net.ipv4.tcp_fastopen`:
- `1` = client only (typically default)
- `2` = server only
- `3` = client and server
- MacOS: `net.inet.tcp.fastopen`
- `1` = client only
- `2` = server only
- `3` = client and server (typically default)
- FreeBSD: `net.inet.tcp.fastopen.server_enable` (boolean) and
`net.inet.tcp.fastopen.client_enable` (boolean)
This feature is always-on, when running on an OS with the capability
enabled. Though some middleboxes have impacted end-to-end TFO and caused
connectivity errors, all modern OSs perform automatic blackholing of IPs
that have issues with TFO. It is not expected this to cause any issues
in the modern day implementations.
This will also help with improving latency for future DoT and DoH
implementations.
Authored-By: Brad House (@bradh352)
Refactor some connection handling to reduce code duplication and to
unify the TCP and UDP codepaths a bit more. This will make some future
changes easier to make.
This also does some structure renaming to better conform with current
standards:
- `struct server_state` -> `ares_server_t`
- `struct server_connection` -> `ares_conn_t`
- `struct query` -> `ares_query_t`
Authored-by: Brad House (@bradh352)
During the implementation of server cookies, test cases were missing
to validate the server cookie in a prior reply was passed back, and
it turns out they were not.
This also adds tests for verification.
Fix By: Brad House (@bradh352)
DNS cookies are a simple form of learned mutual authentication supported
by most DNS server implementations these days and can help prevent DNS
Cache Poisoning attacks for clients and DNS amplification attacks for
servers.
Fixes#620
Fix By: Brad House (@bradh352)
c-ares is getting larger these days and we keep adding source files to
the same directory so it can be hard to differentiate core c-ares
implementation from library/utility functions. Lets make some
subdirectories to help with that and shuffle files around.
Fix By: Brad House (@bradh352)
UDP is connectionless, but systems use ICMP unreachable messages to
indicate there is no ability to reach the host or port, which can result
in a `send()` returning an error like `ECONNREFUSED`. We need to handle
non-retryable codes like that to treat it as a connection failure so we
requeue any queries on that connection to another connection/server
immediately. Otherwise what happens is we just wait on the timeout to
expire which can greatly increase the time required to get a definitive
message.
This also adds a test case to verify the behavior.
Fixes#819
Fix By: Brad Houes (@bradh352)
c-ares utilizes recursion for some operations, and some of these
processes can have unintended side effects, such as if a callback
is called that then recurses into the same function. This can cause
strange cleanup conditions that lead to crashes.
Try to disassociate queries with connections as early as possible and
move cleaning up unneeded connections to its own scan rather than
trying to detect each time a query is disassociated from a connection.
Fix By: Brad House (@bradh352)
In c-ares 1.30.0 we started validating strings parsed are printable.
This caused a regression in a pycares test case due to a wrong response
code being returned as the error was being propagated from a different
section of code that was assuming the only possible failure condition
was out-of-memory.
This PR adds a fix for this and also a test case to validate it.
Ref: https://github.com/saghul/pycares/issues/200
Fix By: Brad House (@bradh352)
We've had reports of user-after-free type crashes in Windows cleanup
code for the Event Thread. In evaluating the code, it appeared there
were some memory leaks on per-connection handles that may have remained
open during shutdown, while trying to resolve that it became apparent
the methodology chosen may not have been the right one for interfacing
with the Windows AFD system as stability issues were seen during this
debugging process.
Since this system is completely undocumented, there was no clear
resolution path other than to switch to the *other* methodology which
involves directly opening `\Device\Afd`, rather than spawning a "peer
socket" to use to queue AFD operations.
The original methodology chosen more closely resembled what is employed
by [libuv](https://github.com/libuv/libuv) and given its widespread use
was the reason it was used. The new methodology more closely resembles
[wepoll](https://github.com/piscisaureus/wepoll).
Its not clear if there are any scalability or performance advantages or
disadvantages for either method. They both seem like different ways to
do the same thing, but this current way does seem more stable.
Fixes#798
Fix By: Brad House (@bradh352)
There was a thread passed data for processing that was cleaned up before
thread exit, and it could cause a use-after-free in the test suite. This
doesn't affect c-ares. This was found during trying to reproduce #798,
but appears unrelated, don't use a helper thread as it isn't necessary.
Fix By: Brad House (@bradh352)
As per #738, there are usecases where the DNS TXT record strings should
not be concatenated like RFC 7208 indicates. We cannot break ABI with
those using the new API, so we need to support retrieving the
concatenated version as well as a new API to retrieve the individual
strings which will be used by `ares_parse_text_reply_ext()` to restore
the old behavior prior to c-ares 1.20.
Fixes Issue: #738
Fix By: Brad House (@bradh352)
This PR enables DNS 0x20 as per
https://datatracker.ietf.org/doc/html/draft-vixie-dnsext-dns0x20-00 .
DNS 0x20 adds additional entropy to the request by randomly altering the
case of the DNS question to help prevent cache poisoning attacks.
Google DNS has implemented this support as of 2023, even though this is
a proposed and expired standard from 2008:
https://groups.google.com/g/public-dns-discuss/c/KxIDPOydA5M
There have been documented cases of name server and caching server
non-conformance, though it is expected to become more rare, especially
since Google has started using this.
This can be enabled via the `ARES_FLAG_DNS0x20` flag, which is currently
disabled by default. The test cases do however enable this flag to
validate this feature.
Implementors using this flag will notice that responses will retain the
mixed case, but since DNS names are case-insensitive, any proper
implementation should not be impacted.
There is currently no fallback mechanism implemented as it isn't
immediately clear how this may affect a stub resolver like c-ares where
we aren't querying the authoritative name server, but instead an
intermediate recursive resolver where some domains may return invalid
results while others return valid results, all while querying the same
nameserver. Likely using DNS cookies as suggested by #620 is a better
mechanism to fight cache poisoning attacks for stub resolvers.
TCP queries do not use this feature even if the `ARES_FLAG_DNS0x20` flag
is specified since they are not subject to cache poisoning attacks.
Fixes Issue: #795
Fix By: Brad House (@bradh352)
The header inclusion logic in c-ares is hard to follow. Lets try to
simplify the way it works to make it easier to understand and less
likely to break on new code changes. There's still more work to be done,
but this is a good start at simplifying things.
Fix By: Brad House (@bradh352)
Brad House