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.

Sign up for GitHub

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

Fix ZMQ_HEARTBEAT_TTL maximum value check #3135

Merged
merged 3 commits into from
May 25, 2018
Merged

Fix ZMQ_HEARTBEAT_TTL maximum value check #3135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d90e70c
Problem: maximum allowed value for ZMQ_HEARTBEAT_TTL is wrong
sigiesec May 25, 2018
8b030a9
Problem: no tests for corner cases of setsockopt ZMQ_HEARTBEAT_TTL
sigiesec May 25, 2018
7637609
Problem: VS2008 has no definition of UINT16_MAX
sigiesec May 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/zmq_setsockopt.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@ no effect.
Option value type:: int
Option value unit:: milliseconds
Default value:: 0
Maximum value:: 6553599 (which is 2^16-1 deciseconds)
Applicable socket types:: all, when using connection-oriented transports


Expand Down
6 changes: 4 additions & 2 deletions src/options.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,8 @@ int zmq::options_t::set_curve_key (uint8_t *destination,
return -1;
}

const int deciseconds_per_millisecond = 100;

int zmq::options_t::setsockopt (int option_,
const void *optval_,
size_t optvallen_)
Expand Down Expand Up @@ -665,8 +667,8 @@ int zmq::options_t::setsockopt (int option_,

case ZMQ_HEARTBEAT_TTL:
// Convert this to deciseconds from milliseconds
value = value / 100;
if (is_int && value >= 0 && value <= 6553) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this was a typo? Right now the max TTL is one hour and a half, this would make it 18 hours which is a bit much

https://rfc.zeromq.org/spec:37/ZMTP

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok... that's really in the spec... But it is a very strange value, isn't it? ;)

Anyway, I will change it back to the previous value, but it makes sense to add the tests, so I keep the PR open and fix.

Copy link
Member

Choose a reason for hiding this comment

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

It's very very strange :-)

The spec is in DRAFT form, so feel free to amend it to something less strange if you want to

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I checked it again... and now I understand, but it is even a bit weirder:

First, the spec says:

The ping-ttl is a 16-bit unsigned integer in network order that MAY be zero or MAY contain a time-to-live measured in tenths of seconds

This would allow values up to UINT16_MAX==65535 deciseconds, as my PR implements.

Then it says:

The maximum TTL is 6553 seconds

This would allow values up to 65530 deciseconds

So UINT16_MAX is almost correct according to the spec, in fact it were UINT16_MAX - UINT16_MAX % 10.

However, I think this second sentence is not meant to restrict the value, but simply explain the consequence of having a 16 bit unsigned integer specifying deciseconds.

Maybe the wording in the spec should be changed to:

As a consequence, the maximum TTL is 6553.5 seconds

Copy link
Member

Choose a reason for hiding this comment

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

right, that makes even more sense, feel free to send a pr to the rfc and I'll merge that too

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I will also add this to doc/zmq_setsockopt.txt

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

value = value / deciseconds_per_millisecond;
if (is_int && value >= 0 && value <= UINT16_MAX) {
heartbeat_ttl = static_cast<uint16_t> (value);
return 0;
}
Expand Down
3 changes: 3 additions & 0 deletions src/stdint.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ typedef unsigned __int32 uint32_t;
#ifndef uint64_t
typedef unsigned __int64 uint64_t;
#endif
#ifndef UINT16_MAX
#define UINT16_MAX _UI16_MAX
#endif
#ifndef UINT32_MAX
#define UINT32_MAX _UI32_MAX
#endif
Expand Down
54 changes: 54 additions & 0 deletions tests/test_heartbeats.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@ typedef SOCKET raw_socket;
typedef int raw_socket;
#endif

#include <limits.h>

// TODO remove this here, either ensure that UINT16_MAX is always properly
// defined or handle this at a more central location
#ifndef UINT16_MAX
#define UINT16_MAX 65535
#endif

#include "testutil_unity.hpp"

void setUp ()
Expand Down Expand Up @@ -366,6 +374,48 @@ DEFINE_TESTS (pull, push, ZMQ_PULL, ZMQ_PUSH)
DEFINE_TESTS (sub, pub, ZMQ_SUB, ZMQ_PUB)
DEFINE_TESTS (pair, pair, ZMQ_PAIR, ZMQ_PAIR)

const int deciseconds_per_millisecond = 100;
const int heartbeat_ttl_max =
(UINT16_MAX + 1) * deciseconds_per_millisecond - 1;

void test_setsockopt_heartbeat_success (const int value)
{
void *const socket = test_context_socket (ZMQ_PAIR);
TEST_ASSERT_SUCCESS_ERRNO (
zmq_setsockopt (socket, ZMQ_HEARTBEAT_TTL, &value, sizeof (value)));

int value_read;
size_t value_read_size = sizeof (value_read);
TEST_ASSERT_SUCCESS_ERRNO (zmq_getsockopt (socket, ZMQ_HEARTBEAT_TTL,
&value_read, &value_read_size));

TEST_ASSERT_EQUAL_INT (value - value % deciseconds_per_millisecond,
value_read);

test_context_socket_close (socket);
}

void test_setsockopt_heartbeat_ttl_max ()
{
test_setsockopt_heartbeat_success (heartbeat_ttl_max);
}

void test_setsockopt_heartbeat_ttl_more_than_max_fails ()
{
void *const socket = test_context_socket (ZMQ_PAIR);
const int value = heartbeat_ttl_max + 1;
TEST_ASSERT_FAILURE_ERRNO (
EINVAL,
zmq_setsockopt (socket, ZMQ_HEARTBEAT_TTL, &value, sizeof (value)));

test_context_socket_close (socket);
}

void test_setsockopt_heartbeat_ttl_near_zero ()
{
test_setsockopt_heartbeat_success (deciseconds_per_millisecond - 1);
}

int main (void)
{
setup_test_environment ();
Expand All @@ -381,6 +431,10 @@ int main (void)
RUN_TEST (test_heartbeat_ttl_sub_pub);
RUN_TEST (test_heartbeat_ttl_pair_pair);

RUN_TEST (test_setsockopt_heartbeat_ttl_max);
RUN_TEST (test_setsockopt_heartbeat_ttl_more_than_max_fails);
RUN_TEST (test_setsockopt_heartbeat_ttl_near_zero);

RUN_TEST (test_heartbeat_notimeout_dealer_router);
RUN_TEST (test_heartbeat_notimeout_req_rep);
RUN_TEST (test_heartbeat_notimeout_pull_push);
Expand Down