1. 30 Sep, 2020 7 commits
    • Michał Kępień's avatar
      Add release note for #1967 · 7c9b4264
      Michał Kępień authored
      7c9b4264
    • Michał Kępień's avatar
      Add CHANGES entry for #1967 · 27e49f63
      Michał Kępień authored
      27e49f63
    • Michał Kępień's avatar
      Properly handle required glue (without glue cache) · 46b81fe9
      Michał Kępień authored
      If an NS RRset at the parent side of a delegation point only contains
      in-bailiwick NS records, at least one glue record should be included in
      every referral response sent for such a delegation point or else clients
      will need to send follow-up queries in order to determine name server
      addresses.  In certain edge cases (when the total size of a referral
      response without glue records was just below to the UDP packet size
      limit), named failed to adhere to that rule by sending non-truncated,
      glueless referral responses.
      
      Fix the problem by adjusting the code preparing the ADDITIONAL section
      of a referral response to properly handle required glue:
      
       1. As query_additional_cb() gets called for each NS record in an NS
          RRset, determine whether the latter only contains in-bailiwick NS
          records.  If any out-of-bailiwick NS records are present, no glue is
          strictly required and thus no extra actions need to be taken.
      
       2. After determining that a given NS RRset only contains in-bailiwick
          NS records, set the DNS_RDATASETATTR_REQUIRED attribute for the
          first glue record found for the first in-bailiwick NS record in
          order to ensure that either that glue record gets included in the
          ADDITIONAL section of the response or the TC bit gets set.  Note
          that updating the rdataset's attributes is delayed until the entire
          NS RRset is processed as the attribute update may turn out not to be
          necessary (see step 1 above).
      
       3. Ensure the owner name for required glue is added to the response at
          the beginning of the ADDITIONAL section, not its end, as
          dns_message_rendersection() only processes the first rdataset
          associated with the first name added to the ADDITIONAL section when
          looking for required glue.  If the DNS_RDATASETATTR_REQUIRED
          attribute is set for an rdataset which is located somewhere else
          (i.e. it is preceded by another rdataset associated with the same
          name or the name it is associated with is preceded by another name
          in the ADDITIONAL section), it will not be honored, i.e. the TC bit
          will not be set even if the rdataset does not fit into the response.
      46b81fe9
    • Michał Kępień's avatar
      Ensure required cached glue is rendered · 14de3289
      Michał Kępień authored
      When looking for required glue, dns_message_rendersection() only
      processes the first rdataset associated with the first name added to the
      ADDITIONAL section.  If the DNS_RDATASETATTR_REQUIRED attribute is set
      for an rdataset which is located somewhere else (i.e. it is preceded by
      another rdataset associated with the same name or the name it is
      associated with is preceded by another name in the ADDITIONAL section),
      it will not be honored, i.e. the TC bit will not be set even if the
      rdataset does not fit into the response.
      
      Check the attributes of each processed rdataset while appending names to
      a referral response based on a glue cache entry.  If a given rdataset is
      marked with DNS_RDATASETATTR_REQUIRED, make sure the name it is
      associated with is added to the response at the beginning of the
      ADDITIONAL section, not its end.
      
      Note that using ISC_LIST_PREPEND() instead of ISC_LIST_APPEND() is not
      necessary when associating the rdataset with its owner name because the
      dns_name_t structures are initialized just before the glue rdatasets are
      associated with them and thus they should be empty at that point (they
      can only be non-empty in case of both A and AAAA glue records being
      found for a given NS record, but it is not a problem as A glue records
      are looked for before AAAA glue records when required glue is first
      identified and thus it will never be the case that an AAAA record is
      marked as required while its sibling A record is not).
      14de3289
    • Michał Kępień's avatar
      Mark required glue during glue cache processing · 4e47fdfe
      Michał Kępień authored
      If an NS RRset at the parent side of a delegation point only contains
      in-bailiwick NS records, at least one glue record should be included in
      every referral response sent for such a delegation point or else clients
      will need to send follow-up queries in order to determine name server
      addresses.  In certain edge cases (when the total size of a referral
      response without glue records was just below to the UDP packet size
      limit), named failed to adhere to that rule by sending non-truncated,
      glueless referral responses.
      
      Fix the problem by making the process of preparing a glue cache entry
      for a given NS RRset more nuanced:
      
       1. As glue_nsdname_cb() gets called for each NS record in an NS RRset,
          determine whether the latter only contains in-bailiwick NS records.
          If any out-of-bailiwick NS records are present, no glue is strictly
          required and thus no extra actions need to be taken.
      
       2. After determining that a given NS RRset only contains in-bailiwick
          NS records, set the DNS_RDATASETATTR_REQUIRED attribute for the
          first glue record found for the first in-bailiwick NS record in
          order to ensure that either that glue record gets included in the
          ADDITIONAL section of the response or the TC bit gets set.  Note
          that updating the rdataset's attributes is delayed until the entire
          NS RRset is processed as the attribute update may turn out not to be
          necessary (see step 1 above).
      4e47fdfe
    • Michał Kępień's avatar
      Add tests for broken glueless referrals · 90b07147
      Michał Kępień authored
      If an NS RRset at the parent side of a delegation point only contains
      in-bailiwick NS records, at least one glue record should be included in
      every referral response sent for such a delegation point or else clients
      will need to send follow-up queries in order to determine name server
      addresses.  In certain edge cases (when the total size of a referral
      response without glue records was just below to the UDP packet size
      limit), named failed to adhere to that rule by sending non-truncated,
      glueless referral responses.
      
      Add tests which attempt to trigger that bug in several different
      scenarios which cover all possible combinations of the following
      factors:
      
        - type of zone (signed, unsigned),
        - glue record type (A, AAAA, both),
        - use of glue cache (enabled, disabled).
      90b07147
    • Michał Kępień's avatar
      Clean up the "glue" system test · c96bc0e2
      Michał Kępień authored
      Bring the "glue" system test up to speed with other system tests: add
      check numbering, ensure test artifacts are preserved upon failure,
      improve error reporting, make the test fail upon unexpected errors,
      address ShellCheck warnings.
      c96bc0e2
  2. 29 Sep, 2020 9 commits
    • Ondřej Surý's avatar
      Merge branch '2124-fix-assertion-failure-in-dns-message' into 'main' · 6e156834
      Ondřej Surý authored
      Resolve "Bind 9.16.6 Assertion failure message.c:4733: REQUIRE(msg->state == (-1)) failed"
      
      Closes #2124
      
      See merge request isc-projects/bind9!4194
      6e156834
    • Ondřej Surý's avatar
      Add CHANGES and release note for GL #2124 · 6179a388
      Ondřej Surý authored
      6179a388
    • Ondřej Surý's avatar
      The dns_message_create() cannot fail, change the return to void · 33eefe9f
      Ondřej Surý authored
      The dns_message_create() function cannot soft fail (as all memory
      allocations either succeed or cause abort), so we change the function to
      return void and cleanup the calls.
      33eefe9f
    • Diego Fronza's avatar
      cocci: Add semantic patch to refactor dns_message_destroy() · 7deaf9a9
      Diego Fronza authored
      dns_message_t objects are now being handled using reference counting
      semantics, so now dns_message_destroy() is not called directly anymore,
      dns_message_detach must be called instead.
      7deaf9a9
    • Diego Fronza's avatar
      Properly handling dns_message_t shared references · cde6227a
      Diego Fronza authored
      This commit fix the problems that arose when moving the dns_message_t
      object from fetchctx_t to the query structure.
      
      Since the lifetime of query objects are different than that of a fetchctx
      and the dns_message_t object held by the query may be being used by some
      external module, e.g. validator, even after the query may have been destroyed,
      propery handling of the references to the message were added in this commit to
      avoid accessing an already destroyed object.
      
      Specifically, in rctx_done(), a reference to the message is attached at
      the beginning of the function and detached at the end, since a possible call
      to fctx_cancelquery() would release the dns_message_t object, and in the next
      lines of code a call to rctx_nextserver() or rctx_chaseds() would require
      a valid pointer to the same object.
      
      In valcreate() a new reference is attached to the message object, this
      ensures that if the corresponding query object is destroyed before the
      validator attempts to access it, no invalid pointer access occurs.
      
      In validated() we have to attach a new reference to the message, since
      we destroy the validator object at the beginning of the function,
      and we need access to the message in the next lines of the same function.
      
      rctx_nextserver() and rctx_chaseds() functions were adapted to receive
      a new parameter of dns_message_t* type, this was so they could receive a
      valid reference to a dns_message_t since using the response context respctx_t
      to access the message through rctx->query->rmessage could lead to an already
      released reference due to the query being canceled.
      cde6227a
    • Diego Fronza's avatar
      Fix invalid dns message state in resolver's logic · 02f9e125
      Diego Fronza authored
      The assertion failure REQUIRE(msg->state == DNS_SECTION_ANY),
      caused by calling dns_message_setclass within function resquery_response()
      in resolver.c, was happening due to wrong management of dns message_t
      objects used to process responses to the queries issued by the resolver.
      
      Before the fix, a resolver's fetch context (fetchctx_t) would hold
      a pointer to the message, this same reference would then be used over all
      the attempts to resolve the query, trying next server, etc... for this to work
      the message object would have it's state reset between each iteration, marking
      it as ready for a new processing.
      
      The problem arose in a scenario with many different forwarders configured,
      managing the state of the dns_message_t object was lacking better
      synchronization, which have led it to a invalid dns_message_t state in
      resquery_response().
      
      Instead of adding unnecessarily complex code to synchronize the object,
      the dns_message_t object was moved from fetchctx_t structure to the
      query structure, where it better belongs to, since each query will produce
      a response, this way whenever a new query is created an associated
      dns_messate_t is also created.
      
      This commit deals mainly with moving the dns_message_t object from fetchctx_t
      to the query structure.
      02f9e125
    • Diego Fronza's avatar
      Refactored dns_message_t for using attach/detach semantics · 12d6d131
      Diego Fronza authored
      This commit will be used as a base for the next code updates in order
      to have a better control of dns_message_t objects' lifetime.
      12d6d131
    • Mark Andrews's avatar
      Merge branch... · e6f2f79f
      Mark Andrews authored
      Merge branch '2189-some-comments-in-lib-dns-stats-c-use-incorrect-notation-for-bit-values' into 'main'
      
      Resolve "some comments in lib/dns/stats.c use incorrect notation for bit values"
      
      Closes #2189
      
      See merge request isc-projects/bind9!4191
      e6f2f79f
    • Mark Andrews's avatar
      Update comments to have binary notation · 6727e23a
      Mark Andrews authored
      6727e23a
  3. 28 Sep, 2020 9 commits
    • Michał Kępień's avatar
      Merge branch '114-out-of-tree-system-tests' into 'main' · 90e1acfd
      Michał Kępień authored
      Add out-of-tree system test job
      
      Closes #114
      
      See merge request isc-projects/bind9!3895
      90e1acfd
    • Michal Nowak's avatar
      Do not remove $systest for out-of-tree builds · 47075f64
      Michal Nowak authored
      Previously, the $systest directory was being removed for out-of-tree
      builds at the end of each system test.  Because of that, running tests
      which depend on compiled objects was breaking subsequent "make check"
      invocations:
      
          make: Target 'check' not remade because of errors.
          Making all in dyndb/driver
          /bin/bash: line 20: cd: dyndb/driver: No such file or directory
          Making all in dlzexternal/driver
          /bin/bash: line 20: cd: dlzexternal/driver: No such file or directory
      
      Address by first removing build/test artifacts for a given test and then
      removing empty directories inside (and potentially including) $systest.
      47075f64
    • Michal Nowak's avatar
      Add an out-of-tree system test job to GitLab CI · 483e5af5
      Michal Nowak authored
      Make sure the new job does not get run for every pipeline as it is not
      expected to break often and it is similar enough to other system test
      jobs.  Change the name of the variable holding the path to the
      out-of-tree build directory to a more generic one.
      483e5af5
    • Ondřej Surý's avatar
      Merge branch 'ondrej/clear-the-uv-event-loop-before-exiting' into 'main' · 36195cf7
      Ondřej Surý authored
      Clear the libuv event loop before exiting
      
      See merge request isc-projects/bind9!4181
      36195cf7
    • Ondřej Surý's avatar
      Refactor the pausing/unpausing and finishing the nm_thread · e5ab137b
      Ondřej Surý authored
      The isc_nm_pause(), isc_nm_resume() and finishing the nm_thread() from
      nm_destroy() has been refactored, so all use the netievents instead of
      directly touching the worker structure members.  This allows us to
      remove most of the locking as the .paused and .finished members are
      always accessed from the matching nm_thread.
      
      When shutting down the nm_thread(), instead of issuing uv_stop(), we
      just shutdown the .async handler, so all uv_loop_t events are properly
      finished first and uv_run() ends gracefully with no outstanding active
      handles in the loop.
      e5ab137b
    • Michał Kępień's avatar
      Merge branch '1725-drop-function-wrapping' into 'main' · b8b3ddb0
      Michał Kępień authored
      Drop function wrapping as it is redundant for now
      
      Closes #1725
      
      See merge request isc-projects/bind9!4174
      b8b3ddb0
    • Michał Kępień's avatar
      Fix function overrides in unit tests on macOS · b60d7345
      Michał Kępień authored
      Since Mac OS X 10.1, Mach-O object files are by default built with a
      so-called two-level namespace which prevents symbol lookups in BIND unit
      tests that attempt to override the implementations of certain library
      functions from working as intended.  This feature can be disabled by
      passing the "-flat_namespace" flag to the linker.  Fix unit tests
      affected by this issue on macOS by adding "-flat_namespace" to LDFLAGS
      used for building all object files on that operating system (it is not
      enough to only set that flag for the unit test executables).
      b60d7345
    • Michał Kępień's avatar
      Drop function wrapping as it is redundant for now · 8bdba2ed
      Michał Kępień authored
      As currently used in the BIND source tree, the --wrap linker option is
      redundant because:
      
        - static builds are no longer supported,
      
        - there is no need to wrap around existing functions - what is
          actually required (at least for now) is to replace them altogether
          in unit tests,
      
        - only functions exposed by shared libraries linked into unit test
          binaries are currently being replaced.
      
      Given the above, providing the alternative implementations of functions
      to be overridden in lib/ns/tests/nstest.c is a much simpler alternative
      to using the --wrap linker option.  Drop the code detecting support for
      the latter from configure.ac, simplify the relevant Makefile.am, and
      remove lib/ns/tests/wrap.c, updating lib/ns/tests/nstest.c accordingly
      (it is harmless for unit tests which are not calling the overridden
      functions).
      8bdba2ed
    • Mark Andrews's avatar
      Merge branch '2185-nsdname-wait-recurse-speed-test-fails-under-tsan' into 'main' · aee29706
      Mark Andrews authored
      Resolve "nsdname-wait-recurse speed test fails under tsan"
      
      Closes #2185
      
      See merge request isc-projects/bind9!4184
      aee29706
  4. 27 Sep, 2020 2 commits
  5. 25 Sep, 2020 3 commits
    • Evan Hunt's avatar
      Merge branch '1041-filter-aaaa-purge-memory-pool-upon-plugin-destruction' into 'main' · bfe9d3a7
      Evan Hunt authored
      filter-aaaa: Purge memory pool upon plugin destruction
      
      Closes #1041
      
      See merge request isc-projects/bind9!1957
      bfe9d3a7
    • Michał Kępień's avatar
      Add CHANGES entry · 555e1f44
      Michał Kępień authored
      5238.	[bug]		filter-aaaa: named crashed upon shutdown if it was in
      			the process of recursing for A RRsets. [GL #1040]
      555e1f44
    • Evan Hunt's avatar
      Purge memory pool upon plugin destruction · 86eddebc
      Evan Hunt authored
      The typical sequence of events for AAAA queries which trigger recursion
      for an A RRset at the same name is as follows:
      
       1. Original query context is created.
       2. An AAAA RRset is found in cache.
       3. Client-specific data is allocated from the filter-aaaa memory pool.
       4. Recursion is triggered for an A RRset.
       5. Original query context is torn down.
      
       6. Recursion for an A RRset completes.
       7. A second query context is created.
       8. Client-specific data is retrieved from the filter-aaaa memory pool.
       9. The response to be sent is processed according to configuration.
      10. The response is sent.
      11. Client-specific data is returned to the filter-aaaa memory pool.
      12. The second query context is torn down.
      
      However, steps 6-12 are not executed if recursion for an A RRset is
      canceled.  Thus, if named is in the process of recursing for A RRsets
      when a shutdown is requested, the filter-aaaa memory pool will have
      outstanding allocations which will never get released.  This in turn
      leads to a crash since every memory pool must not have any outstanding
      allocations by the time isc_mempool_destroy() is called.
      
      Fix by creating a stub query context whenever fetch_callback() is called,
      including cancellation events. When the qctx is destroyed, it will ensure
      the client is detached and the plugin memory is freed.
      86eddebc
  6. 24 Sep, 2020 1 commit
  7. 23 Sep, 2020 9 commits