Skip to content

Commit

Permalink
[XrdCl] Fix race condition resulting in dangling pointer to SidMgr, c…
Browse files Browse the repository at this point in the history 
…loses #1180.

Conflicts:
	src/XrdCl/XrdClChannel.cc
	src/XrdCl/XrdClPostMasterInterfaces.hh
	src/XrdCl/XrdClXRootDTransport.cc
	src/XrdCl/XrdClXRootDTransport.hh
  • Loading branch information
@simonmichal
simonmichal committed Apr 23, 2020
1 parent be7a43e commit 8926eb8
Show file tree
Hide file tree
Showing 8 changed files with 181 additions and 54 deletions.
5 changes: 4 additions & 1 deletion src/XrdCl/XrdClChannel.cc
View file
Expand Up @@ -35,6 +35,7 @@
#include "XrdSys/XrdSysPthread.hh"

#include <ctime>
#include <stdexcept>

namespace
{
Expand Down Expand Up @@ -245,7 +246,9 @@ namespace XrdCl
int timeoutResolution = DefaultTimeoutResolution;
env->GetInt( "TimeoutResolution", timeoutResolution );

pTransport->InitializeChannel( pChannelData );
XRootDTransport *xrdTransport = dynamic_cast<XRootDTransport*>( pTransport );
if( !xrdTransport ) throw std::logic_error( "Expected XRootD transport!" );
xrdTransport->InitializeChannel( url, pChannelData );
uint16_t numStreams = transport->StreamNumber( pChannelData );
log->Debug( PostMasterMsg, "Creating new channel to: %s %d stream(s)",
url.GetHostId().c_str(), numStreams );
Expand Down
19 changes: 4 additions & 15 deletions src/XrdCl/XrdClMessageUtils.cc
View file
Expand Up @@ -57,23 +57,12 @@ namespace XrdCl
log->Dump( XRootDMsg, "[%s] Sending message %s",
url.GetHostId().c_str(), msg->GetDescription().c_str() );


AnyObject sidMgrObj;
SIDManager *sidMgr = 0;
st = postMaster->QueryTransport( url, XRootDQuery::SIDManager,
sidMgrObj );

if( !st.IsOK() )
{
log->Error( XRootDMsg, "[%s] Unable to get stream id manager",
url.GetHostId().c_str() );
return st;
}
sidMgrObj.Get( sidMgr );

//--------------------------------------------------------------------------
// Get an instance of SID manager object
//--------------------------------------------------------------------------
std::shared_ptr<SIDManager> sidMgr( SIDMgrPool::Instance().GetSIDMgr( url ) );
ClientRequestHdr *req = (ClientRequestHdr*)msg->GetBuffer();


//--------------------------------------------------------------------------
// Allocate the SID and marshall the message
//--------------------------------------------------------------------------
Expand Down
61 changes: 61 additions & 0 deletions src/XrdCl/XrdClSIDManager.cc
View file
Expand Up @@ -119,4 +119,65 @@ namespace XrdCl
XrdSysMutexHelper scopedLock( pMutex );
return pSIDCeiling - pFreeSIDs.size() - pTimeOutSIDs.size() - 1;
}

//----------------------------------------------------------------------------
// Returns a pointer to the SIDManager object
//----------------------------------------------------------------------------
std::shared_ptr<SIDManager> SIDMgrPool::GetSIDMgr( const URL &url )
{
//--------------------------------------------------------------------------
// Look for an instance of SID manager in the pool
//--------------------------------------------------------------------------
XrdSysMutexHelper lck1( mtx );
SIDManager *mgr = 0;
auto itr = pool.find( url.GetHostId() );
if( itr == pool.end() )
{
mgr = new SIDManager();
pool[url.GetHostId()] = mgr;
}
else mgr = itr->second;

//--------------------------------------------------------------------------
// Update the reference counter
//--------------------------------------------------------------------------
XrdSysMutexHelper lck2( mgr->pMutex );
++mgr->pRefCount;

//--------------------------------------------------------------------------
// Create a shared pointer that will recycle the SID manager
//--------------------------------------------------------------------------
RecycleSidMgr deleter;
std::shared_ptr<SIDManager> ptr( mgr, deleter );

return std::move( ptr );
}

void SIDMgrPool::Recycle( SIDManager *mgr )
{
//--------------------------------------------------------------------------
// Lock the pool, we need to do it in the same order as in 'GetSIDMgr'
//--------------------------------------------------------------------------
XrdSysMutexHelper lck1( mtx );

//--------------------------------------------------------------------------
// Lock the SID manager object
//--------------------------------------------------------------------------
XrdSysMutexHelper lck2( mgr->pMutex );
--mgr->pRefCount;

if( !mgr->pRefCount )
{
//------------------------------------------------------------------------
// Remove the SID manager from the pool
//------------------------------------------------------------------------
auto itr = pool.begin();
for( ; itr != pool.end() ; ++itr )
if( itr->second == mgr ) break;
pool.erase( itr );

lck2.UnLock();
delete mgr;
}
}
}
94 changes: 92 additions & 2 deletions src/XrdCl/XrdClSIDManager.hh
View file
Expand Up @@ -21,23 +21,41 @@

