resolve: place RRSIG after the corresponding entries#23289
Merged
keszybz merged 6 commits intosystemd:mainfrom May 12, 2022
Merged
resolve: place RRSIG after the corresponding entries#23289keszybz merged 6 commits intosystemd:mainfrom
keszybz merged 6 commits intosystemd:mainfrom
Conversation
Member
Author
|
cc @pemensik and @mrc0mmand. |
0ba81d6 to
986201c
Compare
When `exist->rr` and `rr` point to the same object, then it may be freed by the `dns_resource_record_unref()`.
Previously, we manage DnsAnswerItem by an array and Set, The array was used for the order of the items, and the set is used to dedup items. Let's use OrderedSet, then we can simplify the logic. This fixes dns_answer_remove_by_key() and dns_answer_remove_by_rr() which makes the set in a broken state.
986201c to
d1b8e56
Compare
Member
|
After adding the patches to #23104 it still complains: Without resolved: With resolved: And the only difference, so far, seems to be that resolved doesn't return RRSIG for root zone DNSKEY RR? Without resolved: With resolved: |
Contributor
|
Do you test DNSSEC=yes configuration on resolved? DNSSEC=no is know to strip all RRSIG records away. |
Member
This is with |
Member
Author
|
Not sure, but maybe, the issue is caused by the line here? # Create a trust anchor for resolved with our root zone
keymgr . dnskey | sed 's/ DNSKEY/ IN DNSKEY/g' >/etc/dnssec-trust-anchors.d/root.positiveYou can easily confirm the original issue #22002 is fixed by this PR: |
keszybz
reviewed
May 12, 2022
Comment on lines
+172
to
+174
| dns_resource_record_ref(rr); | ||
| dns_resource_record_unref(exist->rr); | ||
| exist->rr = dns_resource_record_ref(rr); /* lgtm [cpp/inconsistent-null-check] */ | ||
| exist->rr = rr; |
Member
There was a problem hiding this comment.
This reminds me of the helper I added in json.c to ref-and-replace-and-unref. We should add a similar one here, but let's do that later.
Comment on lines
+63
to
+66
| if (n > UINT16_MAX - m) | ||
| n = UINT16_MAX; | ||
| else | ||
| n += m; |
Member
There was a problem hiding this comment.
We should add a helper for this, something like saturate_add, but this can be done later.
Contributor
|
Thanks! Will this be part of v251? |
Member
|
yes |
Contributor
|
Uhm, it seems that this hasn't fixed the issue after all... Or is it still broken just for me? |
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
6 participants
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.
Fixes #22002.