From a48751b34b7047cf77537f88861a5ae48127122a Mon Sep 17 00:00:00 2001 From: Arthur O'Dwyer Date: Fri, 24 Aug 2012 16:30:42 -0700 Subject: [PATCH 1/5] The "count_" out-parameter is doubled instead of unchanged. Static analysis says: src\zmq.cpp(489): error V220: Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: '* count_'. src\zmq.cpp(510): error V127: An overflow of the 32-bit 'nread' variable is possible inside a long cycle which utilizes a memsize-type loop counter. I've silenced the warning on line 489 and ignored the other. But also, it looks to me like there's a serious bug here: The out-parameter "count_" is never set to zero before we start incrementing it. So its final value will always be between 1 and 2 times its initial value. The fix seems obvious. --- src/zmq.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/zmq.cpp b/src/zmq.cpp index 3bfcf5c4..d585760b 100644 --- a/src/zmq.cpp +++ b/src/zmq.cpp @@ -486,9 +486,11 @@ int zmq_recviov (void *s_, iovec *a_, size_t *count_, int flags_) } zmq::socket_base_t *s = (zmq::socket_base_t *) s_; - size_t count = (int) *count_; + size_t count = *count_; int nread = 0; bool recvmore = true; + + *count_ = 0; for (size_t i = 0; recvmore && i < count; ++i) { // Cheat! We never close any msg From 0886b7a26bbca34fdeef3e06d0d75d023ef4b7eb Mon Sep 17 00:00:00 2001 From: Arthur O'Dwyer Date: Fri, 24 Aug 2012 16:33:48 -0700 Subject: [PATCH 2/5] Silence a compiler warning. Static analysis says: src\fd.hpp(38): error V103: Implicit type conversion from memsize to 32-bit type. Adding the explicit cast should shut it up. --- src/fd.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fd.hpp b/src/fd.hpp index 773e3803..01199bf3 100644 --- a/src/fd.hpp +++ b/src/fd.hpp @@ -35,7 +35,7 @@ namespace zmq enum {retired_fd = (fd_t)(~0)}; #else typedef SOCKET fd_t; - enum {retired_fd = INVALID_SOCKET}; + enum {retired_fd = (fd_t)INVALID_SOCKET}; #endif #else typedef int fd_t; From 6347d392fdaa41131c7c7cb3b57c4aa6c1e7a399 Mon Sep 17 00:00:00 2001 From: Arthur O'Dwyer Date: Fri, 24 Aug 2012 16:35:14 -0700 Subject: [PATCH 3/5] Fix a bug in pipe_t::flush(). Static analysis says: src\pipe.cpp(193): error V547: Expression is always false. Probably the '||' operator should be used here. If flush() is called on a pipe whose state was "terminated" or "double_terminated", the programmer's intent was to return immediately. But in fact the two conditions can never be true simultaneously, so the early return never happens, and we may try to flush a terminated pipe anyway. --- src/pipe.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pipe.cpp b/src/pipe.cpp index 49e5d349..da312ab0 100644 --- a/src/pipe.cpp +++ b/src/pipe.cpp @@ -190,7 +190,7 @@ void zmq::pipe_t::rollback () void zmq::pipe_t::flush () { // If terminate() was already called do nothing. - if (state == terminated && state == double_terminated) + if (state == terminated || state == double_terminated) return; // The peer does not exist anymore at this point. From 537a80278835d0f4290edce1bfc47d0624177cc5 Mon Sep 17 00:00:00 2001 From: Arthur O'Dwyer Date: Fri, 24 Aug 2012 16:38:46 -0700 Subject: [PATCH 4/5] Add a missing null-check, turning a segfault into an assertion. Static analysis says: src\tcp_address.cpp(297): error V595: The 'res' pointer was utilized before it was verified against nullptr. Check lines: 297, 301. src\tcp_address.cpp(603): error V106: Implicit type conversion third argument 'full_bytes' of function 'memcmp' to memsize type. src\tcp_address.cpp(603): error V526: The 'memcmp' function returns 0 if corresponding buffers are equal. Consider examining the condition for mistakes. In fact the use of "memcmp" is correct, but the enclosing "if" isn't necessary, and the compiler is happier if "full_bytes" is a size_t. --- src/tcp_address.cpp | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/tcp_address.cpp b/src/tcp_address.cpp index 45d170ca..d29f722d 100644 --- a/src/tcp_address.cpp +++ b/src/tcp_address.cpp @@ -294,12 +294,12 @@ int zmq::tcp_address_t::resolve_interface (const char *interface_, } // Use the first result. + zmq_assert (res != NULL); zmq_assert ((size_t) (res->ai_addrlen) <= sizeof (address)); memcpy (&address, res->ai_addr, res->ai_addrlen); // Cleanup getaddrinfo after copying the possibly referenced result. - if (res) - freeaddrinfo (res); + freeaddrinfo (res); return 0; } @@ -598,11 +598,9 @@ const bool zmq::tcp_address_mask_t::match_address (const struct sockaddr *ss, co } if (address_mask < mask) mask = address_mask; - int full_bytes = mask / 8; - if (full_bytes) { - if (memcmp(our_bytes, their_bytes, full_bytes)) - return false; - } + size_t full_bytes = mask / 8; + if (memcmp(our_bytes, their_bytes, full_bytes)) + return false; uint8_t last_byte_bits = (0xffU << (8 - (mask % 8))) & 0xffU; if (last_byte_bits) { From 7fadd708a04e31e1edf23fdd3935961a141f4d32 Mon Sep 17 00:00:00 2001 From: Arthur O'Dwyer Date: Fri, 24 Aug 2012 16:42:31 -0700 Subject: [PATCH 5/5] Fix monitor_event() to work at all. There are three versions of monitor_event(), all taking variadic arguments. The original code just has the first one creating a va_list and passing that va_list variadically to the second one... which creates a new va_list and passes it variadically to the third one... and of course everything blows up when we try to pull a non-va_list argument off the stack. The correct approach matches the C standard library's use of printf/vprintf, scanf/vscanf, and so on. Once you make a va_list, you must pass it only to functions which expect a va_list parameter. --- src/ctx.cpp | 10 +++++++++- src/ctx.hpp | 3 ++- src/session_base.cpp | 7 ++++++- src/session_base.hpp | 2 ++ src/socket_base.cpp | 7 ++++++- src/socket_base.hpp | 2 ++ 6 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/ctx.cpp b/src/ctx.cpp index 52222779..7af45567 100644 --- a/src/ctx.cpp +++ b/src/ctx.cpp @@ -353,7 +353,15 @@ zmq::endpoint_t zmq::ctx_t::find_endpoint (const char *addr_) return endpoint; } -void zmq::ctx_t::monitor_event (zmq::socket_base_t *socket_, int event_, va_list args_) +void zmq::ctx_t::monitor_event (zmq::socket_base_t *socket_, int event_, ...) +{ + va_list args; + va_start (event_, args); + va_monitor_event (socket_, event_, args); + va_end (args); +} + +void zmq::ctx_t::va_monitor_event (zmq::socket_base_t *socket_, int event_, va_list args_) { if (monitor_fn != NULL) { zmq_event_data_t data; diff --git a/src/ctx.hpp b/src/ctx.hpp index ab5bf897..2aba8057 100644 --- a/src/ctx.hpp +++ b/src/ctx.hpp @@ -97,7 +97,8 @@ namespace zmq // Monitoring specific int monitor (zmq_monitor_fn *monitor_); - void monitor_event (zmq::socket_base_t *socket_, int event_, va_list args_); + void monitor_event (zmq::socket_base_t *socket_, int event_, ...); + void va_monitor_event (zmq::socket_base_t *socket_, int event_, va_list args_); enum { term_tid = 0, diff --git a/src/session_base.cpp b/src/session_base.cpp index 773f6b09..a9af8783 100644 --- a/src/session_base.cpp +++ b/src/session_base.cpp @@ -290,10 +290,15 @@ void zmq::session_base_t::monitor_event (int event_, ...) { va_list args; va_start (args, event_); - socket->monitor_event (event_, args); + va_monitor_event (event_, args); va_end (args); } +void zmq::session_base_t::va_monitor_event (int event_, va_list args) +{ + socket->va_monitor_event (event_, args); +} + void zmq::session_base_t::process_plug () { if (connect) diff --git a/src/session_base.hpp b/src/session_base.hpp index f8bb6c5a..7d26ad89 100644 --- a/src/session_base.hpp +++ b/src/session_base.hpp @@ -24,6 +24,7 @@ #define __ZMQ_SESSION_BASE_HPP_INCLUDED__ #include +#include #include "own.hpp" #include "io_object.hpp" @@ -67,6 +68,7 @@ namespace zmq void terminated (zmq::pipe_t *pipe_); void monitor_event (int event_, ...); + void va_monitor_event (int event_, va_list args); protected: diff --git a/src/socket_base.cpp b/src/socket_base.cpp index 73083276..bb7c3628 100644 --- a/src/socket_base.cpp +++ b/src/socket_base.cpp @@ -1001,6 +1001,11 @@ void zmq::socket_base_t::monitor_event (int event_, ...) { va_list args; va_start (args, event_); - get_ctx ()->monitor_event (this, event_, args); + va_monitor_event(event, args); va_end (args); } + +void zmq::socket_base_t::monitor_event (int event_, va_list args) +{ + get_ctx ()->va_monitor_event (this, event_, args); +} diff --git a/src/socket_base.hpp b/src/socket_base.hpp index ab630e35..2dabd1e0 100644 --- a/src/socket_base.hpp +++ b/src/socket_base.hpp @@ -25,6 +25,7 @@ #include #include +#include #include "own.hpp" #include "array.hpp" @@ -102,6 +103,7 @@ namespace zmq void unlock(); void monitor_event (int event_, ...); + void va_monitor_event (int event_, va_list args); protected: