valitydev/thrift
14
0
Fork 0
mirror of https://github.com/valitydev/thrift.git synced 2024-11-06 18:35:19 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

THRIFT-4376: fix more high impact coverity defects

Browse Source 
Led to the discovery of incorrect lua socket error handling.

This closes #1405
This commit is contained in:
James E. King, III 2017-10-28 18:25:45 -04:00
parent 375bfee701
commit 533405e3f8
14 changed files with 92 additions and 82 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
compiler/cpp/src/thrift/generate/t_erl_generator.cc
View File

@ -57,6 +57,9 @@ public:
legacy_names_ = false;
maps_ = false;
otp16_ = false;
export_lines_first_ = true;
export_types_lines_first_ = true;
for( iter = parsed_options.begin(); iter != parsed_options.end(); ++iter) {
if( iter->first.compare("legacynames") == 0) {
legacy_names_ = true;

2
compiler/cpp/src/thrift/generate/t_java_generator.cc
View File

@ -2716,8 +2716,6 @@ std::string t_java_generator::get_java_type_string(t_type* type) {
} else {
throw std::runtime_error("Unknown thrift type \"" + type->get_name()
+ "\" passed to t_java_generator::get_java_type_string!");
return "Unknown thrift type \"" + type->get_name()
+ "\" passed to t_java_generator::get_java_type_string!";
// This should never happen!
}
}

1
compiler/cpp/src/thrift/generate/t_st_generator.cc
View File

@ -56,6 +56,7 @@ public:
const std::string& option_string)
: t_oop_generator(program) {
(void)option_string;
temporary_var = 0;
std::map<std::string, std::string>::const_iterator iter;
/* no options yet */

10
compiler/cpp/src/thrift/parse/t_const_value.h
View File

@ -38,13 +38,13 @@ void convert(From*, To&);
*/
class t_const_value {
public:
enum t_const_value_type { CV_INTEGER, CV_DOUBLE, CV_STRING, CV_MAP, CV_LIST, CV_IDENTIFIER };
enum t_const_value_type { CV_INTEGER, CV_DOUBLE, CV_STRING, CV_MAP, CV_LIST, CV_IDENTIFIER, CV_UNKNOWN };
t_const_value() {}
t_const_value() : intVal_(0), doubleVal_(0.0f), enum_((t_enum*)0), valType_(CV_UNKNOWN) {}
t_const_value(int64_t val) { set_integer(val); }
t_const_value(int64_t val) : doubleVal_(0.0f), enum_((t_enum*)0), valType_(CV_UNKNOWN) { set_integer(val); }
t_const_value(std::string val) { set_string(val); }
t_const_value(std::string val) : intVal_(0), doubleVal_(0.0f), enum_((t_enum*)0), valType_(CV_UNKNOWN) { set_string(val); }
void set_string(std::string val) {
valType_ = CV_STRING;
@ -134,7 +134,7 @@ public:
void set_enum(t_enum* tenum) { enum_ = tenum; }
t_const_value_type get_type() const { return valType_; }
t_const_value_type get_type() const { if (valType_ == CV_UNKNOWN) { throw std::string("unknown t_const_value"); } return valType_; }
private:
std::map<t_const_value*, t_const_value*> mapVal_;

7
compiler/cpp/src/thrift/platform.h
View File

@ -34,10 +34,13 @@
#include <sys/stat.h>
#endif
#include <cerrno>
// ignore EEXIST, throw on any other error
#ifdef _WIN32
#define MKDIR(x) mkdir(x)
#define MKDIR(x) { int r = _mkdir(x); if (r == -1 && errno != EEXIST) { throw (std::string(x) + ": ") + strerror(errno); } }
#else
#define MKDIR(x) mkdir(x, S_IRWXU | S_IRWXG | S_IRWXO)
#define MKDIR(x) { int r = mkdir(x, S_IRWXU | S_IRWXG | S_IRWXO); if (r == -1 && errno != EEXIST) { throw (std::string(x) + ": ") + strerror(errno); } }
#endif
#ifdef PATH_MAX

6
lib/cpp/src/thrift/transport/TSocket.cpp
View File

@ -385,7 +385,11 @@ void TSocket::openConnection(struct addrinfo* res) {
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));

5
lib/cpp/test/SecurityTest.cpp
View File

@ -255,11 +255,12 @@ BOOST_AUTO_TEST_CASE(ssl_security_matrix)
% 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],

4
lib/cpp/test/TBufferBaseTest.cpp
View File

@ -567,7 +567,7 @@ BOOST_AUTO_TEST_CASE( test_FramedTransport_Write_Read ) {
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_AUTO_TEST_CASE( test_FramedTransport_Write_Read ) {
}
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)));
}
}
}