#include <list>
#include <set>
#include <memory>
#include <unordered_map>
#include <string>
#include <stdint.h>
#include "XrdSys/XrdSysPthread.hh"
#include "XrdCl/XrdClStatus.hh"
#include "XrdCl/XrdClURL.hh"

namespace XrdCl
{
//----------------------------------------------------------------------------
// We need the forward declaration for the friendship to work properly
//----------------------------------------------------------------------------
class SIDMgrPool;

//----------------------------------------------------------------------------
//! Handle XRootD stream IDs
//----------------------------------------------------------------------------
class SIDManager
{
public:
friend class SIDMgrPool;

private:

//------------------------------------------------------------------------
//! Constructor
//------------------------------------------------------------------------
SIDManager(): pSIDCeiling(1) {}
SIDManager(): pSIDCeiling(1), pRefCount(0) { }

//------------------------------------------------------------------------
//! Destructor
//------------------------------------------------------------------------
~SIDManager() { }

public:

//------------------------------------------------------------------------
//! Allocate a SID
Expand Down Expand Up @@ -91,6 +109,78 @@ namespace XrdCl
std::set<uint16_t> pTimeOutSIDs;
uint16_t pSIDCeiling;
mutable XrdSysMutex pMutex;
mutable size_t pRefCount;
};

//----------------------------------------------------------------------------
//! Pool of SID manager objects
//----------------------------------------------------------------------------
class SIDMgrPool
{
public:

//------------------------------------------------------------------------
//! @return : instance of SIDMgrPool
//------------------------------------------------------------------------
static SIDMgrPool& Instance()
{
//----------------------------------------------------------------------
// We could also use a nifty counter but this is simpler and will do!
//----------------------------------------------------------------------
static SIDMgrPool *instance = new SIDMgrPool();
return *instance;
}

//------------------------------------------------------------------------
//! Destructor
//------------------------------------------------------------------------
~SIDMgrPool() { }

//------------------------------------------------------------------------
//! @param url : URL for which we need a SIDManager
//! @return : a shared pointer to SIDManager object, the pointer has
// a custom deleter that will return the object to the pool
//------------------------------------------------------------------------
std::shared_ptr<SIDManager> GetSIDMgr( const URL &url );

//------------------------------------------------------------------------
//! @param mgr : the SIDManager object to be recycled
//------------------------------------------------------------------------
void Recycle( SIDManager *mgr );

private:

//------------------------------------------------------------------------
//! A functional object for handling the deletion of SIDManager objects
//------------------------------------------------------------------------
struct RecycleSidMgr
{
inline void operator()( SIDManager *mgr )
{
SIDMgrPool &pool = SIDMgrPool::Instance();
pool.Recycle( mgr );
}
};

//------------------------------------------------------------------------
//! Constructor
//------------------------------------------------------------------------
SIDMgrPool() { }

//------------------------------------------------------------------------
//! Deleted constructors
//------------------------------------------------------------------------
SIDMgrPool( const SIDMgrPool& ) = delete;
SIDMgrPool( SIDMgrPool&& ) = delete;

//------------------------------------------------------------------------
//! Deleted assigment operators
//------------------------------------------------------------------------
SIDMgrPool& operator=( const SIDMgrPool& ) = delete;
SIDMgrPool& operator=( SIDMgrPool&& ) = delete;

XrdSysMutex mtx;
std::unordered_map<std::string, SIDManager*> pool;
};
}

