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

[WIP] Portname validation #1508

Closed
wants to merge 3 commits into from
Closed

[WIP] Portname validation #1508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9a1ef44
Added check on contact name before Port::open() and Network::connect()
damn1 Jan 2, 2018
db17ef0
added some tests to check valid names for port registering
damn1 Jan 3, 2018
b5478f8
drop checks on leading '=' and unify category check
damn1 Jan 3, 2018
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
23 changes: 23 additions & 0 deletions src/libYARP_OS/include/yarp/os/Contact.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,29 @@ class YARP_OS_API yarp::os::Contact {
*/
static Contact fromString(const ConstString& txt);

/**
* @brief isValidRegisteredName
*
* Checks if a provided name match the syntax of possibly
* registered port names. The syntax is defined into the function code.
*
* @param name char sequence with the name to check
* @return true if the name could correspond to a registered contact
*/
static bool isValidRegisteredName(const char* name);

/**
* @brief isValidRegistrationName
*
* Checks if a provided name is valid to enter registration phase.
* Valid syntax includes valid registered names plus anonymous ports
* and plain char sequences for topics.
*
* @param name char sequence with the name to check
* @return true if the contact can be registered by YARP network
*/
static bool isValidRegistrationName(const char* name);

/** @} */
/** @{ */

Expand Down
82 changes: 82 additions & 0 deletions src/libYARP_OS/src/Contact.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <cstdlib>
#include <cstdio>
#include <cstring>
#include <regex>

#if defined(YARP_HAS_ACE)
# include <ace/INET_Addr.h>
Expand Down Expand Up @@ -197,6 +198,87 @@ Contact Contact::fromString(const ConstString& txt)
return c;
}

bool Contact::isValidRegistrationName(const char* name)
{
if(strcmp(name,"") == 0) // anonymous
return true;

const std::string port_name_s(name);
std::regex port_exp("([a-zA-Z0-9]+(?:[_][a-zA-Z0-9]+)*)" // plain expression "abc[_def]" without leading '/', allowed for topics
"|" // OR
"(\\.\\.\\.)"); // "..." is accepted as registration name, a default one is assigned

std::smatch port_match;
if (std::regex_match(port_name_s, port_match, port_exp))
return true;

return isValidRegisteredName(name); // otherwise, we need a good final name
}

bool Contact::isValidRegisteredName(const char* name)
{
const std::string port_name_s(name);
std::regex port_exp(
Copy link
Member

Choose a reason for hiding this comment

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

Do you have any idea about the order of magnitute necessary to build the std::regex object? If it is not trivial, we could think about having a regex singleton to avoid rebuilding the regex for each check.

"(?:([a-zA-Z0-9]+(?:[_][a-zA-Z0-9]+)*):/)?" // carrier_name:/ - optional
"("
"(" // /port[category]@/node~wire syntax
"((?:/[a-zA-Z0-9]+(?:[_:][a-zA-Z0-9]+)*)+)" // port_name - necessary
"(?:"
"([\\+|-][1]?)?" // category - optional, "+" | "-" | "+1" | "-1"
"(@((?:/[a-zA-Z0-9]+(?:[_:][a-zA-Z0-9]+)*)*))"
// @node_name - optional, could also be ""
"(~([_a-zA-Z0-9]+))?" // ~wire_type - optional
")?"
")"
"|" // OR
"(" // /node[category]#/port~wire syntax
"((?:/[a-zA-Z0-9]+(?:[_:][a-zA-Z0-9]+)*)+)" // node_name - necessary
"([\\+|-][1]?)?" // category - optional, "+" | "-" | "+1" | "-1"
"\x23" // # - necessary
"((?:/[a-zA-Z0-9]+(?:[_:][a-zA-Z0-9]+)*)+)" // /port_name - necessary
"(~([_a-zA-Z0-9]+))?" // ~wire_type - optional
")"
"|" // OR
"(" // /node=[category]/port~wire syntax
"((?:/[a-zA-Z0-9]+(?:[_:][a-zA-Z0-9]+)*)+)" // /node_name - necessary
"=" // = - necessary
"([\\+|-][1]?)?" // category - optional, "+" | "-" | "+1" | "-1"
"((?:/[a-zA-Z0-9]+(?:[_:][a-zA-Z0-9]+)*)+)" // /port_name - necessary
"(~([_a-zA-Z0-9]+))?" // ~wire_type - optional
")"
"|" // OR
"(/" // "/machine:NNN/" syntax
"[a-zA-Z0-9]+(?:\\.[a-zA-Z0-9]+)*" // machine name - "machine.domain.name" or IP addr
":[\\d]+/?" // port number
")"
")");

// Matching
std::smatch port_match;
bool match = std::regex_match(port_name_s, port_match, port_exp);

if (match)
{
return true;
}
else // basic error reporting:
{
std::cout << "\"" << port_name_s << "\" does not match the correct pattern for port definition.\n";
Copy link
Member

Choose a reason for hiding this comment

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

Given that this is an error, perhaps it is better to use std::cerr or one of the countless logging mechanism that can be used in libYARP_OS ?

std::regex_search(port_name_s, port_match, port_exp);
if (port_match[0].str() != "")
{
std::cout << "Check for the error in prefix or suffix of the matched name:\n";
std::string prefix = (port_match.prefix().str().empty() ? "" : ("[" + port_match.prefix().str() + "] "));
std::string suffix = (port_match.suffix().str().empty() ? "" : (" [" + port_match.suffix().str() + "]"));
std::cout << prefix << port_match[0].str() << suffix << std::endl;
}
else
{
std::cout << "There is no matching pattern. Check the correct syntax --> where <-- for port names.\n";
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 not fully understand this message.

}
return false;
}
}


