Session Expiry checking does work only by chance

[ckrey]

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]

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

[ckrey]

Thanks Roger