-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Description
Ran valgrind to verify NetSSL_OpenSSL testrunner and found two problems. Similar issues also present on Net and NetSSL_Win.
Found following issues:
- Wrong 'delete' on unique_ptr could lead to memory corruption in testrunner. Applies to NetSSL_OpenSSL and NetSSL_Win, but does not to Net.
==7913== Thread 2:
==7913== Mismatched free() / delete / delete []
==7913== at 0x4C2B8DC: operator delete(void*) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==7913== by 0x436AC3: std::default_delete::operator()(char*) const (unique_ptr.h:67)
==7913== by 0x4368EC: std::unique_ptr<char, std::default_delete >::~unique_ptr() (unique_ptr.h:184)
==7913== by 0x433CA2: (anonymous namespace)::WebSocketRequestHandler::handleRequest(Poco::Net::HTTPServerRequest&, Poco::Net::HTTPServerResponse&) (WebSocketTest.cpp:52)
==7913== by 0x51F3303: Poco::Net::HTTPServerConnection::run() (HTTPServerConnection.cpp:85)
==7913== by 0x521562B: Poco::Net::TCPServerConnection::start() (TCPServerConnection.cpp:43)
==7913== by 0x522D338: Poco::Net::TCPServerDispatcher::run() (TCPServerDispatcher.cpp:114)
==7913== by 0x62987A6: Poco::PooledThread::run() (ThreadPool.cpp:214)
==7913== by 0x6294086: Poco::(anonymous namespace)::RunnableHolder::run() (Thread.cpp:43)
==7913== by 0x6293ECC: Poco::ThreadImpl::runnableEntry(void*) (Thread_STD.cpp:139)
==7913== by 0x6296D6B: void* std::_Bind_simple<void* ((Poco::ThreadImpl))(void*)>::_M_invoke<0ul>(std::_Index_tuple<0ul>) (functional:1732)
==7913== by 0x6296C76: std::_Bind_simple<void* ((Poco::ThreadImpl))(void*)>::operator()() (functional:1720)
==7913== Address 0xbf1e5f0 is 0 bytes inside a block of size 1,024 alloc'd
==7913== at 0x4C2AEA0: operator new[](unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==7913== by 0x433C1D: (anonymous namespace)::WebSocketRequestHandler::handleRequest(Poco::Net::HTTPServerRequest&, Poco::Net::HTTPServerResponse&) (WebSocketTest.cpp:52)
==7913== by 0x51F3303: Poco::Net::HTTPServerConnection::run() (HTTPServerConnection.cpp:85)
==7913== by 0x521562B: Poco::Net::TCPServerConnection::start() (TCPServerConnection.cpp:43)
==7913== by 0x522D338: Poco::Net::TCPServerDispatcher::run() (TCPServerDispatcher.cpp:114)
==7913== by 0x62987A6: Poco::PooledThread::run() (ThreadPool.cpp:214)
==7913== by 0x6294086: Poco::(anonymous namespace)::RunnableHolder::run() (Thread.cpp:43)
==7913== by 0x6293ECC: Poco::ThreadImpl::runnableEntry(void*) (Thread_STD.cpp:139)
==7913== by 0x6296D6B: void* std::_Bind_simple<void* ((Poco::ThreadImpl))(void*)>::_M_invoke<0ul>(std::_Index_tuple<0ul>) (functional:1732)
==7913== by 0x6296C76: std::_Bind_simple<void* ((Poco::ThreadImpl))(void*)>::operator()() (functional:1720)
==7913== by 0x6296C0F: std::thread::_Impl<std::_Bind_simple<void* ((Poco::ThreadImpl))(void*)> >::_M_run() (thread:115)
==7913== by 0x77BBA1F: ??? (in /usr/lib64/libstdc++.so.6.0.24)
Problem is on WebSocketTest.cpp:52 for NetSSL_OpenSSL and in same location for NetSSL_Win. Problem not found in Net package.
std::unique_ptr pBuffer(new char[_bufSize]);
'char' to be changed to 'char[]'
std::unique_ptr<char[]> pBuffer(new char[_bufSize]);
- Uninitialized 'buffer' reported by valgrind when converted to std::string in 5 parm string compare(). Could lead to segfault if invalid memory accessed.
==12969== Conditional jump or move depends on uninitialised value(s)
==12969== at 0x4C2D4B8: strlen (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==12969== by 0x77D33A4: std::basic_string<char, std::char_traits, std::allocator >::basic_string(char const*, std::allocator const&) (in /usr/lib64/libstdc++.so.6.0.24)
==12969== by 0x434383: WebSocketTest::testWebSocket() (WebSocketTest.cpp:133)
==12969== by 0x437006: CppUnit::TestCaller::runTest() (TestCaller.h:75)
==12969== by 0x65EA334: CppUnit::TestCase::run(CppUnit::TestResult*) (TestCase.cpp:116)
==12969== by 0x65E967F: CppUnit::TestSuite::run(CppUnit::TestResult*) (TestSuite.cpp:30)
==12969== by 0x65E967F: CppUnit::TestSuite::run(CppUnit::TestResult*) (TestSuite.cpp:30)
==12969== by 0x65E967F: CppUnit::TestSuite::run(CppUnit::TestResult*) (TestSuite.cpp:30)
==12969== by 0x65EBDAA: CppUnit::TestRunner::run(CppUnit::Test*) (TestRunner.cpp:133)
==12969== by 0x65EBC57: CppUnit::TestRunner::run(std::vector<std::string, std::allocatorstd::string > const&) (TestRunner.cpp:108)
==12969== by 0x412547: NetSSLApp::main(std::vector<std::string, std::allocatorstd::string > const&) (Driver.cpp:41)
==12969== by 0x57DD41C: Poco::Util::Application::run() (Application.cpp:335)
issue is on:
(WebSocketTest.cpp:133)
(WebSocketTest.cpp:142)
(WebSocketTest.cpp:152)
(WebSocketTest.cpp:160)
(WebSocketTest.cpp:167)
(WebSocketTest.cpp:209)
Problem is with the 'compare':
payload.compare(0, payload.size(), buffer, 0, n) == 0
int compare (size_t pos, size_t len, const string& str,
size_t subpos, size_t sublen) const;
variable 'buffer' in parm3 must be converted to std::string, but it may not be properly initialized and terminated with NULL leading valgrind to complain that uninitialized memory accessed.
Fixed by zeroing out buffer, but changing to 4 parm string compare without std::string conversion would be better.
Better compare() would be this one that doesn't use std::string for buffer.
int compare (size_t pos, size_t len, const char* s, size_t n) const;
Zeroing out buffer is ok solution.