Skip to content

Commit

Permalink
applied patch fixing XPCGetProtocol issue across threads
Browse files Browse the repository at this point in the history 
Gents,

I think I've found a glitch in the MOOSDB source code. This applies to MOOS-IvP 14.7.1 (the latest on moos-ivp.org) with asynchronous clients support active. The problem is more evident when the code is cross compiled for ARM (at least from my experience).
The effects of the bug are MOOSDB not starting correctly and continuously dumping error messages regarding a problem in listening for connection from the TCP socket. Actually the output is as follows:

# ./MOOSDB
------------------- MOOSDB V10 -------------------
Hosting  community                "#1"
Name look up is                   off
Asynchronous support is           on
Connect to this server on port    9000
--------------------------------------------------
network performance data published on localhost:9020
listen with "nc -u -lk 9020"
Exception Thrown in listen loop: Error Listening To Socket. Operation not supported
Exception Thrown in listen loop: Error Listening To Socket. Operation not supported
Exception Thrown in listen loop: Error Listening To Socket. Operation not supported
Exception Thrown in listen loop: Error Listening To Socket. Operation not supported
Exception Thrown in listen loop: Error Listening To Socket. Operation not supported
Exception Thrown in listen loop: Error Listening To Socket. Operation not supported
Exception Thrown in listen loop: Error Listening To Socket. Operation not supported
Exception Thrown in listen loop: Error Listening To Socket. Operation not supported
Exception Thrown in listen loop: Error Listening To Socket. Operation not supported
(goes on endlessly)

The cause was identified in an *UDP* socket asked to listen for connections....
After digging a bit in the code I found the problem in XPCGetProtocol. The constructors of this class are concurrently called by two threads, one creating a TCP socket, the other an UDP one. The class has proper mutex locking, BUT uses, as inner data member, the ret value of the system function "getprotobyname()", which returns info about protocols as a pointer to a statically allocated struct (thus creating a non protected shared data between the two concurrent instances).
Making local copies (in XPCGetProtocol) of the protocol name and number solved the problem.

Please let me know if you agree with my analysis.

Best ragards,

Alberto Grati
Engineering Department
E-mail: [email protected]<mailto:[email protected]%20>
  • Loading branch information
@pmnewman
pmnewman committed May 4, 2015
1 parent 29d2cc7 commit fd517c5
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 70 deletions.
105 changes: 60 additions & 45 deletions Core/libMOOS/Comms/XPCGetProtocol.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,75 +30,90 @@
//
////////////////////////// END_GPL //////////////////////////////////
#include "MOOS/libMOOS/Comms/XPCGetProtocol.h"
#include "MOOS/libMOOS/Utils/MOOSScopedLock.h"

CMOOSLock _ProtocolLock;
CMOOSLock XPCGetProtocol::_ProtocolLock;

XPCGetProtocol::XPCGetProtocol(const char *_sName)
{
MOOS::ScopedLock L(_ProtocolLock);

#ifdef UNIX
cIteratorFlag = 0;
#endif

// Retrieves the protocol structure by name
protocolPtr = getprotobyname(_sName);

struct protoent* ent = getprotobyname(_sName);
if (ent == NULL)
{
XPCException exceptObject("Could Not Get Protocol By Name");
throw exceptObject;
return;
}
_index = 0;
_protocols.push_back(XPCGetProtocol::ProtoEnt(ent));
}

XPCGetProtocol::XPCGetProtocol(int _iProtocol)
{
MOOS::ScopedLock L(_ProtocolLock);

#ifdef UNIX
cIteratorFlag = 0;
#endif
MOOS::ScopedLock L(_ProtocolLock);

// Retrieves the protocol structure by number
protocolPtr = getprotobynumber(_iProtocol);
if (protocolPtr == NULL)
{
XPCException exceptObject("Could Not Get Protocol By Number");
throw exceptObject;
return;
}
// Retrieves the protocol structure by number
struct protoent* ent = getprotobynumber(_iProtocol);
if (ent == NULL)
{
XPCException exceptObject("Could Not Get Protocol By Number");
throw exceptObject;
return;
}
_index = 0;
_protocols.push_back(XPCGetProtocol::ProtoEnt(ent));
}

XPCGetProtocol::~XPCGetProtocol()
{
MOOS::ScopedLock L(_ProtocolLock);
#ifdef UNIX
endprotoent();
#endif
}


