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

Change from time_t to uint32_t for serial in calculateSOASerial #5068

Conversation

pieterlexis
Copy link
Contributor

Short description

time_t can be bigger, which could lead to overflow/underflow issues.

Closes #1010

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and 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

@@ -330,7 +330,7 @@ bool DNSBackend::getBeforeAndAfterNames(uint32_t id, const DNSName& zonename, co
* \param sd Information about the SOA record already available
* \param serial Output parameter. Only inspected when we return true
*/
bool DNSBackend::calculateSOASerial(const DNSName& domain, const SOAData& sd, time_t& serial)
bool DNSBackend::calculateSOASerial(const DNSName& domain, const SOAData& sd, uint32_t& serial)
{
// we do this by listing the domain and taking the maximum last modified timestamp

Copy link
Member

Choose a reason for hiding this comment

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

If we do this, we should probably make newest a uint32_t too then, and perhaps DNSResourceRecord::last_modified as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good one, although last_modified could be bigger when it comes from the backend (after 2038) so we might need some magic here to truncate properly?

Copy link
Member

@Habbie Habbie Apr 7, 2017

Choose a reason for hiding this comment

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

Won't truncation be proper by default, just by merit of stuffing a bigger number into a smaller one?

@Habbie Habbie added this to the auth-4.1.0 milestone Apr 10, 2017
@Habbie
Copy link
Member

Habbie commented Jun 19, 2017

This PR has conflicts.

@Habbie
Copy link
Member

Habbie commented Aug 14, 2017

ping

@pieterlexis pieterlexis force-pushed the issue-1010-calculateSOASerial-uint32_t branch 2 times, most recently from 5fea984 to c4e7632 Compare August 14, 2017 16:27
@aerique aerique modified the milestones: auth-4.2.0, auth-4.1.0 Aug 21, 2017
@pieterlexis pieterlexis closed this Dec 5, 2017
@pieterlexis pieterlexis deleted the issue-1010-calculateSOASerial-uint32_t branch December 5, 2017 13:10
@pieterlexis pieterlexis restored the issue-1010-calculateSOASerial-uint32_t branch December 5, 2017 13:10
@pieterlexis pieterlexis reopened this Dec 5, 2017
`time_t` can be bigger or be negative, which could lead to overflow/underflow issues.

Closes PowerDNS#1010
@pieterlexis pieterlexis force-pushed the issue-1010-calculateSOASerial-uint32_t branch from c4e7632 to f101300 Compare December 6, 2017 08:59
Copy link
Collaborator

@zeha zeha left a comment

Choose a reason for hiding this comment

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

Like it, but note that the gsql schemas have change_date as signed 32bit ints...

@pieterlexis pieterlexis merged commit 810b4de into PowerDNS:master Jan 23, 2018
@pieterlexis pieterlexis deleted the issue-1010-calculateSOASerial-uint32_t branch January 23, 2018 11:21
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.

5 participants