Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test and fix uninitialized data in XmlRpcClient #1244

Merged
merged 1 commit into from
Dec 18, 2017
Merged

Test and fix uninitialized data in XmlRpcClient #1244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions utilities/xmlrpcpp/src/XmlRpcClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,18 +49,22 @@ const char * XmlRpcClient::connectionStateStr(ClientConnectionState state) {
}

XmlRpcClient::XmlRpcClient(const char* host, int port, const char* uri/*=0*/)
: _connectionState(NO_CONNECTION),
_host(host),
_port(port),
_sendAttempts(0),
_bytesWritten(0),
_executing(false),
_eof(false),
_isFault(false),
_contentLength(0)
{
XmlRpcUtil::log(1, "XmlRpcClient new client: host %s, port %d.", host, port);

_host = host;
_port = port;
if (uri)
_uri = uri;
else
_uri = "/RPC2";
_connectionState = NO_CONNECTION;
_executing = false;
_eof = false;

// Default to keeping the connection open until an explicit close is done
setKeepOpen();
Expand Down
65 changes: 60 additions & 5 deletions utilities/xmlrpcpp/test/test_client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ TEST_F(MockSocketTest, constructor) {
EXPECT_EQ(false, a._eof);
EXPECT_EQ(true, a.getKeepOpen());
EXPECT_EQ(-1, a.getfd());
EXPECT_EQ(0, a._sendAttempts);
EXPECT_EQ(0, a._bytesWritten);
EXPECT_EQ(0, a._contentLength);
EXPECT_FALSE(a._isFault);
EXPECT_FALSE(sourceInList(&a, a._disp._sources));

XmlRpcClient b("nowhere.com", 404, "/where");
Expand Down Expand Up @@ -768,9 +772,31 @@ TEST_F(MockSocketTest, readHeader_err) {

// Expect a read and have it fail.
Expect_nbRead(7, "", false, false);
Expect_getError(ENOTCONN);
// Expect the client to close the socket.
Expect_close(7);
// Expect a reconnect attempt
Expect_socket(8);
Expect_setNonBlocking(8, true);
Expect_connect(8, "localhost", 42, true);

EXPECT_TRUE(a.readHeader());

// Check that state machine is in the correct state after getting the header.
EXPECT_EQ(XmlRpcClient::WRITE_REQUEST, a._connectionState);
EXPECT_EQ(1, a._sendAttempts);

// Check that all expected function calls were made before destruction.
CheckCalls();

// Skip the state machine forward to READ_HEADER state again.
a._connectionState = XmlRpcClient::READ_HEADER;

// Do it again, but this time don't expect a reconnect attempt
// Expect a read and have it return eof and success.
Expect_nbRead(8, "", true, true);
Expect_getError(ENOTCONN);
// Expect the client to close the socket.
Expect_close(8);

EXPECT_FALSE(a.readHeader());

Expand All @@ -792,9 +818,31 @@ TEST_F(MockSocketTest, readHeader_eof) {

// Expect a read and have it return eof and success.
Expect_nbRead(7, "", true, true);
Expect_getError(ENOTCONN);
// Expect the client to close the socket.
Expect_close(7);
// Expect a reconnect attempt
Expect_socket(8);
Expect_setNonBlocking(8, true);
Expect_connect(8, "localhost", 42, true);

EXPECT_TRUE(a.readHeader());

// Check that state machine is in the correct state after getting the header.
EXPECT_EQ(XmlRpcClient::WRITE_REQUEST, a._connectionState);
EXPECT_EQ(1, a._sendAttempts);

// Check that all expected function calls were made before destruction.
CheckCalls();

// Skip the state machine forward to READ_HEADER state again.
a._connectionState = XmlRpcClient::READ_HEADER;

// Do it again, but this time don't expect a reconnect attempt
// Expect a read and have it return eof and success.
Expect_nbRead(8, "", true, true);
Expect_getError(ENOTCONN);
// Expect the client to close the socket.
Expect_close(8);

EXPECT_FALSE(a.readHeader());

Expand Down Expand Up @@ -829,14 +877,21 @@ TEST_F(MockSocketTest, readHeader_partial_err) {

// Expect a read and have it return an error.
Expect_nbRead(7, "", false, false);
Expect_getError(ENOTCONN);
Expect_close(7);
EXPECT_FALSE(a.readHeader());
// Expect a reconnect attempt
Expect_socket(8);
Expect_setNonBlocking(8, true);
Expect_connect(8, "localhost", 42, true);

EXPECT_TRUE(a.readHeader());
// Check that state machine is in the correct state after getting the header.
EXPECT_EQ(XmlRpcClient::NO_CONNECTION, a._connectionState);
EXPECT_EQ(XmlRpcClient::WRITE_REQUEST, a._connectionState);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the rationale here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original logic was falling into the not connected state during this test because _sendAttempts was not correctly initialized to zero. This caused the client to immediately fail instead of attempting to retry.

By fixing the initialization, the client now correctly transitions to the write request state if the header read fails on the first attempt. I've updated this test and the others to correctly capture the retry behavior and the failure if readHeader fails again on a second attempt.


// Check that all expected function calls were made before destruction.
CheckCalls();

// Expect socket close on destruction.
Expect_close(8);
}

// Test readResponse()
Expand Down