THRIFT-4376: fix more high impact coverity defects
Led to the discovery of incorrect lua socket error handling.
This closes #1405
diff --git a/lib/cpp/src/thrift/transport/TSocket.cpp b/lib/cpp/src/thrift/transport/TSocket.cpp
index 7f8d7af..d93d0ff 100644
--- a/lib/cpp/src/thrift/transport/TSocket.cpp
+++ b/lib/cpp/src/thrift/transport/TSocket.cpp
@@ -385,7 +385,11 @@
done:
// Set socket back to normal mode (blocking)
- THRIFT_FCNTL(socket_, THRIFT_F_SETFL, flags);
+ if (-1 == THRIFT_FCNTL(socket_, THRIFT_F_SETFL, flags)) {
+ int errno_copy = THRIFT_GET_SOCKET_ERROR;
+ GlobalOutput.perror("TSocket::open() THRIFT_FCNTL " + getSocketInfo(), errno_copy);
+ throw TTransportException(TTransportException::NOT_OPEN, "THRIFT_FCNTL() failed", errno_copy);
+ }
if (path_.empty()) {
setCachedAddress(res->ai_addr, static_cast<socklen_t>(res->ai_addrlen));
diff --git a/lib/cpp/test/SecurityTest.cpp b/lib/cpp/test/SecurityTest.cpp
index 6eb1fe3..51ee427 100644
--- a/lib/cpp/test/SecurityTest.cpp
+++ b/lib/cpp/test/SecurityTest.cpp
@@ -255,11 +255,12 @@
% protocol2str(si) % protocol2str(ci));
mConnected = false;
+ // thread_group manages the thread lifetime - ignore the return value of create_thread
boost::thread_group threads;
- threads.create_thread(bind(&SecurityFixture::server, this, static_cast<apache::thrift::transport::SSLProtocol>(si)));
+ (void)threads.create_thread(bind(&SecurityFixture::server, this, static_cast<apache::thrift::transport::SSLProtocol>(si)));
mCVar.wait(lock); // wait for listen() to succeed
lock.unlock();
- threads.create_thread(bind(&SecurityFixture::client, this, static_cast<apache::thrift::transport::SSLProtocol>(ci)));
+ (void)threads.create_thread(bind(&SecurityFixture::client, this, static_cast<apache::thrift::transport::SSLProtocol>(ci)));
threads.join_all();
BOOST_CHECK_MESSAGE(mConnected == matrix[ci][si],
diff --git a/lib/cpp/test/TBufferBaseTest.cpp b/lib/cpp/test/TBufferBaseTest.cpp
index 4e3509e..4201ddb 100644
--- a/lib/cpp/test/TBufferBaseTest.cpp
+++ b/lib/cpp/test/TBufferBaseTest.cpp
@@ -567,7 +567,7 @@
for (int d1 = 0; d1 < 3; d1++) {
shared_ptr<TMemoryBuffer> buffer(new TMemoryBuffer(16));
TFramedTransport trans(buffer, size);
- uint8_t data_out[1<<15];
+ std::vector<uint8_t> data_out(1<<17, 0);
std::vector<int> flush_sizes;
int write_offset = 0;
@@ -605,7 +605,7 @@
}
BOOST_CHECK_EQUAL((unsigned int)read_offset, sizeof(data));
- BOOST_CHECK(!memcmp(data, data_out, sizeof(data)));
+ BOOST_CHECK(!memcmp(data, &data_out[0], sizeof(data)));
}
}
}
diff --git a/lib/cpp/test/TMemoryBufferTest.cpp b/lib/cpp/test/TMemoryBufferTest.cpp
index 4384187..9492f69 100644
--- a/lib/cpp/test/TMemoryBufferTest.cpp
+++ b/lib/cpp/test/TMemoryBufferTest.cpp
@@ -80,20 +80,11 @@
BOOST_CHECK(a == a2);
}
-BOOST_AUTO_TEST_CASE(test_copy) {
+BOOST_AUTO_TEST_CASE(test_readAppendToString) {
string* str1 = new string("abcd1234");
- ptrdiff_t str1addr = reinterpret_cast<ptrdiff_t>(str1);
- const char* data1 = str1->data();
TMemoryBuffer buf((uint8_t*)str1->data(),
static_cast<uint32_t>(str1->length()),
TMemoryBuffer::COPY);
- delete str1;
- string* str2 = new string("plsreuse");
- bool obj_reuse = (str1addr == reinterpret_cast<ptrdiff_t>(str2));
- bool dat_reuse = (data1 == str2->data());
- BOOST_TEST_MESSAGE("Object reuse: " << obj_reuse << " Data reuse: " << dat_reuse
- << ((obj_reuse && dat_reuse) ? " YAY!" : ""));
- delete str2;
string str3 = "wxyz", str4 = "6789";
buf.readAppendToString(str3, 4);
diff --git a/lib/cpp/test/TServerIntegrationTest.cpp b/lib/cpp/test/TServerIntegrationTest.cpp
index 12657d4..5f7b2e8 100644
--- a/lib/cpp/test/TServerIntegrationTest.cpp
+++ b/lib/cpp/test/TServerIntegrationTest.cpp
@@ -265,6 +265,7 @@
*/
int getServerPort() {
TServerSocket* pSock = dynamic_cast<TServerSocket*>(pServer->getServerTransport().get());
+ if (!pSock) { throw std::logic_error("how come?"); }
return pSock->getPort();
}
diff --git a/lib/java/test/org/apache/thrift/server/ServerTestBase.java b/lib/java/test/org/apache/thrift/server/ServerTestBase.java
index c2d2952..e3e4288 100644
--- a/lib/java/test/org/apache/thrift/server/ServerTestBase.java
+++ b/lib/java/test/org/apache/thrift/server/ServerTestBase.java
@@ -54,13 +54,13 @@
public abstract class ServerTestBase extends TestCase {
public static class TestHandler implements ThriftTest.Iface {
-
+
public TestHandler() {}
-
+
public void testVoid() {
System.out.print("testVoid()\n");
}
-
+
public String testString(String thing) {
System.out.print("testString(\"" + thing + "\")\n");
return thing;
@@ -70,22 +70,22 @@
System.out.print("testBool(" + thing + ")\n");
return thing;
}
-
+
public byte testByte(byte thing) {
System.out.print("testByte(" + thing + ")\n");
return thing;
}
-
+
public int testI32(int thing) {
System.out.print("testI32(" + thing + ")\n");
return thing;
}
-
+
public long testI64(long thing) {
System.out.print("testI64(" + thing + ")\n");
return thing;
}
-
+
public double testDouble(double thing) {
System.out.print("testDouble(" + thing + ")\n");
return thing;
@@ -110,7 +110,7 @@
thing.i64_thing + "})\n");
return thing;
}
-
+
public Xtruct2 testNest(Xtruct2 nest) {
Xtruct thing = nest.struct_thing;
System.out.print("testNest({" +
@@ -122,7 +122,7 @@
nest.i32_thing + "})\n");
return nest;
}
-
+
public Map<Integer,Integer> testMap(Map<Integer,Integer> thing) {
System.out.print("testMap({");
System.out.print(thing);
@@ -136,7 +136,7 @@
System.out.print("})\n");
return thing;
}
-
+
public Set<Integer> testSet(Set<Integer> thing) {
System.out.print("testSet({");
boolean first = true;
@@ -151,7 +151,7 @@
System.out.print("})\n");
return thing;
}
-
+
public List<Integer> testList(List<Integer> thing) {
System.out.print("testList({");
boolean first = true;
@@ -166,58 +166,58 @@
System.out.print("})\n");
return thing;
}
-
+
public Numberz testEnum(Numberz thing) {
System.out.print("testEnum(" + thing + ")\n");
return thing;
}
-
+
public long testTypedef(long thing) {
System.out.print("testTypedef(" + thing + ")\n");
return thing;
}
-
+
public Map<Integer,Map<Integer,Integer>> testMapMap(int hello) {
System.out.print("testMapMap(" + hello + ")\n");
Map<Integer,Map<Integer,Integer>> mapmap =
new HashMap<Integer,Map<Integer,Integer>>();
-
+
HashMap<Integer,Integer> pos = new HashMap<Integer,Integer>();
HashMap<Integer,Integer> neg = new HashMap<Integer,Integer>();
for (int i = 1; i < 5; i++) {
pos.put(i, i);
neg.put(-i, -i);
}
-
+
mapmap.put(4, pos);
mapmap.put(-4, neg);
-
+
return mapmap;
}
-
+
public Map<Long, Map<Numberz,Insanity>> testInsanity(Insanity argument) {
System.out.print("testInsanity()\n");
-
+
HashMap<Numberz,Insanity> first_map = new HashMap<Numberz, Insanity>();
HashMap<Numberz,Insanity> second_map = new HashMap<Numberz, Insanity>();;
-
+
first_map.put(Numberz.TWO, argument);
first_map.put(Numberz.THREE, argument);
-
+
Insanity looney = new Insanity();
second_map.put(Numberz.SIX, looney);
-
+
Map<Long,Map<Numberz,Insanity>> insane =
new HashMap<Long, Map<Numberz,Insanity>>();
insane.put((long)1, first_map);
insane.put((long)2, second_map);
-
+
return insane;
}
-
+
public Xtruct testMulti(byte arg0, int arg1, long arg2, Map<Short,String> arg3, Numberz arg4, long arg5) {
System.out.print("testMulti()\n");
-
+
Xtruct hello = new Xtruct();;
hello.string_thing = "Hello2";
hello.byte_thing = arg0;
@@ -225,7 +225,7 @@
hello.i64_thing = arg2;
return hello;
}
-
+
public void testException(String arg) throws Xception, TException {
System.out.print("testException("+arg+")\n");
if ("Xception".equals(arg)) {
@@ -241,7 +241,7 @@
}
return;
}
-
+
public Xtruct testMultiException(String arg0, String arg1) throws Xception, Xception2 {
System.out.print("testMultiException(" + arg0 + ", " + arg1 + ")\n");
if (arg0.equals("Xception")) {
@@ -256,12 +256,12 @@
x.struct_thing.string_thing = "This is an Xception2";
throw x;
}
-
+
Xtruct result = new Xtruct();
result.string_thing = arg1;
return result;
}
-
+
public void testOneway(int sleepFor) {
System.out.println("testOneway(" + Integer.toString(sleepFor) +
") => sleeping...");
@@ -333,7 +333,7 @@
// todo: add assertions
private void testInsanity(ThriftTest.Client testClient) throws TException {
Insanity insane;
-
+
insane = new Insanity();
insane.userMap = new HashMap<Numberz, Long>();
insane.userMap.put(Numberz.FIVE, (long)5000);
@@ -351,7 +351,7 @@
for (long key : whoa.keySet()) {
Map<Numberz,Insanity> val = whoa.get(key);
System.out.print(key + " => {");
-
+
for (Numberz k2 : val.keySet()) {
Insanity v2 = val.get(k2);
System.out.print(k2 + " => {");
@@ -363,7 +363,7 @@
}
}
System.out.print("}, ");
-
+
List<Xtruct> xtructs = v2.xtructs;
System.out.print("{");
if (xtructs != null) {
@@ -372,7 +372,7 @@
}
}
System.out.print("}");
-
+
System.out.print("}, ");
}
System.out.print("}, ");
@@ -420,6 +420,7 @@
testOneway(testClient);
testI32(testClient);
transport.close();
+ socket.close();
stopServer();
}
@@ -430,7 +431,7 @@
}
public List<TProtocolFactory> getProtocols() {
- return PROTOCOLS;
+ return PROTOCOLS;
}
private void testList(ThriftTest.Client testClient) throws TException {
@@ -466,14 +467,14 @@
testClient.testMapMap(1);
Map<Integer,Map<Integer,Integer>> mapmap =
new HashMap<Integer,Map<Integer,Integer>>();
-
+
HashMap<Integer,Integer> pos = new HashMap<Integer,Integer>();
HashMap<Integer,Integer> neg = new HashMap<Integer,Integer>();
for (int i = 1; i < 5; i++) {
pos.put(i, i);
neg.put(-i, -i);
}
-
+
mapmap.put(4, pos);
mapmap.put(-4, neg);
assertEquals(mapmap, mm);
@@ -551,6 +552,7 @@
ThriftTest.Client testClient = new ThriftTest.Client(protocol);
assertEquals(0, testClient.testByte((byte) 0));
assertEquals(2, factory.count);
+ socket.close();
stopServer();
}
}
diff --git a/lib/lua/src/socket.h b/lib/lua/src/socket.h
index 8019ffe..afb827e 100644
--- a/lib/lua/src/socket.h
+++ b/lib/lua/src/socket.h
@@ -51,8 +51,8 @@
T_ERRCODE socket_recv(p_socket sock, char *data, size_t len, int timeout,
int *received);
-void socket_setblocking(p_socket sock);
-void socket_setnonblocking(p_socket sock);
+T_ERRCODE socket_setblocking(p_socket sock);
+T_ERRCODE socket_setnonblocking(p_socket sock);
T_ERRCODE socket_accept(p_socket sock, p_socket sibling,
p_sa addr, socklen_t *addr_len, int timeout);
diff --git a/lib/lua/src/usocket.c b/lib/lua/src/usocket.c
index 864fa36..1a1b549 100644
--- a/lib/lua/src/usocket.c
+++ b/lib/lua/src/usocket.c
@@ -113,7 +113,7 @@
T_ERRCODE socket_destroy(p_socket sock) {
// TODO Figure out if I should be free-ing this
if (*sock > 0) {
- socket_setblocking(sock);
+ (void)socket_setblocking(sock);
close(*sock);
*sock = -1;
}
@@ -121,13 +121,15 @@
}
T_ERRCODE socket_bind(p_socket sock, p_sa addr, int addr_len) {
- int ret = SUCCESS;
- socket_setblocking(sock);
+ int ret = socket_setblocking(sock);
+ if (ret != SUCCESS) {
+ return ret;
+ }
if (bind(*sock, addr, addr_len)) {
ret = errno;
}
- socket_setnonblocking(sock);
- return ret;
+ int ret2 = socket_setnonblocking(sock);
+ return ret == SUCCESS ? ret2 : ret;
}
T_ERRCODE socket_get_info(p_socket sock, short *port, char *buf, size_t len) {
@@ -168,22 +170,25 @@
if (*client > 0) {
return SUCCESS;
}
- err = errno;
- } while (err != EINTR);
+ } while ((err = errno) == EINTR);
+
if (err == EAGAIN || err == ECONNABORTED) {
return socket_wait(sock, WAIT_MODE_R, timeout);
}
+
return err;
}
T_ERRCODE socket_listen(p_socket sock, int backlog) {
- int ret = SUCCESS;
- socket_setblocking(sock);
+ int ret = socket_setblocking(sock);
+ if (ret != SUCCESS) {
+ return ret;
+ }
if (listen(*sock, backlog)) {
ret = errno;
}
- socket_setnonblocking(sock);
- return ret;
+ int ret2 = socket_setnonblocking(sock);
+ return ret == SUCCESS ? ret2 : ret;
}
////////////////////////////////////////////////////////////////////////////////
@@ -217,12 +222,12 @@
if (put > 0) {
return SUCCESS;
}
- err = errno;
- } while (err != EINTR);
+ } while ((err = errno) == EINTR);
if (err == EAGAIN) {
return socket_wait(sock, WAIT_MODE_W, timeout);
}
+
return err;
}
@@ -232,8 +237,8 @@
if (*sock < 0) {
return CLOSED;
}
+ *received = 0;
- int flags = fcntl(*sock, F_GETFL, 0);
do {
got = recv(*sock, data, len, 0);
if (got > 0) {
@@ -246,27 +251,28 @@
if (got == 0) {
return CLOSED;
}
- } while (err != EINTR);
+ } while (err == EINTR);
if (err == EAGAIN) {
return socket_wait(sock, WAIT_MODE_R, timeout);
}
+
return err;
}
////////////////////////////////////////////////////////////////////////////////
// Util
-void socket_setnonblocking(p_socket sock) {
+T_ERRCODE socket_setnonblocking(p_socket sock) {
int flags = fcntl(*sock, F_GETFL, 0);
flags |= O_NONBLOCK;
- fcntl(*sock, F_SETFL, flags);
+ return fcntl(*sock, F_SETFL, flags) != -1 ? SUCCESS : errno;
}
-void socket_setblocking(p_socket sock) {
+T_ERRCODE socket_setblocking(p_socket sock) {
int flags = fcntl(*sock, F_GETFL, 0);
flags &= (~(O_NONBLOCK));
- fcntl(*sock, F_SETFL, flags);
+ return fcntl(*sock, F_SETFL, flags) != -1 ? SUCCESS : errno;
}
////////////////////////////////////////////////////////////////////////////////
diff --git a/lib/rs/src/server/threaded.rs b/lib/rs/src/server/threaded.rs
index 66680b1..515b20d 100644
--- a/lib/rs/src/server/threaded.rs
+++ b/lib/rs/src/server/threaded.rs
@@ -155,7 +155,7 @@
w_trans_factory: write_transport_factory,
o_proto_factory: output_protocol_factory,
processor: Arc::new(processor),
- worker_pool: ThreadPool::new_with_name(
+ worker_pool: ThreadPool::with_name(
"Thrift service processor".to_owned(),
num_workers,
),