The dns_rrl function's return type, dns_rrl_result_t, is an enumerated
type defined in file lib/dns/include/dns/rrl.h, before the result code
API update, the previous returning of ISC_R_SUCCESS within this function
was compatible with DNS_RRL_RESULT_OK, both had the same value of zero.
After the refactor of the result code API, this is no more true, these
result codes hold different values, besides, the correct result code to
use within this function is DNS_RRL_RESULT_OK and not ISC_R_SUCCESS.
- Result code descriptions and IDs (strings) are now defined in
lib/isc/result_<class>.c.in source files. See lib/isc/result_*.c.in
for examples.
- To create a new result class, check isc/resultclass.h and follow
documentation in there.
- Error codes are now defined in the following fashion:
ISC_RESULTCODE_<CLASS>(number), old style was
ISC_RESULTCLASS_<CLASS> + number.
Following is an example of the old style converted to the new one,
file lib/dns/result_dst.h.in:
;; Old style
#define DST_R_NOCRYPTO (ISC_RESULTCLASS_DST + 2)
;; New Style
#define DST_R_NOCRYPTO ISC_RESULTCODE_DST(2)
- To use error codes, same as before:
isc_result_t code = DST_R_NOCRYPTO;
isr_result_t rc = some_call(..)
if (rc == ISC_R_SUCCESS) ...
- isc_result_totext() does not lock anymore, since all result data from
all classes are available at compile time.
All relevant source files were updated to use the new result API
interface.
This commit automatically generates two files during compile time,
which will be used by the new result API implementation.
lib/isc/result_ext.c: result of combining lib/*/result_*.c.in files.
lib/isc/include/isc/result_ext.h: result of combining
lib/*/result_*.h.in files.
result_ext.c will be included by lib/isc/result.c to have all relevant
symbols available during compile time, this substitutes the old method
of having each library calling <libname>_result_register() functions.
result_ext.h will be included by lib/isc/include/isc/result.h, so source
files can now include only <isc/result.h> to have all the ISC result
codes available.
With the new result API, source files only need to include
<isc/result.h> to have isc_result_(totext|toid) functions available and
able to handle different library results, isc, isccc, dst and so on...
To allow for that, a new structure for defining result codes per library
was employed, it is implemented as follow:
- Suppose a new library called "foo" is being added to BIND.
- Two source files must be added to <BIND_ROOT>/lib/foo/:
result_foo.h.in: The header file defines result codes that may be
returned by functions within the library.
result_foo.c.in: The source file define two string arrays, used for
conversion between library result codes and their string
representation, the array <libname>_result_descriptions define a
comphreensive description of a given result code, while
<libname>_result_ids is a 1:1 result code to string.
On top of this structure, during the build phase all
lib/*/result_<libname>.c.in files will be concatenated to a file named
lib/isc/result_ext.c, which will be included by lib/isc/result.c, so
all the symbols will be available during compile time, thus making the
use of locking to manage the table of result codes not necessary
anymore.
In the same fashion, during the build phase all
lib/*/result_<libname>.h.in files will be concatenated to a file named
lib/isc/include/isc/result_ext.h, which will be included by
lib/isc/include/isc/result.h, thus exporting all error codes of all
libraries in a unified way.
The code for automatically generating such files will be added in the
subsequent commit.
Note: result codes from lib/dns/include/dns/result.h were moved to a new
file: lib/dns/result_dns.h.in, the dns/result.h file was kept since it
defines an extra function 'dns_result_torcode' that is implemented in
lib/dns/result.c, similary, string tables with dns result codes were
moved from lib/dns/result.c to lib/dns/result_dns.c.in to follow the new
result code API structure.
Use semantic patch from the previous commit, which also describes in
details why these includes are being removed.
The wrapping directives around #include <pk11/result.h> were manually
removed: #if USE_PKCS11 / #endif
- dns_result_register()
- dst_result_register()
- isccc_result_register()
- pk11_result_register()
Use the semantic patch from the previous commit to remove all function
calls listed above.
This commit also removes the above listed functions from
lib/*/win32/lib*.def.in files.
It also manually removes the preprocessor directives surrounding
pk11_result_register():
- #if USE_PKCS11
pk11_result_register();
- #endif /* if USE_PKCS11 */
add matching macros to pass arguments from called methods
to generic methods. This will reduce the amount of work
required when extending methods.
Also cleanup unnecessary UNUSED declarations.
util.h requires ISC_CONSTRUCTOR definition, which depends on config.h
inclusion. It does not include it from isc/util.h (or any other header).
Using isc/util.h fails hard when isc/util.h is used without including
bind's config.h.
Move the check to c file, where ISC_CONSTRUCTOR is used. Ensure config.h
is included there.
CDS/CDNSKEY DELETE records are only useful if they are signed,
otherwise the parent cannot verify these RRsets anyway. So once the DS
has been removed (and signaled to BIND), we can remove the DNSKEY and
RRSIG records, and at this point we can also remove the CDS/CDNSKEY
records.
The 'keymgr_key_init()' function initializes key states if they have
not been set previously. It looks at the key timing metadata and
determines using the given times whether a state should be set to
RUMOURED or OMNIPRESENT.
However, the DNSKEY and ZRRSIG states were mixed up: When looking
at the Activate timing metadata we should set the ZRRSIG state, and
when looking at the Published timing metadata we should set the
DNSKEY state.
The current isc_time_now uses CLOCK_REALTIME_COARSE which only updates
on a timer tick. This clock is generally fine for millisecond accuracy,
but on servers with 100hz clocks, this clock is nowhere near accurate
enough for microsecond accuracy.
This commit adds a new isc_time_now_hires function that uses
CLOCK_REALTIME, which gives the current time, though it is somewhat
expensive to call. When microsecond accuracy is required, it may be
required to use extra resources for higher accuracy.
When NETMGR_TRACE(_VERBOSE) is enabled, the build would fail on some
non-Linux non-glibc platforms because:
* Use <stdint.h> print macros because uint_fast32_t is not always
unsigned long
* The header <execinfo.h> is not available on non-glibc, thus commit
adds dummy backtrace() and backtrace_symbols_fd() functions for
platforms without HAVE_BACKTRACE
The netmgr unit tests were designed to push the system limits to maximum
by sending as many queries as possible in the busy loop from multiple
threads. This mostly works with UDP, but in the stateful protocol where
establishing the connection takes more time, it failed quite often in
the CI. On FreeBSD, this happened more often, because the socket() call
would fail spuriosly making the problem even worse.
This commit does several things to improve reliability:
* return value of isc_nm_<proto>connect() is always checked and retried
when scheduling the connection fails
* The busy while loop has been slowed down with usleep(1000); so the
netmgr threads could schedule the work and get executed.
* The isc_thread_yield() was replaced with usleep(1000); also to allow
the other threads to do any work.
* Instead of waiting on just one variable, we wait for multiple
variables to reach the final value
* We are wrapping the netmgr operations (connects, reads, writes,
accepts) with reference counting and waiting for all the callbacks to
be accounted for.
This has two effects:
a) the isc_nm_t is always clean of active sockets and handles when
destroyed, so it will prevent the spurious INSIST(references == 1)
from isc_nm_destroy()
b) the unit test now ensures that all the callbacks are always called
when they should be called, so any stuck test means that there was
a missing callback call and it is always a real bug
These changes allows us to remove the workaround that would not run
certain tests on systems without port load-balancing.
In tls_error(), we now call isc__nm_tlsdns_failed_read() instead of just
stopping timer and reading from the socket. This allows us to properly
cleanup any pending operation on the socket.
When shutting down, calling the isc__nm_failed_connect_cb() was delayed
until the connect callback would be called. It turned out that the
connect callback might not get called at all when the socket is being
shut down. Call the failed_connect_cb() directly in the
tlsdns_shutdown() instead of waiting for the connect callback to call it.
After a partial write the tls.senddata buffer would be rearranged to
contain only the data tha wasn't sent and the len part would be made
shorter, which would lead to attempt to free only part of a socket's
tls.senddata buffer.
The tlsdns_cycle() might call uv_write() to write data to the socket,
when this happens and the socket is shutdown before the callback
completes, the uvreq structure was not freed because the callback would
be called with non-zero status code.
The RFC7828 specifies the keepalive interval to be 16-bit, specified in
units of 100 milliseconds and the configuration options tcp-*-timeouts
are following the suit. The units of 100 milliseconds are very
unintuitive and while we can't change the configuration and presentation
format, we should not follow this weird unit in the API.
This commit changes the isc_nm_(get|set)timeouts() functions to work
with milliseconds and convert the values to milliseconds before passing
them to the function, not just internally.
The udp, tcpdns and tlsdns contained lot of cut&paste code or code that
was very similar making the stack harder to maintain as any change to
one would have to be copied to the the other protocols.
In this commit, we merge the common parts into the common functions
under isc__nm_<foo> namespace and just keep the little differences based
on the socket type.
After the TCPDNS refactoring the initial and idle timers were broken and
only the tcp-initial-timeout was always applied on the whole TCP
connection.
This broke any TCP connection that took longer than tcp-initial-timeout,
most often this would affect large zone AXFRs.
This commit changes the timeout logic in this way:
* On TCP connection accept the tcp-initial-timeout is applied
and the timer is started
* When we are processing and/or sending any DNS message the timer is
stopped
* When we stop processing all DNS messages, the tcp-idle-timeout
is applied and the timer is started again
Dynamic zones with dnssec-policy could not be thawed because KASP
zones were considered always dynamic. But a dynamic KASP zone should
also check whether updates are disabled.
This commit fixes loading the certificate chain files so that the full
chain could be sent to the clients which require that for
verification. Before that fix only the top most certificate would be
loaded from the chain and sent to clients preventing some of them to
perform certificate validation (e.g. Windows 10 DoH client).
The transport should also be detached when we skip a master, otherwise
named will crash when sending a SOA query to the next master over TLS,
because the transport must be NULL when we enter
'dns_view_gettransport'.
When we query the resolver for a domain name that is in the same zone
for which is already one or more fetches outstanding, we could
potentially hit the fetch limits. If so, recursion fails immediately
for the incoming query and if serve-stale is enabled, we may try to
return a stale answer.
If the resolver is also is authoritative for the parent zone (for
example the root zone), first a delegation is found, but we first
check the cache for a better response.
Nothing is found in the cache, so we try to recurse to find the
answer to the query.
Because of fetch-limits 'dns_resolver_createfetch()' returns an error,
which 'ns_query_recurse()' propagates to the caller,
'query_delegation_recurse()'.
Because serve-stale is enabled, 'query_usestale()' is called,
setting 'qctx->db' to the cache db, but leaving 'qctx->version'
untouched. Now 'query_lookup()' is called to search for stale data
in the cache database with a non-NULL 'qctx->version'
(which is set to a zone db version), and thus we hit an assertion
in rbtdb.
This crash was introduced in 'main' by commit
8bcd7fe69e.
- use a value less than 2^32 for DNS_ZONEFLG_FIXJOURNAL; a larger value
could cause problems in some build environments. the zone flag
DNS_ZONEFLG_DIFFONRELOAD, which was no longer in use, has now been
deleted and its value reused for _FIXJOURNAL.
*** CID 329157: Null pointer dereferences (REVERSE_INULL)
/lib/dns/journal.c: 754 in journal_open()
748 j->header.index_size * sizeof(journal_rawpos_t));
749 }
750 if (j->index != NULL) {
751 isc_mem_put(j->mctx, j->index,
752 j->header.index_size * sizeof(journal_pos_t));
753 }
CID 329157: Null pointer dereferences (REVERSE_INULL)
Null-checking "j->filename" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
754 if (j->filename != NULL) {
755 isc_mem_free(j->mctx, j->filename);
756 }
757 if (j->fp != NULL) {
758 (void)isc_stdio_close(j->fp);
759 }
It is advisable to disable Nagle's algorithm for HTTP/2 connections
because multiple HTTP/2 streams could be multiplexed over one
transport connection. Thus, delays when delivering small packets could
bring down performance for the whole session. HTTP/2 is meant to be
used this way.
when called from within the context of a network thread,
isc_nm_tlsconnect() hangs. it is waiting for the socket's
result code to be updated, but that update is supposed to happen
asynchronously in the network thread, and if we're already blocking
in the network thread, it can never occur.
we can kluge around this by setting the socket result code
early; this works for most clients (including "dig"), but it causes
inconsistent behaviors that manifest as test failures in the DoH unit
test.
so we kluged around it even more by setting the socket result code
early *only when running in the network thread*. we need a better
solution for this problem, but this will do for now.
This commit makes the server-side code polite.
It fixes the error handling code on the server side and fixes
returning error code in responses (there was a nasty bug which could
potentially crash the server).
Also, in this commit we limit max size POST request data to 96K, max
processed data size in headers to 128K (should be enough to handle any
GET requests).
If these limits are surpassed, server will terminate the request with
RST_STREAM without responding with error code. Otherwise it politely
responds with error code.
This commit also limits number of concurrent HTTP/2 streams per
transport connection on server to 100 (as nghttp2 advises by default).
Ideally, these parameters should be configurable both globally and per
every HTTP endpoint description in the configuration file, but for now
putting sane limits should be enough.
- style, cleanup, and removal of unnecessary code.
- combined isc_nm_http_add_endpoint() and isc_nm_http_add_doh_endpoint()
into one function, renamed isc_http_endpoint().
- moved isc_nm_http_connect_send_request() into doh_test.c as a helper
function; remove it from the public API.
- renamed isc_http2 and isc_nm_http2 types and functions to just isc_http
and isc_nm_http, for consistency with other existing names.
- shortened a number of long names.
- the caller is now responsible for determining the peer address.
in isc_nm_httpconnect(); this eliminates the need to parse the URI
and the dependency on an external resolver.
- the caller is also now responsible for creating the SSL client context,
for consistency with isc_nm_tlsdnsconnect().
- added setter functions for HTTP/2 ALPN. instead of setting up ALPN in
isc_tlsctx_createclient(), we now have a function
isc_tlsctx_enable_http2client_alpn() that can be run from
isc_nm_httpconnect().
- refactored isc_nm_httprequest() into separate read and send functions.
isc_nm_send() or isc_nm_read() is called on an http socket, it will
be stored until a corresponding isc_nm_read() or _send() arrives; when
we have both halves of the pair the HTTP request will be initiated.
- isc_nm_httprequest() is renamed isc__nm_http_request() for use as an
internal helper function by the DoH unit test. (eventually doh_test
should be rewritten to use read and send, and this function should
be removed.)
- added implementations of isc__nm_tls_settimeout() and
isc__nm_http_settimeout().
- increased NGHTTP2 header block length for client connections to 128K.
- use isc_mem_t for internal memory allocations inside nghttp2, to
help track memory leaks.
- send "Cache-Control" header in requests and responses. (note:
currently we try to bypass HTTP caching proxies, but ideally we should
interact with them: https://tools.ietf.org/html/rfc8484#section-5.1)
Simple typecast to size_t should be enough to silence the warning on
ARMv7, even though the code is in fact correct, because the readlen is
checked for being < 0 in the block before the warning.
'named-journalprint -x' now prints the journal's index table and
the offset of each transaction in the journal, so that index consistency
can be confirmed.
when the 'max-ixfr-ratio' option was added, journal transaction
headers were revised to include a count of RR's in each transaction.
this made it impossible to read old journal files after an upgrade.
this branch restores the ability to read version 1 transaction
headers. when rolling forward, printing journal contents, if
the wrong transaction header format is found, we can switch.
when dns_journal_rollforward() detects a version 1 transaction
header, it returns DNS_R_RECOVERABLE. this triggers zone_postload()
to force a rewrite of the journal file in the new format, and
also to schedule a dump of the zone database with minimal delay.
journal repair is done by dns_journal_compact(), which rewrites
the entire journal, ignoring 'max-journal-size'. journal size is
corrected later.
newly created journal files now have "BIND LOG V9.2" in their headers
instead of "BIND LOG V9". files with the new version string cannot be
read using the old transaction header format. note that this means
newly created journal files will be rejected by older versions of named.
named-journalprint now takes a "-x" option, causing it to print
transaction header information before each delta, including its
format version.
Call the libisc isc__initialize() constructor and isc__shutdown()
destructor from DllMain instead of having duplicate code between
those and DllMain() code.
When AddressSanitizer is in use, disable the internal mempool
implementation and redirect the isc_mempool_get to isc_mem_get
(and similarly for isc_mempool_put). This is the method recommended
by the AddressSanitizer authors for tracking allocations and
deallocations instead of custom poison/unpoison code (see
https://github.com/google/sanitizers/wiki/AddressSanitizerManualPoisoning).
The pthread_self(), thrd_current() or GetCurrentThreadId() could
actually be a pointer, so we should rather convert the value into
uintptr_t instead of unsigned long.
Convert the isc_hp API to use the globally available isc_tid_v instead
of locally defined tid_v. This should solve most of the problems on
machines with many number of cores / CPUs.
Diego Fronza
Mark Andrews
Petr Mensik
Matthijs Mekking
Patrick McLeanandEvan Hunt
Ondřej SurýandOndřej Surý
Artem Boldariev