DPL: fix DataDescriptorMatcher and the associated query builder

* UB in strchr
* protect against unset optional
* reset per-query state
* avoid unneeded copy

Author: ktf

Framework/Core/src/DataDescriptorMatcher.cxx

@@ -156,7 +156,8 @@ DataDescriptorMatcher::DataDescriptorMatcher(DataDescriptorMatcher const& other)
   } else if (auto pval2 = std::get_if<SubSpecificationTypeValueMatcher>(&other.mLeft)) {
     mLeft = *pval2;
   } else if (auto pval3 = std::get_if<std::unique_ptr<DataDescriptorMatcher>>(&other.mLeft)) {
-    mLeft = std::move(std::make_unique<DataDescriptorMatcher>(*pval3->get()));
+    // A moved-from node keeps the unique_ptr alternative, only null.
+    mLeft = *pval3 ? std::make_unique<DataDescriptorMatcher>(**pval3) : nullptr;
   } else if (auto pval4 = std::get_if<ConstantValueMatcher>(&other.mLeft)) {
     mLeft = *pval4;
   } else if (auto pval5 = std::get_if<StartTimeValueMatcher>(&other.mLeft)) {
@@ -173,7 +174,8 @@ DataDescriptorMatcher::DataDescriptorMatcher(DataDescriptorMatcher const& other)
   } else if (auto pval2 = std::get_if<SubSpecificationTypeValueMatcher>(&other.mRight)) {
     mRight = *pval2;
   } else if (auto pval3 = std::get_if<std::unique_ptr<DataDescriptorMatcher>>(&other.mRight)) {
-    mRight = std::move(std::make_unique<DataDescriptorMatcher>(*pval3->get()));
+    // A moved-from node keeps the unique_ptr alternative, only null.
+    mRight = *pval3 ? std::make_unique<DataDescriptorMatcher>(**pval3) : nullptr;
   } else if (auto pval4 = std::get_if<ConstantValueMatcher>(&other.mRight)) {
     mRight = *pval4;
   } else if (auto pval5 = std::get_if<StartTimeValueMatcher>(&other.mRight)) {

Framework/Core/src/DataDescriptorQueryBuilder.cxx

@@ -161,7 +161,7 @@ std::vector<InputSpec> DataDescriptorQueryBuilder::parse(char const* config)
         lifetime = Lifetime::Sporadic;
       }
     }
-    return InputSpec{binding, std::move(*lastMatcher.release()), lifetime, attributes};
+    return InputSpec{binding, std::move(*lastMatcher), lifetime, attributes};
   };
 
   auto pushMatcher = [&nodes, &states](auto&& matcher) {
@@ -201,6 +201,12 @@ std::vector<InputSpec> DataDescriptorQueryBuilder::parse(char const* config)
       case IN_BEGIN_MATCHER: {
         nodes.clear();
         attributes.clear();
+        // Reset per-query state, so a ;-separated query cannot inherit it.
+        currentBinding.reset();
+        currentOrigin.reset();
+        currentDescription.reset();
+        currentSubSpec.reset();
+        currentTimeModulo.reset();
         pushState(IN_BEGIN_BINDING);
       } break;
       case IN_BEGIN_BINDING: {
@@ -210,7 +216,7 @@ std::vector<InputSpec> DataDescriptorQueryBuilder::parse(char const* config)
       case IN_END_BINDING: {
         // We are at the end of the string already.
         // This is really an origin...
-        if (strchr("\0/;", *next)) {
+        if (memchr("\0/;", *next, 3)) {
           pushState(IN_END_ORIGIN);
           continue;
         }
@@ -286,7 +292,8 @@ std::vector<InputSpec> DataDescriptorQueryBuilder::parse(char const* config)
           error("Remove trailing ;");
           continue;
         }
-        result.push_back(buildMatchingTree(*currentBinding, attributes));
+        // A query without a binding, e.g. "TST/A1", is named after its origin.
+        result.push_back(buildMatchingTree(currentBinding.value_or(currentOrigin.value_or("")), attributes));
         if (*cur == '\0') {
           pushState(IN_END_QUERY);
         } else if (*cur == ';') {

Framework/Core/src/InputSpec.cxx

@@ -83,7 +83,8 @@ InputSpec::InputSpec(std::string binding_,
                      enum Lifetime lifetime_,
                      std::vector<ConfigParamSpec> const& metadata_)
   : binding{binding_},
-    matcher{matcher_},
+    // matcher_ is an lvalue here, so without the move this deep copies the tree.
+    matcher{std::move(matcher_)},
     lifetime{lifetime_},
     metadata{metadata_}
 {

Framework/Core/test/test_DataDescriptorMatcher.cxx

@@ -671,6 +671,28 @@ TEST_CASE("DataQuery")
   REQUIRE(result5[0].metadata[2].defaultValue.get<std::string>() == "value3");
 }
 
+TEST_CASE("DataQueryNoBinding")
+{
+  // Without a binding the first token is the origin, and the binding defaults to it.
+  auto result0 = DataDescriptorQueryBuilder::parse("TST/A1");
+  REQUIRE(result0.size() == 1);
+  REQUIRE(result0[0].binding == "TST");
+  auto concrete0 = DataSpecUtils::asConcreteDataTypeMatcher(result0[0]);
+  REQUIRE(concrete0.origin.as<std::string>() == "TST");
+  REQUIRE(concrete0.description.as<std::string>() == "A1");
+
+  // A bare origin, and a bindingless query next to a bound one.
+  auto result1 = DataDescriptorQueryBuilder::parse("TST");
+  REQUIRE(result1.size() == 1);
+  REQUIRE(result1[0].binding == "TST");
+
+  auto result2 = DataDescriptorQueryBuilder::parse("TST;x:FOO/BAR");
+  REQUIRE(result2.size() == 2);
+  REQUIRE(result2[0].binding == "TST");
+  REQUIRE(result2[1].binding == "x");
+  REQUIRE(DataSpecUtils::asConcreteDataTypeMatcher(result2[1]).origin.as<std::string>() == "FOO");
+}
+
 TEST_CASE("DataQueryLifetime")
 {
   auto result0 = DataDescriptorQueryBuilder::parse("x:TST/A1?lifetime=timeframe");