From 0d7e7bfac04a9d7777e8ea348e23f24f4aa8ab32 Mon Sep 17 00:00:00 2001 From: Pieter Hintjens Date: Fri, 5 Feb 2016 12:18:31 +0100 Subject: [PATCH 1/4] Problem: MSVC project filters were out of date Solution: update (with correct one from VS2015) --- .../msvc/vs2010/libzmq/libzmq.vcxproj.filters | 32 ++++++++++++++++++- .../msvc/vs2012/libzmq/libzmq.vcxproj.filters | 32 ++++++++++++++++++- .../msvc/vs2013/libzmq/libzmq.vcxproj.filters | 30 +++++++++++++++++ 3 files changed, 92 insertions(+), 2 deletions(-) diff --git a/builds/msvc/vs2010/libzmq/libzmq.vcxproj.filters b/builds/msvc/vs2010/libzmq/libzmq.vcxproj.filters index bde688db..e32e6bf5 100644 --- a/builds/msvc/vs2010/libzmq/libzmq.vcxproj.filters +++ b/builds/msvc/vs2010/libzmq/libzmq.vcxproj.filters @@ -238,7 +238,22 @@ src - + + src + + + src + + + src + + + src + + + src + + src @@ -519,6 +534,21 @@ src\include + + src\include + + + src\include + + + src\include + + + src\include + + + src\include + diff --git a/builds/msvc/vs2012/libzmq/libzmq.vcxproj.filters b/builds/msvc/vs2012/libzmq/libzmq.vcxproj.filters index bde688db..e32e6bf5 100644 --- a/builds/msvc/vs2012/libzmq/libzmq.vcxproj.filters +++ b/builds/msvc/vs2012/libzmq/libzmq.vcxproj.filters @@ -238,7 +238,22 @@ src - + + src + + + src + + + src + + + src + + + src + + src @@ -519,6 +534,21 @@ src\include + + src\include + + + src\include + + + src\include + + + src\include + + + src\include + diff --git a/builds/msvc/vs2013/libzmq/libzmq.vcxproj.filters b/builds/msvc/vs2013/libzmq/libzmq.vcxproj.filters index 27b71d9a..e32e6bf5 100644 --- a/builds/msvc/vs2013/libzmq/libzmq.vcxproj.filters +++ b/builds/msvc/vs2013/libzmq/libzmq.vcxproj.filters @@ -241,6 +241,21 @@ src + + src + + + src + + + src + + + src + + + src + @@ -519,6 +534,21 @@ src\include + + src\include + + + src\include + + + src\include + + + src\include + + + src\include + From c8318912f5c12cf76a426d75ae073fa26cdaeac8 Mon Sep 17 00:00:00 2001 From: Pieter Hintjens Date: Sat, 6 Feb 2016 13:30:44 +0100 Subject: [PATCH 2/4] Problem: test case is using internal API It is poor style for test cases to use the internal API (i.e. libzmq classes or header files), as this code serves the purpose of teaching developers how to use the library (it doesn't do this very well, it's an ambition). Also, including headers in src/ creates problems when compiling. Solution: remove use of src/macros.hpp. --- tests/test_msg_ffn.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/test_msg_ffn.cpp b/tests/test_msg_ffn.cpp index d2a8307f..4e2cccdd 100644 --- a/tests/test_msg_ffn.cpp +++ b/tests/test_msg_ffn.cpp @@ -27,12 +27,11 @@ along with this program. If not, see . */ -#include "macros.hpp" #include "testutil.hpp" void ffn(void *data, void *hint) { - LIBZMQ_UNUSED (data); // Signal that ffn has been called by writing "freed" to hint + (void) data; // Suppress 'unused' warnings at compile time memcpy(hint, (void *) "freed", 5); } From 27a8961c37704f73480b6105ac0128d6ab41641b Mon Sep 17 00:00:00 2001 From: Pieter Hintjens Date: Sat, 6 Feb 2016 13:32:23 +0100 Subject: [PATCH 3/4] Problem: resolution of int optval_ was made more verbose There's no value in this as the same pattern is repeated in several places and it's fair to expect people to understand it. Solution: revert to the old, one-liner style. --- src/dealer.cpp | 3 +-- src/options.cpp | 3 +-- src/req.cpp | 4 ++-- src/router.cpp | 3 +-- src/stream.cpp | 4 ++-- 5 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/dealer.cpp b/src/dealer.cpp index ce182224..3d7b62b8 100644 --- a/src/dealer.cpp +++ b/src/dealer.cpp @@ -70,8 +70,7 @@ int zmq::dealer_t::xsetsockopt (int option_, const void *optval_, size_t optvallen_) { bool is_int = (optvallen_ == sizeof (int)); - int value = 0; - if (is_int) memcpy(&value, optval_, sizeof (int)); + int value = is_int? *((int *) optval_): 0; switch (option_) { case ZMQ_PROBE_ROUTER: diff --git a/src/options.cpp b/src/options.cpp index 3f8fa639..1acabae1 100644 --- a/src/options.cpp +++ b/src/options.cpp @@ -92,8 +92,7 @@ int zmq::options_t::setsockopt (int option_, const void *optval_, size_t optvallen_) { bool is_int = (optvallen_ == sizeof (int)); - int value = 0; - if (is_int) memcpy(&value, optval_, sizeof (int)); + int value = is_int? *((int *) optval_): 0; #if defined (ZMQ_ACT_MILITANT) bool malformed = true; // Did caller pass a bad option value? #endif diff --git a/src/req.cpp b/src/req.cpp index 3aec0751..874a80ad 100644 --- a/src/req.cpp +++ b/src/req.cpp @@ -204,8 +204,8 @@ bool zmq::req_t::xhas_out () int zmq::req_t::xsetsockopt (int option_, const void *optval_, size_t optvallen_) { bool is_int = (optvallen_ == sizeof (int)); - int value = 0; - if (is_int) memcpy(&value, optval_, sizeof (int)); + int value = is_int? *((int *) optval_): 0; + switch (option_) { case ZMQ_REQ_CORRELATE: if (is_int && value >= 0) { diff --git a/src/router.cpp b/src/router.cpp index 1beba58d..64a16628 100644 --- a/src/router.cpp +++ b/src/router.cpp @@ -97,8 +97,7 @@ int zmq::router_t::xsetsockopt (int option_, const void *optval_, size_t optvallen_) { bool is_int = (optvallen_ == sizeof (int)); - int value = 0; - if (is_int) memcpy(&value, optval_, sizeof (int)); + int value = is_int? *((int *) optval_): 0; switch (option_) { case ZMQ_CONNECT_RID: diff --git a/src/stream.cpp b/src/stream.cpp index af8c730a..e63ca41d 100644 --- a/src/stream.cpp +++ b/src/stream.cpp @@ -178,8 +178,8 @@ int zmq::stream_t::xsetsockopt (int option_, const void *optval_, size_t optvallen_) { bool is_int = (optvallen_ == sizeof (int)); - int value = 0; - if (is_int) memcpy(&value, optval_, sizeof (int)); + int value = is_int? *((int *) optval_): 0; + switch (option_) { case ZMQ_CONNECT_RID: if (optval_ && optvallen_) { From a1f51b695f85e747ac26818f37d419c20ce8b0a6 Mon Sep 17 00:00:00 2001 From: Pieter Hintjens Date: Sat, 6 Feb 2016 14:11:21 +0100 Subject: [PATCH 4/4] Problem: unclear rules for passing null arguments Solution: be more explicit in the code, and in the zmq_recv man page (which is the most unobvious case). Assert if length is not zero and buffer is nonetheless null. --- doc/zmq_recv.txt | 3 ++- src/sub.cpp | 6 ++++-- src/xsub.cpp | 8 ++++++-- src/zmq.cpp | 22 +++++++++++++--------- tests/test_timers.cpp | 4 ++-- 5 files changed, 27 insertions(+), 16 deletions(-) diff --git a/doc/zmq_recv.txt b/doc/zmq_recv.txt index 5ec328c8..5813e1d4 100644 --- a/doc/zmq_recv.txt +++ b/doc/zmq_recv.txt @@ -19,7 +19,8 @@ by the 'socket' argument and store it in the buffer referenced by the 'buf' argument. Any bytes exceeding the length specified by the 'len' argument shall be truncated. If there are no messages available on the specified 'socket' the _zmq_recv()_ function shall block until the request can be satisfied. -The 'flags' argument is a combination of the flags defined below: +The 'flags' argument is a combination of the flags defined below: The 'buf' +argument may be null if len is zero. *ZMQ_DONTWAIT*:: Specifies that the operation should be performed in non-blocking mode. If there diff --git a/src/sub.cpp b/src/sub.cpp index 90ef672d..3c9bb8cd 100644 --- a/src/sub.cpp +++ b/src/sub.cpp @@ -62,9 +62,11 @@ int zmq::sub_t::xsetsockopt (int option_, const void *optval_, else if (option_ == ZMQ_UNSUBSCRIBE) *data = 0; - if (optvallen_ > 0) + // We explicitly allow a NULL subscription with size zero + if (optvallen_) { + assert (optval_); memcpy (data + 1, optval_, optvallen_); - + } // Pass it further on in the stack. int err = 0; rc = xsub_t::xsend (&msg); diff --git a/src/xsub.cpp b/src/xsub.cpp index 616e93bd..0a0493fc 100644 --- a/src/xsub.cpp +++ b/src/xsub.cpp @@ -226,14 +226,18 @@ void zmq::xsub_t::send_subscription (unsigned char *data_, size_t size_, { pipe_t *pipe = (pipe_t*) arg_; - // Create the subsctription message. + // Create the subscription message. msg_t msg; int rc = msg.init_size (size_ + 1); errno_assert (rc == 0); unsigned char *data = (unsigned char*) msg.data (); data [0] = 1; - if (size_ > 0) + + // We explicitly allow a NULL subscription with size zero + if (size_) { + assert (data_); memcpy (data + 1, data_, size_); + } // Send it to the pipe. bool sent = pipe->write (&msg); diff --git a/src/zmq.cpp b/src/zmq.cpp index 60bb0fca..475b5d24 100644 --- a/src/zmq.cpp +++ b/src/zmq.cpp @@ -390,14 +390,16 @@ int zmq_send (void *s_, const void *buf_, size_t len_, int flags_) return -1; } zmq_msg_t msg; - int rc = zmq_msg_init_size (&msg, len_); - if (rc != 0) + if (zmq_msg_init_size (&msg, len_)) return -1; - if (len_ > 0) - memcpy (zmq_msg_data (&msg), buf_, len_); + // We explicitly allow a send from NULL, size zero + if (len_) { + assert (buf_); + memcpy (zmq_msg_data (&msg), buf_, len_); + } zmq::socket_base_t *s = (zmq::socket_base_t *) s_; - rc = s_sendmsg (s, &msg, flags_); + int rc = s_sendmsg (s, &msg, flags_); if (unlikely (rc < 0)) { int err = errno; int rc2 = zmq_msg_close (&msg); @@ -517,12 +519,14 @@ int zmq_recv (void *s_, void *buf_, size_t len_, int flags_) return -1; } - // At the moment an oversized message is silently truncated. - // TODO: Build in a notification mechanism to report the overflows. + // An oversized message is silently truncated. size_t to_copy = size_t (nbytes) < len_ ? size_t (nbytes) : len_; - if (to_copy > 0) - memcpy (buf_, zmq_msg_data (&msg), to_copy); + // We explicitly allow a null buffer argument if len is zero + if (to_copy) { + assert (buf_); + memcpy (buf_, zmq_msg_data (&msg), to_copy); + } rc = zmq_msg_close (&msg); errno_assert (rc == 0); diff --git a/tests/test_timers.cpp b/tests/test_timers.cpp index 5125dbb6..f0d1becf 100644 --- a/tests/test_timers.cpp +++ b/tests/test_timers.cpp @@ -27,7 +27,7 @@ along with this program. If not, see . */ -#include "macros.hpp" +#include "testutil.hpp" #if defined ZMQ_HAVE_WINDOWS #include "windows.hpp" @@ -50,7 +50,7 @@ void sleep_ (long timeout_) void handler (int timer_id, void* arg) { - LIBZMQ_UNUSED (timer_id); + (void) timer_id; // Stop 'unused' compiler warnings *((bool *)arg) = true; }