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

Jump to bottom

HPCC-32715 SSL_connect/accept should honor timeout provided #19156

Open
wants to merge 1 commit into
base: candidate-9.8.x
Choose a base branch
from
Open

HPCC-32715 SSL_connect/accept should honor timeout provided #19156

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
2 changes: 1 addition & 1 deletion common/thorhelper/thorsoapcall.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2499,7 +2499,7 @@ class CWSCAsyncFor : implements IWSCAsyncFor, public CInterface, public CAsyncFo
if (ssock)
{
checkTimeLimitExceeded(&remainingMS);
int status = ssock->secure_connect();
int status = ssock->secure_connect(remainingMS);
if (status < 0)
{
StringBuffer err;
Expand Down
4 changes: 3 additions & 1 deletion esp/bindings/http/client/httpclient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,12 +310,14 @@ int CHttpClient::connect(StringBuffer& errmsg, bool forceNewConnection)
m_isPersistentSocket = false;
try
{
CCycleTimer timer;
m_socket = ISocket::connect_timeout(ep, m_connectTimeoutMs);

if(strcmp(m_protocol.get(), "HTTPS") == 0)
{
ISecureSocket* securesocket = m_ssctx->createSecureSocket(m_socket, SSLogNormal, m_host.str());
int res = securesocket->secure_connect();
unsigned remainingMs = timer.remainingMs(m_connectTimeoutMs);
Copy link
Member

Choose a reason for hiding this comment

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

remainingMs could be zero here (due to some weird delay in createSecureSocket),
but strictly speaking it should probably abort if so before trying to call secure_connect.

int res = securesocket->secure_connect(remainingMs);
if(res < 0)
{
close();
Expand Down
2 changes: 1 addition & 1 deletion esp/bindings/http/platform/httpprot.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ bool CHttpThread::onRequest()
try
{
ESPLOG(LogMax, "Accepting from secure socket");
res = secure_sock->secure_accept(logLevel);
res = secure_sock->secure_accept();
if(res < 0)
return false;
}
Expand Down
4 changes: 3 additions & 1 deletion esp/clients/roxiecontrol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ IPropertyTree *sendRoxieControlAllNodes(const SocketEndpoint &ep, const char *ms
static ISocket *createRoxieControlSocket(ISmartSocketFactory *conn, unsigned wait, unsigned connect_wait)
{
const SocketEndpoint &ep = conn->nextEndpoint();
CCycleTimer timer;
Owned<ISocket> sock = ISocket::connect_timeout(ep, connect_wait);
if (conn->isTlsService())
{
Expand All @@ -137,7 +138,8 @@ static ISocket *createRoxieControlSocket(ISmartSocketFactory *conn, unsigned wai
if (!ssock)
throw makeStringException(SECURE_CONNECTION_FAILURE, "failed creating secure socket for roxie control message");

int status = ssock->secure_connect();
unsigned remainingMs = timer.remainingMs(connect_wait);
int status = ssock->secure_connect(remainingMs);
if (status < 0)
{
StringBuffer err;
Expand Down
25 changes: 18 additions & 7 deletions fs/dafsclient/rmtclient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,13 +727,16 @@ void CRemoteBase::connectSocket(SocketEndpoint &ep, unsigned connectTimeoutMs, u
//PrintStackReport();
}
bool ok = true;
unsigned connecttimeoutMs = DEFAULT_CONNECT_TIME;
Copy link
Member

Choose a reason for hiding this comment

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

rmtclient has it's own default connect time dafsConnectTimeoutMs

try
{
CCycleTimer timer;
Copy link
Member

Choose a reason for hiding this comment

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

previously the timer was outside of the loop, i.e. 'connectTimeoutMs' was for the whole attempt time, now timer is (in effect) reset each attempt. Should move up outside of while loop.

if (tm.timemon)
{
unsigned remaining;
if (tm.timemon->timedout(&remaining))
THROWJSOCKEXCEPTION(JSOCKERR_connection_failed);
connecttimeoutMs = remaining;
Copy link
Member

Choose a reason for hiding this comment

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

this means there's 2 timing constructs, it would be better/cleaner to unify on 1.

Also I've noticed a bizarre bug in the existing sRFTM.
It should be deleted and replaced. There's only 2 uses, this being 1 of them, and the timemon in it will never be null.

I think this code should either be:

            unsigned remainingMs = timer.remainingMs(connectTimeoutMs);
            if (0 == remainingMs)
                THROWJSOCKEXCEPTION(JSOCKERR_timeout_expired);
            socket.setown(ISocket::connect_timeout(ep, remainingMs));

OR, and I think preferable given other existing code in this method reliant on CTimeMon:

            unsigned remainingMs;
            if (timeMon.timedout(&remainingMs))
                THROWJSOCKEXCEPTION(JSOCKERR_timeout_expired);
            socket.setown(ISocket::connect_timeout(ep, remainingMs));

( and replace sRFTM tm(connectTimeoutMs); with CTimeMon timeMon(connectTimeoutMs); )

and similar for the secure_connect below.

socket.setown(ISocket::connect_timeout(ep,remaining));
}
else
Expand Down Expand Up @@ -767,7 +770,8 @@ void CRemoteBase::connectSocket(SocketEndpoint &ep, unsigned connectTimeoutMs, u
}
else
ssock.setown(createSecureSocket(socket.getClear(), nullptr));
int status = ssock->secure_connect();
unsigned remainingMs = timer.remainingMs(connecttimeoutMs);
int status = ssock->secure_connect(remainingMs);
Copy link
Member

Choose a reason for hiding this comment

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

see comments above re. using same timer construct (either the CTimeMon or new CCycleTimer)

if (status < 0)
throw createDafsException(DAFSERR_connection_failed, "Failure to establish secure connection");
socket.setown(ssock.getLink());
Expand Down Expand Up @@ -1128,7 +1132,7 @@ IDaFsConnection *createDaFsConnection(const SocketEndpoint &ep, DAFSConnectCfg c

/////////////////////////

ISocket *checkSocketSecure(ISocket *socket)
ISocket *checkSocketSecure(ISocket *socket, unsigned timeoutms = DEFAULT_CONNECT_TIME)
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this default be in the header?

{
if (securitySettings.queryConnectMethod() == SSLNone)
return LINK(socket);
Expand All @@ -1144,7 +1148,7 @@ ISocket *checkSocketSecure(ISocket *socket)
try
{
ssock.setown(createSecureSocket(LINK(socket), nullptr));
int status = ssock->secure_connect();
int status = ssock->secure_connect(timeoutms);
if (status < 0)
throw createDafsException(DAFSERR_connection_failed, "Failure to establish secure connection");
return ssock.getClear();
Expand Down Expand Up @@ -1173,6 +1177,7 @@ ISocket *connectDafs(SocketEndpoint &ep, unsigned timeoutms, const IPropertyTree

if (isContainerized())
{
CCycleTimer timer;
socket.setown(ISocket::connect_timeout(ep, timeoutms));

if (service && service->getPropBool("@tls"))
Expand All @@ -1182,7 +1187,8 @@ ISocket *connectDafs(SocketEndpoint &ep, unsigned timeoutms, const IPropertyTree
try
{
ssock.setown(createSecureSocket(LINK(socket), service->queryProp("@issuer")));
int status = ssock->secure_connect();
unsigned remainingMs = timer.remainingMs(timeoutms);
int status = ssock->secure_connect(remainingMs);
if (status < 0)
throw createDafsException(DAFSERR_connection_failed, "Failure to establish secure connection to dafilesrv");
return ssock.getClear();
Expand Down Expand Up @@ -1214,12 +1220,15 @@ ISocket *connectDafs(SocketEndpoint &ep, unsigned timeoutms, const IPropertyTree
{
if ( (securitySettings.queryConnectMethod() == SSLNone) || (securitySettings.queryConnectMethod() == SSLOnly) || (securitySettings.queryConnectMethod() == UnsecureAndSSL))
{
CCycleTimer timer;
socket.setown(ISocket::connect_timeout(ep, timeoutms));
return checkSocketSecure(socket);
unsigned remainingMs = timer.remainingMs(timeoutms);
return checkSocketSecure(socket, remainingMs);
}

// SSLFirst or UnsecureFirst ...

unsigned remainingMs;
unsigned newtimeout = timeoutms;
if (newtimeout > 5000)
newtimeout = 5000;
Expand All @@ -1229,10 +1238,12 @@ ISocket *connectDafs(SocketEndpoint &ep, unsigned timeoutms, const IPropertyTree
{
conAttempts--;
bool connected = false;
CCycleTimer timer;
try
{
socket.setown(ISocket::connect_timeout(ep, newtimeout));
connected = true;
remainingMs = timer.remainingMs(newtimeout);
Copy link
Member

Choose a reason for hiding this comment

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

not sure this is right. Let's say connectDafs (.., timeoutms=60000, ..)
newtimeout will be be set to 5000.
connect_timeout taks 4900 ms, remainingMs = 100
checkSocketSecure called with timeout of 100ms (but the timeout was supposed to be 60s overall).

maybe should be:

remainingMs = timer.remainingMs(timeoutms);

but CCycleTimer should be outside of attempts loop too I think.

newtimeout = timeoutms;
}
catch (IJSOCK_Exception *e)
Expand All @@ -1257,7 +1268,7 @@ ISocket *connectDafs(SocketEndpoint &ep, unsigned timeoutms, const IPropertyTree
{
try
{
return checkSocketSecure(socket);
return checkSocketSecure(socket, remainingMs);
}
catch (IDAFS_Exception *e)
{
Expand Down Expand Up @@ -1343,7 +1354,7 @@ unsigned getRemoteVersion(ISocket *origSock, StringBuffer &ver)
if (!origSock)
return 0;

Owned<ISocket> socket = checkSocketSecure(origSock);
Owned<ISocket> socket = checkSocketSecure(origSock, 10000);
Copy link
Member

Choose a reason for hiding this comment

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

any significance to 10000 vs using default ?


unsigned ret;
MemoryBuffer sendbuf;
Expand Down
2 changes: 1 addition & 1 deletion fs/dafsclient/rmtclient.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ extern DAFSCLIENT_API int getDafsInfo(ISocket * socket, unsigned level, StringBu
extern DAFSCLIENT_API void setDafsEndpointPort(SocketEndpoint &ep);
extern DAFSCLIENT_API void setDafsLocalMountRedirect(const IpAddress &ip,const char *dir,const char *mountdir);
extern DAFSCLIENT_API ISocket *connectDafs(SocketEndpoint &ep, unsigned timeoutms, const IPropertyTree *service); // NOTE: might alter ep.port if configured for multiple ports ...
extern DAFSCLIENT_API ISocket *checkSocketSecure(ISocket *socket);
extern DAFSCLIENT_API ISocket *checkSocketSecure(ISocket *socket, unsigned timeoutms);
Copy link
Member

Choose a reason for hiding this comment

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

cpp has unsigned timeoutms = DEFAULT_CONNECT_TIME, I suspect default should be here.

extern DAFSCLIENT_API unsigned short getActiveDaliServixPort(const IpAddress &ip);
extern DAFSCLIENT_API unsigned getDaliServixVersion(const IpAddress &ip,StringBuffer &ver);
extern DAFSCLIENT_API unsigned getDaliServixVersion(const SocketEndpoint &ep,StringBuffer &ver);
Expand Down
2 changes: 1 addition & 1 deletion roxie/ccd/ccdlistener.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ class CascadeManager : public CInterface
if (!ssock)
throw makeStringException(ROXIE_TLS_ERROR, "Roxie CascadeManager failed creating secure socket for roxie control message");

int status = ssock->secure_connect();
int status = ssock->secure_connect(2000);
Copy link
Member

Choose a reason for hiding this comment

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

I do wonder whether the secure socket connect/accept defaults should be different from the jsocket defaults.
These timings (to ssl connect/accept) would generally expect to be shorter than connecting to the raw socket.

And then maybe this/other places, could generally rely on the defaults.

if (status < 0)
{
StringBuffer err;
Expand Down
4 changes: 2 additions & 2 deletions roxie/ccd/ccdprotocol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,11 +291,11 @@ class ProtocolSocketListener : public ProtocolListener
Owned<ISecureSocket> ssock;
try
{
ssock.setown(secureContext->createSecureSocket(base));
int loglevel = SSLogMin;
if (doTrace(traceSockets))
loglevel = SSLogMax;
int status = ssock->secure_accept(loglevel);
ssock.setown(secureContext->createSecureSocket(base, loglevel));
int status = ssock->secure_accept();
if (status < 0)
{
// secure_accept may also DBGLOG() errors ...
Expand Down
2 changes: 0 additions & 2 deletions system/jlib/jsocket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,6 @@
#define CONNECT_TIMEOUT_REFUSED_WAIT 1000 // maximum to sleep on connect_timeout
#define TRACE_SLOW_BLOCK_TRANSFER

#define DEFAULT_CONNECT_TIME (100*1000) // for connect_wait

#ifdef _DEBUG
// #define SIMULATE_LOST_UDP_PACKETS
#endif
Expand Down
2 changes: 2 additions & 0 deletions system/jlib/jsocket.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@
#define WAIT_FOREVER ((unsigned)-1)
#endif

#define DEFAULT_CONNECT_TIME (100*1000) // for connect_wait
Copy link
Member

Choose a reason for hiding this comment

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

now used for accept too, prob. either worth updating the comment, or introducing another #define (or constexpr) for defaultAcceptTimeMs.


enum JSOCKET_ERROR_CODES {
JSOCKERR_ok = 0,
JSOCKERR_not_opened = -1, // accept,name,peer_name,read,write
Expand Down
11 changes: 7 additions & 4 deletions system/mp/mpcomm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1099,16 +1099,19 @@ protected: friend class CMPPacketReader;
}
if (remaining<10000)
remaining = 10000; // 10s min granularity for MP

CCycleTimer timer;
newsock.setown(ISocket::connect_timeout(remoteep,remaining));

#if defined(_USE_OPENSSL)
if (parent->useTLS)
{
Owned<ISecureSocket> ssock = secureContextClient->createSecureSocket(newsock.getClear());
int tlsTraceLevel = SSLogMin;
if (parent->mpTraceLevel >= MPVerboseMsgThreshold)
tlsTraceLevel = SSLogMax;
int status = ssock->secure_connect(tlsTraceLevel);
Owned<ISecureSocket> ssock = secureContextClient->createSecureSocket(newsock.getClear(), tlsTraceLevel);
tm.timedout(&remaining);
Copy link
Member

Choose a reason for hiding this comment

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

I think should be:

if (tm.timedout(&remaining))
    return false;

int status = ssock->secure_connect(remaining);
if (status < 0)
{
ssock->close();
Expand Down Expand Up @@ -2567,11 +2570,11 @@ int CMPConnectThread::run()
#if defined(_USE_OPENSSL)
if (parent->useTLS)
{
Owned<ISecureSocket> ssock = secureContextServer->createSecureSocket(sock.getClear());
int tlsTraceLevel = SSLogMin;
if (parent->mpTraceLevel >= MPVerboseMsgThreshold)
tlsTraceLevel = SSLogMax;
int status = ssock->secure_accept(tlsTraceLevel);
Owned<ISecureSocket> ssock = secureContextServer->createSecureSocket(sock.getClear(), tlsTraceLevel);
int status = ssock->secure_accept(10000);
Copy link
Member

Choose a reason for hiding this comment

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

see other comment(s). There's a few different constants used for secure_connect/secure_accept, in most cases sensible defaults should be used. I think secure socket connect/accept should have different defaults to jsocket's raw socket connect/accept. They are not expected to have the same timing semantics.

if (status < 0)
{
ssock->close();
Expand Down
Loading
Loading