Conversation
Signed-off-by: Zafer Balkan <zafer@zaferbalkan.com>
Member
|
Thanks for the PR. Will check it soon in detail. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DnsCachecaches failed/SERVFAILlookups (BadCache/FailureCacherecords) so a resolver doesn't hammer an unreachable name server on every query. Per RFC 9520 ("Negative Caching of DNS Resolution Failures"), that failure TTL must stay bounded — long enough to suppress query storms, short enough that a real recovery is noticed quickly.The bound wasn't actually enforced.
SetExpiry()for failure records used the generic_minimumRecordTtl/_maximumRecordTtlfields (defaults 10s–3600s, and instance-configurable), not a bound specific to failure caching. A caller that raised the generic minimum TTL — a legitimate, independent knob — would silently drag failure-cache entries along with it, past the RFC's 5-minute ceiling.FailureRecordTtl's public setter also took anyuintas-is, with no bound at all.What changed
FAILURE_RECORD_TTL_MIN(1s) andFAILURE_RECORD_TTL_MAX(300s) constants inDnsCache.BadCache/FailureCacherecords now callSetExpiry(FAILURE_RECORD_TTL_MIN, FAILURE_RECORD_TTL_MAX, …)instead of the generic min/max, so failure-cache TTL is bounded independently ofMinimumRecordTtl/MaximumRecordTtl.FailureRecordTtlsetter now clamps viaMath.Clamp(value, FAILURE_RECORD_TTL_MIN, FAILURE_RECORD_TTL_MAX)instead of accepting the raw value.FailureRecordTtlproperty instead of the backing field directly, so the clamp applies consistently regardless of whether the value is set at construction or later via the property.No public API signatures changed — this is a behavioral fix, not an interface change.
Why this approach
Bounding failure-cache TTL independently (rather than, say, just validating input at each call site) keeps the invariant in one place: any path that sets
FailureRecordTtl, or that constructs a failure/bad-cache record, goes through the same clamp. That removes the coupling to the generic TTL bounds, which was the actual bug, and makes it impossible to construct aDnsCacheinstance whose failure TTL disagrees with RFC 9520 regardless of how it's configured.Testing
dotnet build TechnitiumLibrary.Net/TechnitiumLibrary.Net.csproj -c Release— succeeds, 0 warnings, 0 errors.dotnet build TechnitiumLibrary.sln -c Release— succeeds for every project except the pre-existing, unrelatedTechnitiumLibrary.Net.Firewall(Windows-only COM interop, fails on Linux regardless of this change).FailureRecordTtlwrite path in the downstream consumer,TechnitiumSoftware/DnsServer(binary config load, legacy web service, settings API) — all go through the public property, so none bypasses the new clamp.DnsServerCore.csprojfromTechnitiumSoftware/DnsServeragainst this branch's compiled assemblies — succeeds with 0 warnings, 0 errors, confirming no breakage in the primary consumer.DnsCachein this repo today. Given this is a boundary-value fix, a couple of unit tests (constructing with out-of-rangefailureRecordTtland asserting the clamp; caching a failure response and asserting the resulting record's TTL is within [1, 300]) would be a good addition — happy to add them if wanted, opened as a follow-up otherwise.Caveat
TechnitiumSoftware/DnsServer's settings API and admin UI (cacheFailureRecordTtl) perform no independent range validation before callingFailureRecordTtl's setter. After this change, a value submitted outside [1, 300] is silently clamped rather than rejected — the admin gets a different value than what they entered, with no error surfaced (though the settings-read endpoint does report back the clamped value, so the UI isn't left showing a stale number). This is a downstream UX gap, not a defect introduced here, and not a reason to hold this PR; it's tracked as a follow-up forTechnitiumSoftware/DnsServerto add explicit input validation.