Skip to content

Commit

Permalink
Merge pull request #31806 from vespa-engine/balder/avoid-trying-to-ta…
Browse files Browse the repository at this point in the history 
…ke-substring-outside-of-string_view

Balder/avoid trying to take substring outside of string view
  • Loading branch information
@baldersheim
baldersheim authored Jul 2, 2024
2 parents c1e7fff + d2605a9 commit 9d554b9
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 61 deletions.
69 changes: 32 additions & 37 deletions document/src/tests/documentselectparsertest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,12 @@ class DocumentSelectParserTest : public ::testing::Test {

~DocumentSelectParserTest() override;

Document::SP createDoc(
static Document::SP createDoc(
vespalib::stringref doctype, vespalib::stringref id, uint32_t hint,
double hfloat, vespalib::stringref hstr, vespalib::stringref cstr,
uint64_t hlong = 0);

DocumentUpdate::SP createUpdate(
static DocumentUpdate::SP createUpdate(
const std::string& doctype, const std::string& id, uint32_t hint,
const std::string& hstr);

Expand Down Expand Up @@ -113,10 +113,9 @@ void DocumentSelectParserTest::SetUp()
_parser = std::make_unique<select::Parser>(*_repo, _bucketIdFactory);
}

Document::SP DocumentSelectParserTest::createDoc(
vespalib::stringref doctype, vespalib::stringref id, uint32_t hint,
double hfloat, vespalib::stringref hstr, vespalib::stringref cstr,
uint64_t hlong)
Document::SP
DocumentSelectParserTest::createDoc(vespalib::stringref doctype, vespalib::stringref id, uint32_t hint, double hfloat,
vespalib::stringref hstr, vespalib::stringref cstr, uint64_t hlong)
{
const DocumentType* type = _repo->getDocumentType(doctype);
auto doc = std::make_shared<Document>(*_repo, *type, DocumentId(id));
Expand All @@ -131,9 +130,8 @@ Document::SP DocumentSelectParserTest::createDoc(
return doc;
}

DocumentUpdate::SP DocumentSelectParserTest::createUpdate(
const std::string& doctype, const std::string& id, uint32_t hint,
const std::string& hstr)
DocumentUpdate::SP
DocumentSelectParserTest::createUpdate(const std::string& doctype, const std::string& id, uint32_t hint, const std::string& hstr)
{
const DocumentType* type = _repo->getDocumentType(doctype);
auto doc = std::make_shared<DocumentUpdate>(*_repo, *type, DocumentId(id));
Expand Down Expand Up @@ -270,7 +268,7 @@ void doVerifyParse(select::Node *node, const std::string &query, const char *exp
EXPECT_EQ(exp, clonedStr.str());
}

void verifySimpleParse(const std::string& query, const char* expected = 0) {
void verifySimpleParse(const std::string& query, const char* expected = nullptr) {
BucketIdFactory factory;
select::simple::SelectionParser parser(factory);
std::string message("Query "+query+" failed to parse.");
Expand All @@ -279,31 +277,32 @@ void verifySimpleParse(const std::string& query, const char* expected = 0) {
doVerifyParse(node.get(), query, expected);
}

void verifyParse(const std::string& query, const char* expected = 0) {
void verifyParse(const std::string& query, const char* expected = nullptr) {
BucketIdFactory factory;
select::Parser parser(*_repo, factory);
std::unique_ptr<select::Node> node(parser.parse(query));
doVerifyParse(node.get(), query, expected);
}

void verifyFailedParse(const std::string& query, const std::string& error) {
try{
BucketIdFactory factory;
TestDocRepo test_repo;
select::Parser parser(test_repo.getTypeRepo(), factory);
std::unique_ptr<select::Node> node(parser.parse(query));
FAIL() << "Expected exception parsing query '" << query << "'";
} catch (select::ParsingFailedException& e) {
std::string message(e.what());
if (message.size() > error.size())
message = message.substr(0, error.size());
std::string failure("Expected: " + error + "\n- Actual : "
+ std::string(e.what()));
EXPECT_EQ(error, message) << failure;
}
void verifyFailedParse(const std::string& query, const std::string& error) {
try {
BucketIdFactory factory;
TestDocRepo test_repo;
select::Parser parser(test_repo.getTypeRepo(), factory);
std::unique_ptr<select::Node> node(parser.parse(query));
FAIL() << "Expected exception parsing query '" << query << "'";
} catch (select::ParsingFailedException& e) {
std::string message(e.what());
if (message.size() > error.size())
message = message.substr(0, error.size());
std::string failure("Expected: " + error + "\n- Actual : "
+ std::string(e.what()));
EXPECT_EQ(error, message) << failure;
}
}

}

TEST_F(DocumentSelectParserTest, test_syntax_error_reporting)
{
createDocs();
Expand Down Expand Up @@ -355,30 +354,26 @@ TEST_F(DocumentSelectParserTest, testParseTerminals)

// Test string value
verifyParse("testdoctype1.headerval == \"test\"");
std::unique_ptr<select::Node> node(
_parser->parse("testdoctype1.headerval == \"test\""));
const select::Compare& compnode(
dynamic_cast<const select::Compare&>(*node));
const select::FieldValueNode& fnode(
dynamic_cast<const select::FieldValueNode&>(compnode.getLeft()));
const select::StringValueNode& vnode(
dynamic_cast<const select::StringValueNode&>(compnode.getRight()));
std::unique_ptr<select::Node> node = _parser->parse("testdoctype1.headerval == \"test\"");
const auto & compnode = dynamic_cast<const select::Compare&>(*node);
const auto & fnode = dynamic_cast<const select::FieldValueNode&>(compnode.getLeft());
const auto & vnode = dynamic_cast<const select::StringValueNode&>(compnode.getRight());

EXPECT_EQ(vespalib::string("headerval"), fnode.getFieldName());
EXPECT_EQ(vespalib::string("test"), vnode.getValue());
// Test whitespace
verifyParse("testdoctype1.headerval == \"te st \"");
verifyParse(" \t testdoctype1.headerval\t== \t \"test\"\t",
"testdoctype1.headerval == \"test\"");

// Test escaping
verifyParse("testdoctype1.headerval == \"tab\\ttest\"");
verifyParse("testdoctype1.headerval == \"tab\\x09test\"",
"testdoctype1.headerval == \"tab\\ttest\"");
verifyParse("testdoctype1.headerval == \"tab\\x055test\"");
node = _parser->parse("testdoctype1.headerval == \"\\tt\\x48 \\n\"");
select::Compare& escapednode(dynamic_cast<select::Compare&>(*node));
const select::StringValueNode& escval(
dynamic_cast<const select::StringValueNode&>(escapednode.getRight()));
const auto & escval = dynamic_cast<const select::StringValueNode&>(escapednode.getRight());
EXPECT_EQ(vespalib::string("\ttH \n"), escval.getValue());
// Test <= <, > >=
verifyParse("testdoctype1.headerval >= 123");
Expand Down Expand Up @@ -957,7 +952,7 @@ namespace {
std::ostringstream data;

public:
~TestVisitor() {}
~TestVisitor() override {}

void visitConstant(const select::Constant& node) override {
data << "CONSTANT(" << node << ")";
Expand Down
43 changes: 24 additions & 19 deletions document/src/vespa/document/select/simpleparser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,21 @@

namespace document::select::simple {

size_t eatWhite(const char * s, size_t len)
{
size_t pos(0);
for (;(pos < len) && isspace(s[pos]); pos++);
return pos;
namespace {
size_t eatWhite(const char *s, size_t len) {
size_t pos(0);
for (; (pos < len) && isspace(s[pos]); pos++);
return pos;
}

bool icmp(char c, char l) {
return tolower(c) == l;
}
}

bool icmp(char c, char l)
{
return tolower(c) == l;
void
Parser::setRemaining(vespalib::stringref s, size_t fromPos) {
_remaining = s.substr(std::min(fromPos, s.size()));
}

bool IdSpecParser::parse(vespalib::stringref s)
Expand All @@ -42,7 +47,7 @@ bool IdSpecParser::parse(vespalib::stringref s)
((len == 9) && (strncasecmp(&s[startPos], "namespace", 9) == 0)))
{
retval = true;
setValue(ValueNode::UP(new IdValueNode(_bucketIdFactory, "id", s.substr(startPos, len), widthBits, divisionBits)));
setValue(std::make_unique<IdValueNode>(_bucketIdFactory, "id", s.substr(startPos, len), widthBits, divisionBits));
} else {
pos = startPos;
}
Expand All @@ -58,7 +63,7 @@ bool IdSpecParser::parse(vespalib::stringref s)
case ' ':
{
retval = true;
setValue(ValueNode::UP(new IdValueNode(_bucketIdFactory, "id", "")));
setValue(std::make_unique<IdValueNode>(_bucketIdFactory, "id", ""));
}
break;
default:
Expand All @@ -67,7 +72,7 @@ bool IdSpecParser::parse(vespalib::stringref s)
}
}
}
setRemaining(s.substr(pos));
setRemaining(s, pos);
return retval;
}

Expand Down Expand Up @@ -111,7 +116,7 @@ bool OperatorParser::parse(vespalib::stringref s)
retval = false;
}
}
setRemaining(s.substr(pos));
setRemaining(s, pos);
return retval;
}

Expand All @@ -132,10 +137,10 @@ bool StringParser::parse(vespalib::stringref s)
if (s[pos] == '"') {
pos++;
retval = true;
setValue(ValueNode::UP(new StringValueNode(str)));
setValue(std::make_unique<StringValueNode>(str));
}
}
setRemaining(s.substr(pos+1));
setRemaining(s, pos+1);
}
return retval;
}
Expand All @@ -145,7 +150,7 @@ bool IntegerParser::parse(vespalib::stringref s)
bool retval(false);
size_t pos(eatWhite(s.data(), s.size()));
if (pos < s.size()) {
char * err(NULL);
char * err(nullptr);
errno = 0;
bool isHex((s.size() - pos) && (s[pos] == '0') && (s[pos+1] == 'x'));
int64_t v = isHex
Expand All @@ -155,10 +160,10 @@ bool IntegerParser::parse(vespalib::stringref s)
if ((errno == 0) && (pos+len <= s.size())) {
retval = true;
pos += len;
setValue(ValueNode::UP(new IntegerValueNode(v, false)));
setValue(std::make_unique<IntegerValueNode>(v, false));
}
}
setRemaining(s.substr(pos));
setRemaining(s, pos);
return retval;
}

Expand All @@ -172,14 +177,14 @@ bool SelectionParser::parse(vespalib::stringref s)
if (id.isUserSpec()) {
IntegerParser v;
if (v.parse(op.getRemaining())) {
setNode(Node::UP(new Compare(id.stealValue(), *op.getOperator(), v.stealValue(), _bucketIdFactory)));
setNode(std::make_unique<Compare>(id.stealValue(), *op.getOperator(), v.stealValue(), _bucketIdFactory));
retval = true;
}
setRemaining(v.getRemaining());
} else {
StringParser v;
if (v.parse(op.getRemaining())) {
setNode(Node::UP(new Compare(id.stealValue(), *op.getOperator(), v.stealValue(), _bucketIdFactory)));
setNode(std::make_unique<Compare>(id.stealValue(), *op.getOperator(), v.stealValue(), _bucketIdFactory));
retval = true;
}
setRemaining(v.getRemaining());
Expand Down
13 changes: 8 additions & 5 deletions document/src/vespa/document/select/simpleparser.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,19 @@ namespace document::select::simple {

class Parser {
public:
virtual ~Parser() { }
virtual ~Parser() = default;
virtual bool parse(vespalib::stringref s) = 0;
vespalib::stringref getRemaining() const { return _remaining; }
protected:
void setRemaining(vespalib::stringref s) { _remaining = s; }
void setRemaining(vespalib::stringref s, size_t fromPos);
private:
vespalib::stringref _remaining;
};

class NodeResult {
public:
//TODO Dirty, should force use of std::move
Node::UP getNode() { return std::move(_node); }
protected:
void setNode(Node::UP node) { _node = std::move(node); }
Expand All @@ -30,6 +32,7 @@ class NodeResult {

class ValueResult {
public:
//TODO Dirty, should force use of std::move
ValueNode::UP stealValue() { return std::move(_value); }
const ValueNode & getValue() const { return *_value; }
protected:
Expand All @@ -41,8 +44,8 @@ class ValueResult {
class IdSpecParser : public Parser, public ValueResult
{
public:
IdSpecParser(const BucketIdFactory& bucketIdFactory) :
_bucketIdFactory(bucketIdFactory)
explicit IdSpecParser(const BucketIdFactory& bucketIdFactory) noexcept
: _bucketIdFactory(bucketIdFactory)
{}
bool parse(vespalib::stringref s) override;
const IdValueNode & getId() const { return static_cast<const IdValueNode &>(getValue()); }
Expand Down Expand Up @@ -75,8 +78,8 @@ class IntegerParser : public Parser, public ValueResult
class SelectionParser : public Parser, public NodeResult
{
public:
SelectionParser(const BucketIdFactory& bucketIdFactory) :
_bucketIdFactory(bucketIdFactory)
explicit SelectionParser(const BucketIdFactory& bucketIdFactory) noexcept
: _bucketIdFactory(bucketIdFactory)
{}
bool parse(vespalib::stringref s) override;
private:
Expand Down

0 comments on commit 9d554b9

Please sign in to comment.