#ifdef UNIX
void XPCGetProtocol::vOpenProtocolDb()
{
MOOS::ScopedLock L(_ProtocolLock);
void XPCGetProtocol::vOpenProtocolDb()
{
MOOS::ScopedLock L(_ProtocolLock);

endprotoent();
cIteratorFlag = 1;
setprotoent(1);
endprotoent();
setprotoent(1);
struct protoent* ent = 0;
_index = -1; // call cGetNextProtocol before accessing the first one...
_protocols.clear();
while ((ent = getprotoent())) {
_protocols.push_back(XPCGetProtocol::ProtoEnt(ent));
}
endprotoent();
}

// Iterates through the list of protocols
char XPCGetProtocol::cGetNextProtocol()
{
MOOS::ScopedLock L(_ProtocolLock);

if (cIteratorFlag == 1)
{
if ((protocolPtr = getprotoent()) == NULL)
return 0;
else
return 1;
}
return 0;
}
// Iterates through the list of protocols
char XPCGetProtocol::cGetNextProtocol()
{
MOOS::ScopedLock L(_ProtocolLock);

if (_index + 1 >= static_cast<int>(_protocols.size())) return 0;
++_index;
return 1;
}
#endif

XPCGetProtocol::ProtoEnt::ProtoEnt(struct protoent const* ent):
_name(ent? ent->p_name: ""),
_number(ent? ent->p_proto: 0)
{
if (ent == 0) return;
for (char** alias = ent->p_aliases; *alias; alias++) {
_aliases.push_back(std::string(*alias));
}
}
XPCGetProtocol::ProtoEnt::~ProtoEnt()
{
}

std::string const& XPCGetProtocol::ProtoEnt::name() const {
return _name;
}

int XPCGetProtocol::ProtoEnt::number() const {
return _number;
}
65 changes: 40 additions & 25 deletions Core/libMOOS/Comms/include/MOOS/libMOOS/Comms/XPCGetProtocol.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,44 +36,59 @@
#endif

#include "XPCException.h"
#include "MOOS/libMOOS/Utils/MOOSScopedLock.h"
#include <string>
#include <vector>

class XPCGetProtocol
{
private:
static CMOOSLock _ProtocolLock;
// Helper class: encapsulates a copy of the protoent structure
class ProtoEnt {
private:
std::string _name;
std::vector<std::string> _aliases;
int _number;

public:
ProtoEnt(struct protoent const* ent);
~ProtoEnt();
std::string const& name() const;
int number() const;
};
int _index;
std::vector<ProtoEnt> _protocols;
public:
#ifdef UNIX
char cIteratorFlag; // Protocol database iteration flag
#endif
struct protoent *protocolPtr; // Pointer to protocol database entry
public:
#ifdef UNIX
// Default constructor. Opens the protocol database
XPCGetProtocol()
{
vOpenProtocolDb();
}
// Default constructor. Opens the protocol database
XPCGetProtocol()
{
vOpenProtocolDb();
}
#endif

// Constructor. Returns the protoent structure given the protocol name.
XPCGetProtocol(const char *_sName);
// Constructor. Returns the protoent structure given the protocol name.
XPCGetProtocol(const char *_sName);

// Constructor. Returns the protoent structure given the protocol number
XPCGetProtocol(int _iProtocol);
// Constructor. Returns the protoent structure given the protocol number
XPCGetProtocol(int _iProtocol);

// Destructor closes the database connection
~XPCGetProtocol();
// Destructor closes the database connection
~XPCGetProtocol();

#ifdef UNIX
// Opens the protocol database and sets the cIteratorFlag to true
void vOpenProtocolDb();
// Opens and reads the protocol database
void vOpenProtocolDb();

// Iterates through the list of protocols
char cGetNextProtocol();
// Iterates through the list of protocols
char cGetNextProtocol();
#endif

// Returns the protocol name
char *sGetProtocolName() { return protocolPtr->p_name; }
// Returns the protocol name
char const* sGetProtocolName() { return _index >= static_cast<int>(_protocols.size())? "": _protocols[_index].name().c_str(); }

// Returns the protcol number
int iGetProtocolNumber() { return protocolPtr->p_proto; }
// Returns the protcol number
int iGetProtocolNumber() { return _index >= static_cast<int>(_protocols.size())? 0: _protocols[_index].number(); }
};

#endif

0 comments on commit fd517c5

Please sign in to comment.