1. 19 Feb, 2020 2 commits
    • Diego dos Santos Fronza's avatar
    • Diego dos Santos Fronza's avatar
      Fixed lock-order-inversion · 5d0bc2fb
      Diego dos Santos Fronza authored
      There were two locks involved which were being acquired
      in inverse order by two separate threads:
      Thread 1 (T1):
      	1. Acquires validator lock: validator.c/validator_start()
      	2. Acquires fetchctx bucket lock: resolver.c/dns_resolver_createfetch()
      Thread 2 (T2):
      	1. Acquires fetchctx bucket lock: resolver.c/validated()
      	2. Acquires validator lock: validator.c/dns_validator_destroy()
      Problem was:
      	T1 acquired validator lock (validator_start).
      	T2 acquired fetchctx bucket lock (validated).
      	T1 tries to acquire fetchctx bucket lock (dns_resolver_createfetch)
      	T2 tries to acquire validator lock (dns_validator_destroy)
      To fix the cycle, validated function was patched to acquire
      validator lock before acquiring fetchctx lock, and a complementary
      function dns_validator_destroy_locked was added so it assumes the lock
      was already held before the call.
      There are no traces in code of a possible lock order inversion that
      could result from this change, i.e., a thread acquiring fetchctx bucket
      lock then acquiring validator lock.
  2. 18 Feb, 2020 6 commits
  3. 17 Feb, 2020 2 commits
  4. 16 Feb, 2020 7 commits
  5. 14 Feb, 2020 11 commits
    • Diego dos Santos Fronza's avatar
      Merge branch... · 718ec80e
      Diego dos Santos Fronza authored
      Merge branch '1472-threadsanitizer-lock-order-inversion-potential-deadlock-dns_resolver_createfetch-vs' into 'master'
      Resolve "ThreadSanitizer: lock-order-inversion (potential deadlock) - dns_resolver_createfetch vs. dns_resolver_prime"
      Closes #1472
      See merge request !3020
    • Diego dos Santos Fronza's avatar
    • Diego dos Santos Fronza's avatar
      Fixed potential-lock-inversion · e7b36924
      Diego dos Santos Fronza authored
      This commit simplifies a bit the lock management within dns_resolver_prime()
      and prime_done() functions by means of turning resolver's attribute
      "priming" into an atomic_bool and by creating only one dependent object on the
      lock "primelock", namely the "primefetch" attribute.
      By having the attribute "priming" as an atomic type, it save us from having to
      use a lock just to test if priming is on or off for the given resolver context
      object, within "dns_resolver_prime" function.
      The "primelock" lock is still necessary, since dns_resolver_prime() function
      internally calls dns_resolver_createfetch(), and whenever this function
      succeeds it registers an event in the task manager which could be called by
      another thread, namely the "prime_done" function, and this function is
      responsible for disposing the "primefetch" attribute in the resolver object,
      also for resetting "priming" attribute to false.
      It is important that the invariant "priming == false AND primefetch == NULL"
      remains constant, so that any thread calling "dns_resolver_prime" knows for sure
      that if the "priming" attribute is false, "primefetch" attribute should also be
      NULL, so a new fetch context could be created to fulfill this purpose, and
      assigned to "primefetch" attribute under the lock protection.
      To honor the explanation above, dns_resolver_prime is implemented as follow:
      	1. Atomically checks the attribute "priming" for the given resolver context.
      	2. If "priming" is false, assumes that "primefetch" is NULL (this is
                 ensured by the "prime_done" implementation), acquire "primelock"
      	   lock and create a new fetch context, update "primefetch" pointer to
      	   point to the newly allocated fetch context.
      	3. If "priming" is true, assumes that the job is already in progress,
      	   no locks are acquired, nothing else to do.
      To keep the previous invariant consistent, "prime_done" is implemented as follow:
      	1. Acquire "primefetch" lock.
      	2. Keep a reference to the current "primefetch" object;
      	3. Reset "primefetch" attribute to NULL.
      	4. Release "primefetch" lock.
      	5. Atomically update "priming" attribute to false.
      	6. Destroy the "primefetch" object by using the temporary reference.
      This ensures that if "priming" is false, "primefetch" was already reset to NULL.
      It doesn't make any difference in having the "priming" attribute not protected
      by a lock, since the visible state of this variable would depend on the calling
      order of the functions "dns_resolver_prime" and "prime_done".
      As an example, suppose that instead of using an atomic for the "priming" attribute
      we employed a lock to protect it.
      Now suppose that "prime_done" function is called by Thread A, it is then preempted
      before acquiring the lock, thus not reseting "priming" to false.
      In parallel to that suppose that a Thread B is scheduled and that it calls
      "dns_resolver_prime()", it then acquires the lock and check that "priming" is true,
      thus it will consider that this resolver object is already priming and it won't do
      any more job.
      Conversely if the lock order was acquired in the other direction, Thread B would check
      that "priming" is false (since prime_done acquired the lock first and set "priming" to false)
      and it would initiate a priming fetch for this resolver.
      An atomic variable wouldn't change this behavior, since it would behave exactly the
      same, depending on the function call order, with the exception that it would avoid
      having to use a lock.
      There should be no side effects resulting from this change, since the previous
      implementation employed use of the more general resolver's "lock" mutex, which
      is used in far more contexts, but in the specifics of the "dns_resolver_prime"
      and "prime_done" it was only used to protect "primefetch" and "priming" attributes,
      which are not used in any of the other critical sections protected by the same lock,
      thus having zero dependency on those variables.
    • Diego dos Santos Fronza's avatar
      Added atomic_compare_exchange_strong_acq_rel macro · c210413a
      Diego dos Santos Fronza authored
      It is much better to read than:
      atomic_compare_exchange_strong_explicit() with 5 arguments.
    • Ondřej Surý's avatar
      Merge branch '46-enforce-clang-format-rules' into 'master' · a04cdde4
      Ondřej Surý authored
      Start enforcing the clang-format rules on changed files
      Closes #46
      See merge request !3063
    • Ondřej Surý's avatar
      Don't enforce copyrights on .clang-format · 60d29f69
      Ondřej Surý authored
    • Ondřej Surý's avatar
      Reformat using the new rules · 5777c44a
      Ondřej Surý authored
    • Ondřej Surý's avatar
    • Ondřej Surý's avatar
      Switch AlwaysBreakAfterReturnType from TopLevelDefinitions to All · 618947c6
      Ondřej Surý authored
      The AlwaysBreakAfterReturnType: TopLevelDefinitions was unwrapping
      the declarations of the functions in the header files.
    • Ondřej Surý's avatar
    • Ondřej Surý's avatar
      Merge branch 'each-style-tweak' into 'master' · d3b49b66
      Ondřej Surý authored
      adjust clang-format options to get closer to ISC style
      See merge request !3061
  6. 13 Feb, 2020 6 commits
    • Evan Hunt's avatar
      apply the modified style · e851ed0b
      Evan Hunt authored
    • Evan Hunt's avatar
      revise .clang-format and add a C formatting script in util · 0255a974
      Evan Hunt authored
      - add util/cformat.sh, which runs clang-format on all C files with
        the default .clang-format, and on all header files with a slightly
        modified version.
      - use correct bracing after multi-line control statements
      - stop aligning variable declarations to avoid problems with pointer
        alignment, but retain aligned declarations in header files so that
        struct definitions look cleaner.
      - static function prototypes in C files can skip the line break after
        the return type, but function prototypes in header files still have
        the line break.
      - don't break-before-brace in function definitions. ISC style calls
        for braces on the same line when function parameters fit on a single
        line, and a line break if they don't, but clang-format doesn't yet
        support that distinction. one-line function definitions are about
        four times more common than multi-line, so let's use the option that
        deviates less.
    • Ondřej Surý's avatar
      Merge branch '46-add-curly-braces' into 'master' · 67b68e06
      Ondřej Surý authored
      Add curly braces using uncrustify and then reformat with clang-format back
      Closes #46
      See merge request !3057
    • Ondřej Surý's avatar
      Use clang-tidy to add curly braces around one-line statements · 056e133c
      Ondřej Surý authored
      The command used to reformat the files in this commit was:
      ./util/run-clang-tidy \
      	-clang-tidy-binary clang-tidy-11
      	-clang-apply-replacements-binary clang-apply-replacements-11 \
      	-checks=-*,readability-braces-around-statements \
      	-j 9 \
      	-fix \
      	-format \
      	-style=file \
      clang-format -i --style=format $(git ls-files '*.c' '*.h')
      uncrustify -c .uncrustify.cfg --replace --no-backup $(git ls-files '*.c' '*.h')
      clang-format -i --style=format $(git ls-files '*.c' '*.h')
    • Ondřej Surý's avatar
    • Ondřej Surý's avatar
      Use coccinelle to add braces to nested single line statement · 36c6105e
      Ondřej Surý authored
      Both clang-tidy and uncrustify chokes on statement like this:
      for (...)
      	if (...)
      This commit uses a very simple semantic patch (below) to add braces around such
      Semantic patch used:
      statement S;
      expression E;
      while (...)
      - if (E) S
      + { if (E) { S } }
      statement S;
      expression E;
      for (...;...;...)
      - if (E) S
      + { if (E) { S } }
      statement S;
      expression E;
      if (...)
      - if (E) S
      + { if (E) { S } }
  7. 12 Feb, 2020 6 commits