11
lib/cpp/test/TMemoryBufferTest.cpp
View File

@ -80,20 +80,11 @@ BOOST_AUTO_TEST_CASE(test_roundtrip) {
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);

1
lib/cpp/test/TServerIntegrationTest.cpp
View File

@ -265,6 +265,7 @@ public:
*/
int getServerPort() {
TServerSocket* pSock = dynamic_cast<TServerSocket*>(pServer->getServerTransport().get());
if (!pSock) { throw std::logic_error("how come?"); }
return pSock->getPort();
}

74
lib/java/test/org/apache/thrift/server/ServerTestBase.java
View File

@ -54,13 +54,13 @@ import thrift.test.Xtruct2;
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 @@ public abstract class ServerTestBase extends TestCase {
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 @@ public abstract class ServerTestBase extends TestCase {
thing.i64_thing + "})\n");
return thing;
}
public Xtruct2 testNest(Xtruct2 nest) {
Xtruct thing = nest.struct_thing;
System.out.print("testNest({" +
@ -122,7 +122,7 @@ public abstract class ServerTestBase extends TestCase {
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 @@ public abstract class ServerTestBase extends TestCase {
System.out.print("})\n");
return thing;
}
public Set<Integer> testSet(Set<Integer> thing) {
System.out.print("testSet({");
boolean first = true;
@ -151,7 +151,7 @@ public abstract class ServerTestBase extends TestCase {
System.out.print("})\n");
return thing;
}
public List<Integer> testList(List<Integer> thing) {
System.out.print("testList({");
boolean first = true;
@ -166,58 +166,58 @@ public abstract class ServerTestBase extends TestCase {
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 @@ public abstract class ServerTestBase extends TestCase {
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 @@ public abstract class ServerTestBase extends TestCase {
}
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 @@ public abstract class ServerTestBase extends TestCase {
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 @@ public abstract class ServerTestBase extends TestCase {
// 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 @@ public abstract class ServerTestBase extends TestCase {
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 @@ public abstract class ServerTestBase extends TestCase {
}
}
System.out.print("}, ");
List<Xtruct> xtructs = v2.xtructs;
System.out.print("{");
if (xtructs != null) {
@ -372,7 +372,7 @@ public abstract class ServerTestBase extends TestCase {
}
}
System.out.print("}");
System.out.print("}, ");
}
System.out.print("}, ");
@ -420,6 +420,7 @@ public abstract class ServerTestBase extends TestCase {
testOneway(testClient);
testI32(testClient);
transport.close();
socket.close();
stopServer();
}
@ -430,7 +431,7 @@ public abstract class ServerTestBase extends TestCase {
}
public List<TProtocolFactory> getProtocols() {
return PROTOCOLS;
return PROTOCOLS;
}
private void testList(ThriftTest.Client testClient) throws TException {
@ -466,14 +467,14 @@ public abstract class ServerTestBase extends TestCase {
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 @@ public abstract class ServerTestBase extends TestCase {
ThriftTest.Client testClient = new ThriftTest.Client(protocol);
assertEquals(0, testClient.testByte((byte) 0));
assertEquals(2, factory.count);
socket.close();
stopServer();
}
}

4
lib/lua/src/socket.h
View File

@ -51,8 +51,8 @@ T_ERRCODE socket_send(p_socket sock, const char *data, size_t len, int timeout);
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);

44
lib/lua/src/usocket.c
View File

@ -113,7 +113,7 @@ T_ERRCODE socket_create(p_socket sock, int domain, int type, int protocol) {
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_destroy(p_socket sock) {
}
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 @@ T_ERRCODE socket_accept(p_socket sock, p_socket client,
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 @@ T_ERRCODE socket_send(
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 @@ T_ERRCODE socket_recv(
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 @@ T_ERRCODE socket_recv(
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;
}
////////////////////////////////////////////////////////////////////////////////

2
lib/rs/src/server/threaded.rs
View File

@ -155,7 +155,7 @@ impl<PRC, RTF, IPF, WTF, OPF> TServer<PRC, RTF, IPF, WTF, OPF>
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,
),