App: Coverity fixes

Author: chennes

src/App/ComplexGeoData.cpp

@@ -586,6 +586,11 @@ void ComplexGeoData::restoreStream(std::istream& stream, std::size_t count)
                 // NOLINTNEXTLINE
                 FC_THROWM(Base::RuntimeError, "Failed to restore element map " << _persistenceName);
             }
+            constexpr std::size_t oneGbOfInts {(1 << 30) / sizeof(int)};
+            if (sCount > oneGbOfInts) {
+                // NOLINTNEXTLINE
+                FC_THROWM(Base::RuntimeError, "Failed to restore element map (>1GB) " << _persistenceName);
+            }
             sids.reserve(static_cast<int>(sCount));
             for (std::size_t j = 0; j < sCount; ++j) {
                 long id = 0;

src/App/ComplexGeoData.h

@@ -369,7 +369,7 @@ class AppExport ComplexGeoData: public Base::Persistence, public Base::Handled
      */
     void traceElement(const MappedName& name, TraceCallback cb) const
     {
-        _elementMap->traceElement(name, Tag, cb);
+        _elementMap->traceElement(name, Tag, std::move(cb));
     }
 
     /** Flush an internal buffering for element mapping */

src/App/Document.cpp

@@ -3807,6 +3807,9 @@ void Document::_removeObject(DocumentObject* pcObject)
     _checkTransaction(pcObject, nullptr, __LINE__);
 
     auto pos = d->objectMap.find(pcObject->getNameInDocument());
+    if (pos == d->objectMap.end()) {
+        FC_ERR("Internal error, could not find " << pcObject->getFullName() << " to remove");
+    }
 
     if (!d->rollback && d->activeUndoTransaction && pos->second->hasChildElement()) {
         // Preserve link group children global visibility. See comments in
@@ -4272,15 +4275,15 @@ std::vector<App::DocumentObject*> Document::getRootObjectsIgnoreLinks() const
 {
     std::vector<App::DocumentObject*> ret;
 
-    for (auto objectIt : d->objectArray) {
+    for (const auto &objectIt : d->objectArray) {
         auto list = objectIt->getInList();
         bool noParents = list.empty();
 
         if (!noParents) {
             // App::Document getRootObjects returns the root objects of the dependency graph.
-            // So if an object is referenced by a App::Link, it will not be returned by that
+            // So if an object is referenced by an App::Link, it will not be returned by that
             // function. So here, as we want the tree-root level objects, we check if all the
-            // parents are links. In which case its still a root object.
+            // parents are links. In which case it's still a root object.
             noParents = std::all_of(list.cbegin(), list.cend(), [](App::DocumentObject* obj) {
                 return obj->isDerivedFrom<App::Link>();
             });
@@ -4300,21 +4303,19 @@ void DocumentP::findAllPathsAt(const std::vector<Node>& all_nodes,
                                Path tmp)
 {
     if (std::find(tmp.begin(), tmp.end(), id) != tmp.end()) {
-        Path tmp2(tmp);
-        tmp2.push_back(id);
-        all_paths.push_back(tmp2);
+        tmp.push_back(id);
+        all_paths.push_back(std::move(tmp));
         return;  // a cycle
     }
 
     tmp.push_back(id);
     if (all_nodes[id].empty()) {
-        all_paths.push_back(tmp);
+        all_paths.push_back(std::move(tmp));
         return;
     }
 
     for (size_t i = 0; i < all_nodes[id].size(); i++) {
-        Path tmp2(tmp);
-        findAllPathsAt(all_nodes, all_nodes[id][i], all_paths, tmp2);
+        findAllPathsAt(all_nodes, all_nodes[id][i], all_paths, tmp);
     }
 }
 
@@ -4344,18 +4345,18 @@ Document::getPathsByOutList(const App::DocumentObject* from, const App::Document
     size_t index_to = indexMap[to];
     Path tmp;
     std::vector<Path> all_paths;
-    DocumentP::findAllPathsAt(all_nodes, index_from, all_paths, tmp);
+    DocumentP::findAllPathsAt(all_nodes, index_from, all_paths, std::move(tmp));
 
     for (const Path& it : all_paths) {
         Path::const_iterator jt = std::find(it.begin(), it.end(), index_to);
         if (jt != it.end()) {
-            std::list<App::DocumentObject*> path;
+            array.push_back({});
+            auto& path = array.back();
             for (Path::const_iterator kt = it.begin(); kt != jt; ++kt) {
                 path.push_back(d->objectArray[*kt]);
             }
 
             path.push_back(d->objectArray[*jt]);
-            array.push_back(path);
         }
     }
 

src/App/DocumentObject.cpp

@@ -1162,7 +1162,7 @@ void DocumentObject::Save(Base::Writer& writer) const
 
 void DocumentObject::setExpression(const ObjectIdentifier& path, std::shared_ptr<Expression> expr)
 {
-    ExpressionEngine.setValue(path, expr);
+    ExpressionEngine.setValue(path, std::move(expr));
 }
 
 /**

src/App/DocumentObjectPyImp.cpp

@@ -383,7 +383,7 @@ PyObject* DocumentObjectPy::setExpression(PyObject* args)
             shared_expr->comment = comment;
         }
 
-        getDocumentObjectPtr()->setExpression(p, shared_expr);
+        getDocumentObjectPtr()->setExpression(p, std::move(shared_expr));
         Py_Return;
     }
 

src/App/ElementMap.cpp

@@ -258,7 +258,8 @@ ElementMapPtr ElementMap::restore(::App::StringHasherRef hasherRef, std::istream
 
     std::vector<ElementMapPtr> childMaps;
     count = 0;
-    if (!(stream >> tmp >> count) || tmp != "MapCount" || count == 0) {
+    constexpr int practicalMaximum {1 << 30 / sizeof(ElementMapPtr)};  // a 1GB child map vector: almost certainly a bug
+    if (!(stream >> tmp >> count) || tmp != "MapCount" || count == 0 || count > practicalMaximum) {
         FC_THROWM(Base::RuntimeError, msg);  // NOLINT
     }
     childMaps.reserve(count - 1);
@@ -285,6 +286,10 @@ ElementMapPtr ElementMap::restore(::App::StringHasherRef hasherRef,
     if (!(stream >> tmp >> index >> id >> typeCount) || tmp != "ElementMap") {
         FC_THROWM(Base::RuntimeError, msg);  // NOLINT
     }
+    constexpr int maxTypeCount(1000);
+    if (typeCount < 0 || typeCount > maxTypeCount) {
+        FC_THROWM(Base::RuntimeError, "Bad type count in element map, ignoring map");  // NOLINT
+    }
 
     auto& map = _idToElementMap[id];
     if (map) {

src/App/Expression.cpp

@@ -1728,7 +1728,7 @@ FunctionExpression::FunctionExpression(const DocumentObject *_owner, Function _f
     : UnitExpression(_owner)
     , f(_f)
     , fname(std::move(name))
-    , args(_args)
+    , args(std::move(_args))
 {
     switch (f) {
     case ABS:
@@ -2643,7 +2643,7 @@ Expression *FunctionExpression::simplify() const
         return eval();
     }
     else
-        return new FunctionExpression(owner, f, std::string(fname), a);
+        return new FunctionExpression(owner, f, std::string(fname), std::move(a));
 }
 
 /**
@@ -2811,7 +2811,7 @@ Expression *FunctionExpression::_copy() const
         a.push_back((*i)->copy());
         ++i;
     }
-    return new FunctionExpression(owner, f, std::string(fname), a);
+    return new FunctionExpression(owner, f, std::string(fname), std::move(a));
 }
 
 void FunctionExpression::_visit(ExpressionVisitor &v)

src/App/ExpressionTokenizer.cpp

@@ -75,14 +75,17 @@ QString ExpressionTokenizer::perform(const QString& prefix, int pos)
     // Pop those trailing tokens depending on the given position, which may be
     // in the middle of a token, and we shall include that token.
     for (auto it = tokens.begin(); it != tokens.end(); ++it) {
-        if (std::get<1>(*it) >= pos) {
+        int tokenType = std::get<0>(*it);
+        int location = std::get<1>(*it);
+        int tokenLength = static_cast<int> (std::get<2>(*it).size());
+        if (location >= pos) {
             // Include the immediately followed '.' or '#', because we'll be
             // inserting these separators too, in ExpressionCompleteModel::pathFromIndex()
-            if (it != tokens.begin() && std::get<0>(*it) != '.' && std::get<0>(*it) != '#') {
+            if (it != tokens.begin() && tokenType != '.' && tokenType != '#') {
                 it = it - 1;
             }
-            tokens.resize(it - tokens.begin() + 1);
-            prefixEnd = start + std::get<1>(*it) + (int)std::get<2>(*it).size();
+            tokens.resize(it - tokens.begin() + 1); // Invalidates it, but we already calculated tokenLength
+            prefixEnd = start + location + tokenLength;
             break;
         }
     }

src/App/GeoFeature.cpp

@@ -223,14 +223,6 @@ bool GeoFeature::getCameraAlignmentDirection(Base::Vector3d& direction, const ch
 bool GeoFeature::hasMissingElement(const char* subname)
 {
     return Data::hasMissingElement(subname);
-    if (!subname) {
-        return false;
-    }
-    auto dot = strrchr(subname, '.');
-    if (!dot) {
-        return subname[0] == '?';
-    }
-    return dot[1] == '?';
 }
 
 void GeoFeature::updateElementReference()

src/App/GeoFeatureGroupExtension.cpp

@@ -421,10 +421,8 @@ bool GeoFeatureGroupExtension::extensionGetSubObject(DocumentObject*& ret,
             }
         }
         if (ret) {
-            if (dot) {
-                ++dot;
-            }
-            if (dot && *dot && !ret->hasExtension(App::LinkBaseExtension::getExtensionClassTypeId())
+            ++dot;
+            if (*dot && !ret->hasExtension(App::LinkBaseExtension::getExtensionClassTypeId())
                 && !ret->hasExtension(App::GeoFeatureGroupExtension::getExtensionClassTypeId())) {
                 // Consider this
                 // Body
@@ -451,7 +449,7 @@ bool GeoFeatureGroupExtension::extensionGetSubObject(DocumentObject*& ret,
                 *mat *=
                     const_cast<GeoFeatureGroupExtension*>(this)->placement().getValue().toMatrix();
             }
-            ret = ret->getSubObject(dot ? dot : "", pyObj, mat, true, depth + 1);
+            ret = ret->getSubObject(dot, pyObj, mat, true, depth + 1);
         }
     }
     return true;