ConstString Contact::getName() const
Expand Down
21 changes: 18 additions & 3 deletions src/libYARP_OS/src/NestedContact.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,24 @@ bool NestedContact::fromString(const ConstString& nFullName) {
nodeName = fullName.substr(0, idx);
nestedName = fullName.substr(idx+1, fullName.length());
char ch = nodeName[nodeName.length()-1];
if (ch=='-'||ch=='+') {
category += ch;
nodeName = nodeName.substr(0, nodeName.length()-1);
if (ch=='-'||ch=='+'||ch=='1') {
size_t offset = 1;
bool ok = true;
if (ch=='1') {
ok = false;
if (nodeName.length()>=2) {
char ch0 = nodeName[nodeName.length()-2];
if (ch0=='-'||ch0=='+') {
offset++;
category += ch0;
ok = true;
}
}
}
if (ok) {
category += ch;
nodeName = nodeName.substr(0, nodeName.length()-offset);
}
}
return true;
}
Expand Down
4 changes: 2 additions & 2 deletions src/libYARP_OS/src/Network.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,12 +354,12 @@ static int metaConnect(const ConstString& src,
CARRIER_DEBUG("DYNAMIC_SRC: name=%s, carrier=%s\n", dynamicSrc.getName().c_str(), dynamicSrc.getCarrier().c_str());
CARRIER_DEBUG("DYNAMIC_DST: name=%s, carrier=%s\n", dynamicDest.getName().c_str(), dynamicDest.getCarrier().c_str());

if(!NetworkBase::isValidPortName(dynamicSrc.getName()))
if(!Contact::isValidRegisteredName(dynamicSrc.getName().c_str()))
{
fprintf(stderr, "Failure: no way to make connection, invalid source '%s'\n", dynamicSrc.getName().c_str());
return 1;
}
if(!NetworkBase::isValidPortName(dynamicDest.getName()))
if(!Contact::isValidRegisteredName(dynamicDest.getName().c_str()))
{
fprintf(stderr, "Failure: no way to make connection, invalid destination '%s'\n", dynamicDest.getName().c_str());
return 1;
Expand Down
11 changes: 7 additions & 4 deletions src/libYARP_OS/src/Port.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,10 @@ bool Port::openFake(const ConstString& name)

bool Port::open(const ConstString& name)
{
return open(Contact(name));
if(Contact::isValidRegistrationName(name.c_str())) {
return open(Contact::fromString(name));
}
return false;
}

bool Port::open(const Contact& contact, bool registerName)
Expand Down Expand Up @@ -127,7 +130,7 @@ bool Port::open(const Contact& contact, bool registerName,
PortCoreAdapter *currentCore = &(IMPL());
if (currentCore!=nullptr) {
currentCore->active = false;
if (n!="" && (n[0]!='/'||currentCore->includeNode) && n[0]!='=' && n!="..." && n.substr(0, 3)!="...") {
if (n!="" && (n[0]!='/'||currentCore->includeNode) && n!="..." && n.substr(0, 3)!="...") {
if (fakeName==nullptr) {
Nodes& nodes = NameClient::getNameClient().getNodes();
ConstString node_name = nodes.getActiveName();
Expand All @@ -137,15 +140,15 @@ bool Port::open(const Contact& contact, bool registerName,
}
}
}
if (n!="" && n[0]!='/' && n[0]!='=' && n!="..." && n.substr(0, 3)!="...") {
if (n!="" && n[0]!='/' && n!="..." && n.substr(0, 3)!="...") {
if (fakeName==nullptr) {
YARP_SPRINTF1(Logger::get(), error,
"Port name '%s' needs to start with a '/' character",
n.c_str());
return false;
}
}
if (n!="" && n!="..." && n[0]!='=' && n.substr(0, 3)!="...") {
if (n!="" && n!="..." && n.substr(0, 3)!="...") {
if (fakeName==nullptr) {
ConstString prefix = NetworkBase::getEnvironment("YARP_PORT_PREFIX");
if (prefix!="") {
Expand Down
9 changes: 3 additions & 6 deletions src/libYARP_serversql/src/NameServiceOnTriples.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,14 +273,11 @@ bool NameServiceOnTriples::cmdRegister(NameTripleState& act) {
at++;
}
lock();
if (port=="..." || (port.length()>0 && port[0]=='=')) {
if (port=="...") {
Contact c(port, carrier, machine, sock);
c = alloc->completePortName(c);
if (port =="...") {
port = c.getName();
} else {
port = c.getName() + port;
}
port = c.getName();

}
t.setNameValue("port",port.c_str());
act.mem.remove_query(t, nullptr);
Expand Down
56 changes: 56 additions & 0 deletions tests/libYARP_OS/PortTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,60 @@ class PortTest : public UnitTest {

virtual ConstString getName() override { return "PortTest"; }

void testName() {
// good names
checkTrue(Contact::isValidRegistrationName(""), "Void name is a good registration name.");
checkTrue(Contact::isValidRegistrationName("..."), "\"...\" name is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/port"), "\"/port\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/port:spec"), "\"/port:spec\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/p1/p2"), "\"/p1/p2\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/p1/p2/p3/p4"), "\"/p1/p2/p3/p4\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/p1/p2/p3:any/p4"), "\"/p1/p2/p3:any/p4\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/p1/p_2"), "\"/p1/p_2\" is a good name.");
checkTrue(Contact::isValidRegistrationName("/p1/p_2:sp1:ssp2"), "\"/p1/p_2:sp1:ssp2\" is a good name.");
checkTrue(Contact::isValidRegistrationName("carr://port"), "\"carr://port\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("carr://port/port:spec"), "\"carr://port:spec\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("carr://p1/p2"), "\"carr://p1/p2\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("fast_carr://p1/p2/p3/p4"), "\"fast_carr://p1/p2/p3/p4\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/n1#/p1/p2"), "\"/n1#/p1/p2\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/n1+#/p1"), "\"/n1+#/p1\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/n1+1#/p1"), "\"/n1+1#/p1\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/n1-1#/p1"), "\"/n1-1#/p1\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/n1=/p1"), "\"/n1=/p1\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/n1=-1/p1"), "\"/n1=-1/p1\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/n1=+/p1/p2:sp"), "\"/n1=+/p1/p2:sp\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/p@"), "\"/p@\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/p:s@/n~wire"), "\"/p:s@/n~wire\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/p-1@/n~wire"), "\"/p-1@/n~wire\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/machine:10005"), "\"machine:10005\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/mac12.at.domain:10005"), "\"mac12.at.domain:10005\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("/111.2.30.4:10005"), "\"111.2.30.4:10005\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("carr://11.12.13.14:10005"), "\"carr://11.12.13.14:10005\" is a good registration name.");
checkTrue(Contact::isValidRegistrationName("carr://11.12.13.14:10005/"),"\"carr://11.12.13.14:10005\" is a good registration name.");

// bad names:
checkFalse(Contact::isValidRegistrationName("/"), "\"/\" is not a good registration name.");
checkFalse(Contact::isValidRegistrationName("/p space"), "\"/p space\" is not a good registration name.");
checkFalse(Contact::isValidRegistrationName("/_"), "\"/_\" is not a good registration name.");
checkFalse(Contact::isValidRegistrationName("/:"), "\"/:\" is not a good registration name.");
checkFalse(Contact::isValidRegistrationName("/p_"), "\"/p_\" is a not good registration name.");
checkFalse(Contact::isValidRegistrationName("/p/"), "\"/p_\" is a not good registration name.");
checkFalse(Contact::isValidRegistrationName("/port:"), "\"/port:\" is not a good registration name.");
checkFalse(Contact::isValidRegistrationName("/str@ng&%port"), "\"/str@ng&%port\" is not a good registration name.");
checkFalse(Contact::isValidRegistrationName("str4ng&~c4rr://p1/p2"),"\"str4ng&~c4rr://p1/p2\" is not a good registration name.");
checkFalse(Contact::isValidRegistrationName("carr:/"), "\"carr:/ \" is not a good registration name.");
checkFalse(Contact::isValidRegistrationName("=+/p1/p2:sp"), "\"=+/p1/p2:sp\" is not a good registration name.");
checkFalse(Contact::isValidRegistrationName("#+/p1/p2:sp"), "\"#+/p1/p2:sp\" is not a good registration name.");
checkFalse(Contact::isValidRegistrationName("/n="), "\"/n=\" is not a good registration name.");
checkFalse(Contact::isValidRegistrationName("/n#"), "\"/n#\" is not a good registration name.");
checkFalse(Contact::isValidRegistrationName("="), "\"=\" is not a good registration name.");
checkFalse(Contact::isValidRegistrationName("#"), "\"#\" is not a good registration name.");
checkFalse(Contact::isValidRegistrationName("@"), "\"@\" is not a good registration name.");
checkFalse(Contact::isValidRegistrationName("machine:10005"), "\"machine:10005\" is not a good registration name.");
checkFalse(Contact::isValidRegistrationName("111.2.30.4:abc"), "\"111.2.30.4:abc\" is not a good registration name.");
checkFalse(Contact::isValidRegistrationName("carr:/11.12.13.14:69"),"\"carr:/11.12.13.14:69\" is not a good registration name.");
}

void testOpen() {
report(0,"checking opening and closing ports");
Port out, in;
Expand Down Expand Up @@ -1546,6 +1600,8 @@ class PortTest : public UnitTest {
#ifdef BROKEN_TEST
testTcp();
#endif
testName();

testOpen();
//bbb testReadBuffer();
testPair();
Expand Down