Expand Down
16 changes: 2 additions & 14 deletions src/XrdCl/XrdClXRootDMsgHandler.cc
View file
Expand Up @@ -2076,20 +2076,8 @@ namespace XrdCl
// file and in this case there is no SID)
if( !url.IsLocalFile() )
{
AnyObject sidMgrObj;
Status st = pPostMaster->QueryTransport( url, XRootDQuery::SIDManager,
sidMgrObj );
if( !st.IsOK() )
{
log->Error( XRootDMsg, "[%s] Impossible to send message %s.",
pUrl.GetHostId().c_str(),
pRequest->GetDescription().c_str() );
return st;
}
sidMgrObj.Get( pSidMgr );

// and finally allocate new stream id
st = pSidMgr->AllocateSID( req->streamid );
pSidMgr = SIDMgrPool::Instance().GetSIDMgr( url );
Status st = pSidMgr->AllocateSID( req->streamid );
if( !st.IsOK() )
{
log->Error( XRootDMsg, "[%s] Impossible to send message %s.",
Expand Down
13 changes: 6 additions & 7 deletions src/XrdCl/XrdClXRootDMsgHandler.hh
View file
Expand Up @@ -119,11 +119,11 @@ namespace XrdCl
//! @param sidMgr the sid manager used to allocate SID for the initial
//! message
//------------------------------------------------------------------------
XRootDMsgHandler( Message *msg,
ResponseHandler *respHandler,
const URL *url,
SIDManager *sidMgr,
LocalFileHandler *lFileHandler):
XRootDMsgHandler( Message *msg,
ResponseHandler *respHandler,
const URL *url,
std::shared_ptr<SIDManager> sidMgr,
LocalFileHandler *lFileHandler):
pRequest( msg ),
pResponse( 0 ),
pResponseHandler( respHandler ),
Expand Down Expand Up @@ -205,7 +205,6 @@ namespace XrdCl
pResponse = reinterpret_cast<Message*>( 0xDEADBEEF );
pResponseHandler = reinterpret_cast<ResponseHandler*>( 0xDEADBEEF );
pPostMaster = reinterpret_cast<PostMaster*>( 0xDEADBEEF );
pSidMgr = reinterpret_cast<SIDManager*>( 0xDEADBEEF );
pLFileHandler = reinterpret_cast<LocalFileHandler*>( 0xDEADBEEF );
pHosts = reinterpret_cast<HostList*>( 0xDEADBEEF );
pChunkList = reinterpret_cast<ChunkList*>( 0xDEADBEEF );
Expand Down Expand Up @@ -544,7 +543,7 @@ namespace XrdCl
URL pUrl;
URL *pEffectiveDataServerUrl;
PostMaster *pPostMaster;
SIDManager *pSidMgr;
std::shared_ptr<SIDManager> pSidMgr;
LocalFileHandler *pLFileHandler;
Status pStatus;
Status pLastError;
Expand Down
23 changes: 10 additions & 13 deletions src/XrdCl/XrdClXRootDTransport.cc
View file
Expand Up @@ -51,6 +51,7 @@
#include <iomanip>
#include <set>
#include <limits>
#include <stdexcept>

#if __cplusplus >= 201103L
#include <atomic>
Expand Down Expand Up @@ -192,11 +193,10 @@ namespace XrdCl
//--------------------------------------------------------------------------
// Constructor
//--------------------------------------------------------------------------
XRootDChannelInfo():
XRootDChannelInfo( const URL &url ):
serverFlags(0),
protocolVersion(0),
firstLogIn(true),
sidManager(0),
authBuffer(0),
authProtocol(0),
authParams(0),
Expand All @@ -208,7 +208,7 @@ namespace XrdCl
protRespSize(0),
strmSelector(0)
{
sidManager = new SIDManager();
sidManager = SIDMgrPool::Instance().GetSIDMgr( url.GetHostId() );
memset( sessionId, 0, 16 );
memset( oldSessionId, 0, 16 );
}
Expand All @@ -218,7 +218,6 @@ namespace XrdCl
//--------------------------------------------------------------------------
~XRootDChannelInfo()
{
delete sidManager;
delete [] authBuffer;
delete strmSelector;
}
Expand All @@ -233,7 +232,7 @@ namespace XrdCl
uint8_t sessionId[16];
uint8_t oldSessionId[16];
bool firstLogIn;
SIDManager *sidManager;
std::shared_ptr<SIDManager> sidManager;
char *authBuffer;
XrdSecProtocol *authProtocol;
XrdSecParameters *authParams;
Expand Down Expand Up @@ -350,7 +349,12 @@ namespace XrdCl
//----------------------------------------------------------------------------
void XRootDTransport::InitializeChannel( AnyObject &channelData )
{
XRootDChannelInfo *info = new XRootDChannelInfo();
throw std::logic_error( "InitializeChannel: deprecated interface!" );
}

void XRootDTransport::InitializeChannel( const URL &url, AnyObject &channelData )
{
XRootDChannelInfo *info = new XRootDChannelInfo( url );
XrdSysMutexHelper scopedLock( info->mutex );
channelData.Set( info );

Expand Down Expand Up @@ -1184,13 +1188,6 @@ namespace XrdCl
result.Set( new std::string( info->authProtocolName ), false );
return Status();

//------------------------------------------------------------------------
// SID Manager object
//------------------------------------------------------------------------
case XRootDQuery::SIDManager:
result.Set( info->sidManager, false );
return Status();

//------------------------------------------------------------------------
// Server flags
//------------------------------------------------------------------------
Expand Down
4 changes: 2 additions & 2 deletions src/XrdCl/XrdClXRootDTransport.hh
View file
Expand Up @@ -43,7 +43,6 @@ namespace XrdCl
//----------------------------------------------------------------------------
struct XRootDQuery
{
static const uint16_t SIDManager = 1001; //!< returns the SIDManager object
static const uint16_t ServerFlags = 1002; //!< returns server flags
static const uint16_t ProtocolVersion = 1003; //!< returns the protocol version
};
Expand Down Expand Up @@ -93,7 +92,8 @@ namespace XrdCl
//------------------------------------------------------------------------
//! Initialize channel
//------------------------------------------------------------------------
virtual void InitializeChannel( AnyObject &channelData );
virtual void InitializeChannel( AnyObject &channelData ); //< we keep this as we need to implement the abstract method
virtual void InitializeChannel( const URL &url, AnyObject &channelData );

//------------------------------------------------------------------------
//! Finalize channel
Expand Down

0 comments on commit 8926eb8

Please sign in to comment.