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

Session Expiry checking does work only by chance #1525

Closed
ckrey opened this issue Dec 6, 2019 · 2 comments
Closed

Session Expiry checking does work only by chance #1525

ckrey opened this issue Dec 6, 2019 · 2 comments

Comments

@ckrey
Copy link

@ckrey ckrey commented Dec 6, 2019

session_expiry.c uses an ordered list to check periodically for expired sessions.
The list is supposed to be ordered by session_expiry_time.

The cmp function used does return an int, but subtracts two UINT32s, which does not give
the expected result if the operands are very different.

Example:
unsigned 4294967295 - 3 = 4294967292, converted to in int is -4

Reproduce:

  • OK case:
    • run mosquitto_sub -t '#' -v -d -i def -c -D connect session-expiry-interval 5
    • abort
1575668662: New client connected from 127.0.0.1 as def (p5, c0, k60).
1575668662: No will message specified.
1575668662: Sending CONNACK to def (1, 0)
1575668662: Received SUBSCRIBE from def
1575668662:     # (QoS 0)
1575668662: def 0 #
1575668662: Sending SUBACK to def
1575668662: Received DISCONNECT from def
1575668662: Client def disconnected.
1575668668: Expiring client def due to timeout.
  • Error case:
    • run mosquitto_sub -t '#' -v -d -i abc-c -D connect session-expiry-interval 4294967295
    • abort
    • run mosquitto_sub -t '#' -v -d -i def -c -D connect session-expiry-interval 5
    • abort
1575668867: New client connected from 127.0.0.1 as abc (p5, c0, k60).
1575668867: No will message specified.
1575668867: Sending CONNACK to abc (1, 0)
1575668867: Received SUBSCRIBE from abc
1575668867:     # (QoS 0)
1575668867: abc 0 #
1575668867: Sending SUBACK to abc
1575668868: Received DISCONNECT from abc
1575668868: Client abc disconnected.
1575668871: New connection from 127.0.0.1 on port 1883.
1575668871: New client connected from 127.0.0.1 as def (p5, c0, k60).
1575668871: No will message specified.
1575668871: Sending CONNACK to def (0, 0)
1575668871: Received SUBSCRIBE from def
1575668871:     # (QoS 0)
1575668871: def 0 #
1575668871: Sending SUBACK to def
1575668872: Received DISCONNECT from def
1575668872: Client def disconnected.
...
NO EXPIRING HAPPENING HERE
  • Current Code:
static int session_expiry__cmp(struct session_expiry_list *i1, struct session_expiry_list *i2)
{
        return i1->context->session_expiry_interval - i2->context->session_expiry_interval;
}
  • I would suggest to use
 if ( i1->context->session_expiry_time ==  i2->context->session_expiry_time) {
     return 0;
} else if (i1->context->session_expiry_time >  i2->context->session_expiry_time) {
    return 1;
} else {
    return -1;
}

because it

  • deals correctly with the unsigned values
  • uses expiry time rather than expiry interval taking the configured maximum client expiration time into account
@ralight

This comment has been minimized.

Copy link
Contributor

@ralight ralight commented Dec 12, 2019

Thanks Christoph, that's fixed and a test modified to include your example.

@ralight ralight closed this Dec 12, 2019
ralight added a commit that referenced this issue Dec 12, 2019
Closes #1525. Thanks to Christoph Krey.
@ckrey

This comment has been minimized.

Copy link
Author

@ckrey ckrey commented Dec 12, 2019

Thanks Roger

@lock lock bot locked as resolved and limited conversation to collaborators Mar 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.