Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rec: Sanitize d_orig_ttl stored in record cache #12673

Merged
merged 5 commits into from May 31, 2023

Conversation

omoerbeek
Copy link
Member

@omoerbeek omoerbeek commented Mar 22, 2023

The computed d_orig_ttl can be wrong if d_now.tv_sec is more recent than the d_now.tv_sec the TTD was computed from. This can happen if we went out to get e.g. keys and that took time. d_orig_ttl even potentially wraps around in that case if the TTL value was smaller than that delay. Work around that and make sure d_orig_ttl is within the legal range.

An alternative approach would be to store d_orig_ttl the moment we change the d_ttl to be a TTD. I looked into that but we have no convenient place to store it. There are also a few places the TTD is replaced yet again with another value, so things started to get quite complicated. But maybe I missed an approach to have the real original TTL stored somewhere. I'll keep looking into that, as that is a better approach in the end. Hence the draft status.

Note: 2nd commit takes the alternate approach. I jave left the original approach in between #if 0/#endif markers. Those blocks will be removed if the 2nd approach is deemed OK.

Short description

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • checked that this code was merged to master

@omoerbeek omoerbeek added the rec label Apr 3, 2023
@omoerbeek omoerbeek added this to the rec-4.9.0 milestone Apr 3, 2023
@omoerbeek omoerbeek marked this pull request as ready for review May 15, 2023 06:51
@Habbie
Copy link
Member

Habbie commented May 15, 2023

I prefer reading the first version. I think I functionally prefer the second version.

Approving this, while leaving you free to choose. Either way looks good.

@rgacogne rgacogne self-requested a review May 15, 2023 08:06
Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Peter, both options look fine. The second one is more accurate but also more complicated, I wonder if it would not be easier to record the time when we computed the TTD instead of the original TTL, so we would in theory not need to "compensate" the value later?

@omoerbeek
Copy link
Member Author

I agree with Peter, both options look fine. The second one is more accurate but also more complicated, I wonder if it would not be easier to record the time when we computed the TTD instead of the original TTL, so we would in theory not need to "compensate" the value later?

That looks like a viable thing to investigate, will do so.

@omoerbeek omoerbeek force-pushed the rec-origttl branch 2 times, most recently from 26826a9 to 3b87b19 Compare May 16, 2023 06:30
@omoerbeek omoerbeek requested review from Habbie and rgacogne May 16, 2023 06:48
The computed orig_ttl can be wrong if d_now.tv_sec is more recent
than the d_now.tv_sec the ttd was computed from. This can happen
if we went out to get e.g. keys and that took time. d_orig_ttl wraps
around in that case if the TTL value was smaller than that delay.
Work around that and make sure d_orig_ttl is within the legal range.
…ou think, as

in somce cases TTDs are modified later.
@omoerbeek omoerbeek merged commit 4d0036d into PowerDNS:master May 31, 2023
74 checks passed
@omoerbeek omoerbeek deleted the rec-origttl branch May 31, 2023 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants