Bug fix/issues 2019 05 06 (#8913)

arangod/Agency/AgencyComm.cpp

@@ -72,7 +72,7 @@ static void addEmptyVPackObject(std::string const& name, VPackBuilder& builder)
 }
 
 const std::vector<std::string> AgencyTransaction::TypeUrl(
-    {"/read", "/write", "/transact", "/transient", "/config"});
+    {"/read", "/write", "/transact", "/transient"});
 
 // -----------------------------------------------------------------------------
 // --SECTION--                                                AgencyPrecondition

arangod/Aql/Ast.cpp

@@ -3502,8 +3502,9 @@ AstNode* Ast::nodeFromVPack(VPackSlice const& slice, bool copyStringValues) {
     node->members.reserve(static_cast<size_t>(it.size()));
 
     while (it.valid()) {
+      auto current = (*it);
       VPackValueLength nameLength;
-      char const* attributeName = it.key().getString(nameLength);
+      char const* attributeName = current.key.getString(nameLength);
 
       if (copyStringValues) {
         // create a copy of the string value
@@ -3513,7 +3514,7 @@ AstNode* Ast::nodeFromVPack(VPackSlice const& slice, bool copyStringValues) {
 
       node->addMember(
           createNodeObjectElement(attributeName, static_cast<size_t>(nameLength),
-                                  nodeFromVPack(it.value(), copyStringValues)));
+                                  nodeFromVPack(current.value, copyStringValues)));
       it.next();
     }
 

arangod/Cluster/CreateCollection.cpp

@@ -99,7 +99,7 @@ CreateCollection::CreateCollection(MaintenanceFeature& feature, ActionDescriptio
   }
 }
 
-CreateCollection::~CreateCollection(){};
+CreateCollection::~CreateCollection() {}
 
 bool CreateCollection::first() {
   auto const& database = _description.get(DATABASE);

arangod/Cluster/CreateCollection.h

@@ -39,9 +39,9 @@ class CreateCollection : public ActionBase {
 
   virtual ~CreateCollection();
 
-  virtual bool first() override final;
+  bool first() override final;
 
-  virtual void setState(ActionState state) override final;
+  void setState(ActionState state) override final;
 };
 
 }  // namespace maintenance

arangod/Cluster/SynchronizeShard.h

@@ -44,9 +44,9 @@ class SynchronizeShard : public ActionBase {
 
   virtual ~SynchronizeShard();
 
-  virtual bool first() override final;
+  bool first() override final;
 
-  virtual void setState(ActionState state) override final;
+  void setState(ActionState state) override final;
 
  private:
   arangodb::Result getReadLock(std::string const& endpoint, std::string const& database,

arangod/IResearch/IResearchRocksDBRecoveryHelper.cpp

@@ -275,8 +275,8 @@ void IResearchRocksDBRecoveryHelper::handleDeleteCF(uint32_t column_family_id,
 }
 
 void IResearchRocksDBRecoveryHelper::DeleteRangeCF(uint32_t column_family_id,
-                                                   const rocksdb::Slice& end_key,
+                                                   const rocksdb::Slice& begin_key,
-                                                   const rocksdb::Slice& begin_key) {
+                                                   const rocksdb::Slice& end_key) {
   // not needed for anything atm
 }
 

arangod/MMFiles/MMFilesCollection.cpp

@@ -641,13 +641,7 @@ MMFilesCollection::MMFilesCollection(LogicalCollection& logical,
   _path = mmfiles._path;
   _doCompact = mmfiles._doCompact;
   _maxTick = mmfiles._maxTick;
-  /*
+  
-    // Copy over index definitions
-    _indexes.reserve(mmfiles._indexes.size());
-    for (auto const& idx : mmfiles._indexes) {
-      _indexes.emplace_back(idx);
-    }
-  */
   TRI_ASSERT(!ServerState::instance()->isCoordinator());
   setCompactionStatus("compaction not yet started");
   //  not copied

arangod/RestHandler/RestUploadHandler.cpp

@@ -61,18 +61,7 @@ RestStatus RestUploadHandler::execute() {
     }
   }
 
-  std::string relativeString;
+  std::string relativeString = TRI_GetFilename(filename);
-  {
-    char* relative = TRI_GetFilename(filename.c_str());
-
-    if (relative == nullptr) {
-      THROW_ARANGO_EXCEPTION(TRI_ERROR_OUT_OF_MEMORY);
-    }
-
-    relativeString.append(relative);
-    TRI_FreeString(relative);
-  }
-
   arangodb::velocypack::StringRef bodyStr = _request->rawPayload();
   char const* body = bodyStr.data();
   size_t bodySize = bodyStr.size();

arangod/Scheduler/Scheduler.cpp

@@ -145,11 +145,12 @@ Scheduler::WorkHandle Scheduler::queueDelay(RequestLane lane, clock::duration de
 
   if (delay < std::chrono::milliseconds(1)) {
     // execute directly
-    queue(lane, [handler]() { handler(false); });
+    queue(lane, [handler = std::move(handler)]() { handler(false); });
     return nullptr;
   }
 
   auto item = std::make_shared<WorkItem>(std::move(handler), lane, this);
+  {
     std::unique_lock<std::mutex> guard(_cronQueueMutex);
     _cronQueue.emplace(clock::now() + delay, item);
 
@@ -157,6 +158,7 @@ Scheduler::WorkHandle Scheduler::queueDelay(RequestLane lane, clock::duration de
       // wakeup thread
       _croncv.notify_one();
     }
+  }
 
   return item;
 }

arangod/StorageEngine/PhysicalCollection.cpp

@@ -193,22 +193,23 @@ Result PhysicalCollection::mergeObjectsForUpdate(
   {
     VPackObjectIterator it(newValue, true);
     while (it.valid()) {
-      arangodb::velocypack::StringRef key(it.key());
+      auto current = *it;
-      if (!key.empty() && key[0] == '_' &&
+      arangodb::velocypack::StringRef key(current.key);
+      if (key.size() >= 3 && key[0] == '_' &&
           (key == StaticStrings::KeyString || key == StaticStrings::IdString ||
            key == StaticStrings::RevString ||
            key == StaticStrings::FromString || key == StaticStrings::ToString)) {
         // note _from and _to and ignore _id, _key and _rev
         if (isEdgeCollection) {
           if (key == StaticStrings::FromString) {
-            fromSlice = it.value();
+            fromSlice = current.value;
           } else if (key == StaticStrings::ToString) {
-            toSlice = it.value();
+            toSlice = current.value;
           }
         }  // else do nothing
       } else {
         // regular attribute
-        newValues.emplace(key, it.value());
+        newValues.emplace(key, current.value);
       }
 
       it.next();
@@ -269,9 +270,10 @@ Result PhysicalCollection::mergeObjectsForUpdate(
   {
     VPackObjectIterator it(oldValue, true);
     while (it.valid()) {
-      arangodb::velocypack::StringRef key(it.key());
+      auto current = (*it);
+      arangodb::velocypack::StringRef key(current.key);
       // exclude system attributes in old value now
-      if (!key.empty() && key[0] == '_' &&
+      if (key.size() >= 3 && key[0] == '_' &&
           (key == StaticStrings::KeyString || key == StaticStrings::IdString ||
            key == StaticStrings::RevString ||
            key == StaticStrings::FromString || key == StaticStrings::ToString)) {
@@ -283,12 +285,12 @@ Result PhysicalCollection::mergeObjectsForUpdate(
 
       if (found == newValues.end()) {
         // use old value
-        b.addUnchecked(key.data(), key.size(), it.value());
+        b.addUnchecked(key.data(), key.size(), current.value);
-      } else if (mergeObjects && it.value().isObject() && (*found).second.isObject()) {
+      } else if (mergeObjects && current.value.isObject() && (*found).second.isObject()) {
         // merge both values
         auto& value = (*found).second;
         if (keepNull || (!value.isNone() && !value.isNull())) {
-          VPackBuilder sub = VPackCollection::merge(it.value(), value, true, !keepNull);
+          VPackBuilder sub = VPackCollection::merge(current.value, value, true, !keepNull);
           b.addUnchecked(key.data(), key.size(), sub.slice());
         }
         // clear the value in the map so its not added again

arangod/Transaction/Methods.cpp

@@ -861,19 +861,19 @@ transaction::Methods::Methods(std::shared_ptr<transaction::Context> const& ctx,
 
   Result res;
   for (auto const& it : exclusiveCollections) {
-    res = addCollection(it, AccessMode::Type::EXCLUSIVE);
+    res = Methods::addCollection(it, AccessMode::Type::EXCLUSIVE);
     if (res.fail()) {
       THROW_ARANGO_EXCEPTION(res);
     }
   }
   for (auto const& it : writeCollections) {
-    res = addCollection(it, AccessMode::Type::WRITE);
+    res = Methods::addCollection(it, AccessMode::Type::WRITE);
     if (res.fail()) {
       THROW_ARANGO_EXCEPTION(res);
     }
   }
   for (auto const& it : readCollections) {
-    res = addCollection(it, AccessMode::Type::READ);
+    res = Methods::addCollection(it, AccessMode::Type::READ);
     if (res.fail()) {
       THROW_ARANGO_EXCEPTION(res);
     }

arangod/VocBase/vocbase.cpp

@@ -1744,14 +1744,10 @@ bool TRI_vocbase_t::IsAllowedName(bool allowSystem,
   for (char const* ptr = name.data(); length < name.size(); ++ptr, ++length) {
     bool ok;
     if (length == 0) {
-      if (allowSystem) {
+      ok = ('a' <= *ptr && *ptr <= 'z') || ('A' <= *ptr && *ptr <= 'Z') || (allowSystem && *ptr == '_');
-        ok = (*ptr == '_') || ('a' <= *ptr && *ptr <= 'z') || ('A' <= *ptr && *ptr <= 'Z');
     } else {
-        ok = ('a' <= *ptr && *ptr <= 'z') || ('A' <= *ptr && *ptr <= 'Z');
+      ok = ('a' <= *ptr && *ptr <= 'z') || ('A' <= *ptr && *ptr <= 'Z') ||
-      }
+           (*ptr == '_') || (*ptr == '-') || ('0' <= *ptr && *ptr <= '9');
-    } else {
-      ok = (*ptr == '_') || (*ptr == '-') || ('0' <= *ptr && *ptr <= '9') ||
-           ('a' <= *ptr && *ptr <= 'z') || ('A' <= *ptr && *ptr <= 'Z');
     }
 
     if (!ok) {
@@ -1759,12 +1755,7 @@ bool TRI_vocbase_t::IsAllowedName(bool allowSystem,
     }
   }
 
-  // invalid name length
+  return (length > 0 && length <= TRI_COL_NAME_LENGTH);
-  if (length == 0 || length > TRI_COL_NAME_LENGTH) {
-    return false;
-  }
-
-  return true;
 }
 
 /// @brief determine whether a collection name is a system collection name
@@ -1914,7 +1905,7 @@ TRI_voc_rid_t TRI_ExtractRevisionId(VPackSlice slice) {
   VPackSlice r(slice.get(StaticStrings::RevString));
   if (r.isString()) {
     VPackValueLength l;
-    char const* p = r.getString(l);
+    char const* p = r.getStringUnchecked(l);
     return TRI_StringToRid(p, l, false);
   }
   if (r.isInteger()) {
@@ -1940,7 +1931,8 @@ void TRI_SanitizeObject(VPackSlice const slice, VPackBuilder& builder) {
   VPackObjectIterator it(slice);
   while (it.valid()) {
     arangodb::velocypack::StringRef key(it.key());
-    if (key.empty() || key[0] != '_' ||
+    // _id, _key, _rev. minimum size here is 3
+    if (key.size() < 3 || key[0] != '_' ||
         (key != StaticStrings::KeyString && key != StaticStrings::IdString &&
          key != StaticStrings::RevString)) {
       builder.add(key.data(), key.size(), it.value());
@@ -1956,7 +1948,8 @@ void TRI_SanitizeObjectWithEdges(VPackSlice const slice, VPackBuilder& builder)
   VPackObjectIterator it(slice, true);
   while (it.valid()) {
     arangodb::velocypack::StringRef key(it.key());
-    if (key.empty() || key[0] != '_' ||
+    // _id, _key, _rev, _from, _to. minimum size here is 3
+    if (key.size() < 3 || key[0] != '_' ||
         (key != StaticStrings::KeyString && key != StaticStrings::IdString &&
          key != StaticStrings::RevString && key != StaticStrings::FromString &&
          key != StaticStrings::ToString)) {

lib/Basics/StringUtils.cpp

@@ -263,8 +263,8 @@ std::string escapeUnicode(std::string const& name, bool escapeSlash) {
 
   bool corrupted = false;
 
-  char* buffer = new char[6 * len + 1];
+  auto buffer = std::make_unique<char[]>(6 * len + 1);
-  char* qtr = buffer;
+  char* qtr = buffer.get();
   char const* ptr = name.c_str();
   char const* end = ptr + len;
 
@@ -430,12 +430,10 @@ std::string escapeUnicode(std::string const& name, bool escapeSlash) {
 
   *qtr = '\0';
 
-  std::string result(buffer, qtr - buffer);
+  std::string result(buffer.get(), qtr - buffer.get());
-
-  delete[] buffer;
 
   if (corrupted) {
-    LOG_TOPIC("4c231", WARN, arangodb::Logger::FIXME)
+    LOG_TOPIC("4c231", DEBUG, arangodb::Logger::FIXME)
         << "escaped corrupted unicode string";
   }
 
@@ -449,8 +447,8 @@ std::vector<std::string> split(std::string const& source, char delim, char quote
     return result;
   }
 
-  char* buffer = new char[source.size() + 1];
+  auto buffer = std::make_unique<char[]>(source.size() + 1);
-  char* p = buffer;
+  char* p = buffer.get();
 
   char const* q = source.c_str();
   char const* e = source.c_str() + source.size();
@@ -458,9 +456,8 @@ std::vector<std::string> split(std::string const& source, char delim, char quote
   if (quote == '\0') {
     for (; q < e; ++q) {
       if (*q == delim) {
-        *p = '\0';
+        result.emplace_back(buffer.get(), p - buffer.get());
-        result.push_back(std::string(buffer, p - buffer));
+        p = buffer.get();
-        p = buffer;
       } else {
         *p++ = *q;
       }
@@ -472,20 +469,15 @@ std::vector<std::string> split(std::string const& source, char delim, char quote
           *p++ = *++q;
         }
       } else if (*q == delim) {
-        *p = '\0';
+        result.emplace_back(buffer.get(), p - buffer.get());
-        result.push_back(std::string(buffer, p - buffer));
+        p = buffer.get();
-        p = buffer;
       } else {
         *p++ = *q;
       }
     }
   }
 
-  *p = '\0';
+  result.emplace_back(buffer.get(), p - buffer.get());
-  result.push_back(std::string(buffer, p - buffer));
-
-  delete[] buffer;
-
   return result;
 }
 
@@ -497,8 +489,8 @@ std::vector<std::string> split(std::string const& source,
     return result;
   }
 
-  char* buffer = new char[source.size() + 1];
+  auto buffer = std::make_unique<char[]>(source.size() + 1);
-  char* p = buffer;
+  char* p = buffer.get();
 
   char const* q = source.c_str();
   char const* e = source.c_str() + source.size();
@@ -506,9 +498,8 @@ std::vector<std::string> split(std::string const& source,
   if (quote == '\0') {
     for (; q < e; ++q) {
       if (delim.find(*q) != std::string::npos) {
-        *p = '\0';
+        result.emplace_back(buffer.get(), p - buffer.get());
-        result.push_back(std::string(buffer, p - buffer));
+        p = buffer.get();
-        p = buffer;
       } else {
         *p++ = *q;
       }
@@ -520,20 +511,15 @@ std::vector<std::string> split(std::string const& source,
           *p++ = *++q;
         }
       } else if (delim.find(*q) != std::string::npos) {
-        *p = '\0';
+        result.emplace_back(buffer.get(), p - buffer.get());
-        result.push_back(std::string(buffer, p - buffer));
+        p = buffer.get();
-        p = buffer;
       } else {
         *p++ = *q;
       }
     }
   }
 
-  *p = '\0';
+  result.emplace_back(buffer.get(), p - buffer.get());
-  result.push_back(std::string(buffer, p - buffer));
-
-  delete[] buffer;
-
   return result;
 }
 
@@ -664,7 +650,8 @@ std::string replace(std::string const& sourceStr, std::string const& fromStr,
   // is length of sourceStr
   maxLength = (std::max)(maxLength, sourceLength) + 1;
 
-  char* result = new char[maxLength];
+  auto result = std::make_unique<char[]>(maxLength);
+  char* ptr = result.get();
   size_t k = 0;
 
   for (size_t j = 0; j < sourceLength; ++j) {
@@ -678,26 +665,20 @@ std::string replace(std::string const& sourceStr, std::string const& fromStr,
     }
 
     if (!match) {
-      result[k] = sourceStr[j];
+      ptr[k] = sourceStr[j];
       ++k;
       continue;
     }
 
     for (size_t i = 0; i < toLength; ++i) {
-      result[k] = toStr[i];
+      ptr[k] = toStr[i];
       ++k;
     }
 
     j += (fromLength - 1);
   }
 
-  result[k] = '\0';
+  return std::string(ptr, k);
-
-  std::string retStr(result);
-
-  delete[] result;
-
-  return retStr;
 }
 
 void tolowerInPlace(std::string* str) {

lib/Basics/files.cpp

@@ -317,7 +317,6 @@ static bool IsSymbolicLink(char const* path, struct stat* stbuf) {
 
   return (res == 0) && ((stbuf->st_mode & S_IFMT) == S_IFLNK);
 }
-static void deleteBuf(char* p) { std::free(p); };
 }  // namespace
 
 std::string TRI_ResolveSymbolicLink(std::string path, bool& hadError, bool recursive) {
@@ -1343,20 +1342,12 @@ int TRI_DestroyLockFile(char const* filename) {
 /// @brief return the filename component of a file (without path)
 ////////////////////////////////////////////////////////////////////////////////
 
-char* TRI_GetFilename(char const* filename) {
+std::string TRI_GetFilename(std::string const& filename) {
-  char const* p;
+  size_t pos = filename.find_last_of("\\/:");
-  char const* s;
+  if (pos == std::string::npos) {
-
+    return filename;
-  p = s = filename;
-
-  while (*p != '\0') {
-    if (*p == '\\' || *p == '/' || *p == ':') {
-      s = p + 1;
   }
-    p++;
+  return filename.substr(pos + 1);
-  }
-
-  return TRI_DuplicateString(s);
 }
 
 ////////////////////////////////////////////////////////////////////////////////

lib/Basics/files.h

@@ -233,7 +233,7 @@ int TRI_DestroyLockFile(char const* filename);
 /// @brief return the filename component of a file (without path)
 ////////////////////////////////////////////////////////////////////////////////
 
-char* TRI_GetFilename(char const*);
+std::string TRI_GetFilename(std::string const&);
 
 ////////////////////////////////////////////////////////////////////////////////
 /// @brief return the absolute path of a file

lib/Basics/tri-zip.cpp

@@ -164,12 +164,14 @@ static int ExtractCurrentFile(unzFile uf, void* buffer, size_t const bufferSize,
       std::string tmp = basics::FileUtils::buildFilename(outPath, filenameInZip);
       int res = TRI_CreateRecursiveDirectory(tmp.c_str(), systemError, errorMessage);
       
+      // write back the original value
+      // cppcheck-suppress *
+      *(filenameWithoutPath - 1) = c;
+
       if (res != TRI_ERROR_NO_ERROR) {
         return res;
       }
 
-      *(filenameWithoutPath - 1) = c;
-
       // try again
       fout = TRI_FOPEN(fullPath.c_str(), "wb");
     } else if (fout == nullptr) {

lib/Endpoint/EndpointIp.h

@@ -45,9 +45,9 @@ class EndpointIp : public Endpoint {
   TRI_socket_t connectSocket(addrinfo const*, double, double);
 
  public:
-  TRI_socket_t connect(double, double) override;
+  TRI_socket_t connect(double, double) override final;
-  virtual void disconnect() override;
+  void disconnect() override final;
-  virtual bool initIncoming(TRI_socket_t) override;
+  bool initIncoming(TRI_socket_t) override final;
 
   int port() const override { return _port; }
   std::string host() const override { return _host; }

lib/Maskings/Maskings.h

@@ -76,7 +76,7 @@ class Maskings {
   void addMaskedObject(Collection& collection, VPackBuilder& builder,
                        std::vector<std::string>& path, VPackSlice const& data);
   void addMasked(Collection& collection, VPackBuilder& builder, VPackSlice const& data);
-  void addMasked(Collection& collection, basics::StringBuffer&, VPackSlice const& data);
+  void addMasked(Collection& collection, basics::StringBuffer& data, VPackSlice const& slice);
 
  private:
   std::map<std::string, Collection> _collections;

tests/Basics/files-test.cpp

@@ -392,6 +392,22 @@ SECTION("tst_normalize") {
 #endif
 }
 
+SECTION("tst_getfilename") {
+  CHECK("" == TRI_GetFilename(""));
+  CHECK("." == TRI_GetFilename("."));
+  CHECK("" == TRI_GetFilename("/"));
+  CHECK("haxxmann" == TRI_GetFilename("haxxmann"));
+  CHECK("haxxmann" == TRI_GetFilename("/haxxmann"));
+  CHECK("haxxmann" == TRI_GetFilename("/tmp/haxxmann"));
+  CHECK("haxxmann" == TRI_GetFilename("/a/b/c/haxxmann"));
+  CHECK("haxxmann" == TRI_GetFilename("c:/haxxmann"));
+  CHECK("haxxmann" == TRI_GetFilename("c:/tmp/haxxmann"));
+  CHECK("foo" == TRI_GetFilename("c:/tmp/haxxmann/foo"));
+  CHECK("haxxmann" == TRI_GetFilename("\\haxxmann"));
+  CHECK("haxxmann" == TRI_GetFilename("\\a\\haxxmann"));
+  CHECK("haxxmann" == TRI_GetFilename("\\a\\b\\haxxmann"));
+}
+
 }
 
 ////////////////////////////////////////////////////////////////////////////////