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

NTPClient: ignore invalid timestamps #2041

Merged
merged 1 commit into from
Feb 4, 2020

Conversation

fooker
Copy link
Contributor

@fooker fooker commented Feb 3, 2020

This fixes #2040 by checking against 0 timestamps.

@fooker fooker requested a review from mikee47 February 3, 2020 13:31
if(mode == NTP_MODE_SERVER && (ver == NTP_VERSION || ver == (NTP_VERSION - 1))) {
//Most likely a correct NTP packet received.
if(mode != NTP_MODE_SERVER || (ver != NTP_VERSION && ver != (NTP_VERSION - 1))) {
// Got a dodgy packet so treat it as we would a response failure
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the "reflowing" of the code. Can you please change this original comment // Got a dodgy packet so treat it as we would a response failure to // Got a dodgy packet - ignore it and retry ?

@slaff slaff added this to the 4.1.0 milestone Feb 3, 2020
@slaff
Copy link
Contributor

slaff commented Feb 3, 2020

@fooker

Some things to fix before we can merge this PR

  1. Rebase your PR against the lastest develop branch.
  2. Fix the coding style:
@@ -143,7 +143,7 @@ void NtpClient::onReceive(pbuf* buf, IpAddress remoteIP, uint16_t remotePort)
 	pbuf_copy_partial(buf, &timestamp, sizeof(timestamp), 40); // Copy only timestamp.
 	timestamp = ntohl(timestamp);
 
-	if (timestamp == 0) {
+	if(timestamp == 0) {
 		// Got a invalid timestamp - retry
 		startTimer(NTP_RESPONSE_TIMEOUT_MS);
 		return;'

You can run:

cd $SMING_HOME; make cs
  1. Fix any issue reported by travis, codacy and appveyor.

@slaff slaff removed this from the 4.1.0 milestone Feb 3, 2020
Copy link
Contributor

@mikee47 mikee47 left a comment

Choose a reason for hiding this comment

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

All good. I do have a suggestion for more self-documenting code:

auto retry = [this]() { startTimer(NTP_RESPONSE_TIMEOUT_MS); };

if(mode != NTP_MODE_SERVER) {
    return retry();
}

if(ver != NTP_VERSION && ver != (NTP_VERSION - 1)) {
    return retry();
}

...

if(timestamp == 0) {
    return retry();
}


@mikee47
Copy link
Contributor

mikee47 commented Feb 3, 2020

@fooker Don't worry about appveyor - rebase will fix that.

@fooker fooker changed the base branch from master to develop February 4, 2020 08:04
@fooker
Copy link
Contributor Author

fooker commented Feb 4, 2020

@mikee47 I really like the idea using a lambda for code deduplication. I've adopted it.
@slaff rebased to develop and fixed formatting (by implementing suggestion from above).

@slaff slaff added this to the 4.1.0 milestone Feb 4, 2020
@slaff slaff removed the 3 - Review label Feb 4, 2020
@slaff slaff merged commit 6d7a855 into SmingHub:develop Feb 4, 2020
@fooker fooker deleted the fix-ntpclient branch February 4, 2020 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NTPClient accepts malformed responses
3 participants