Merge pull request #3222 from eduar-hte/remove-copies

Remove several string copies and unnecessary heap allocations

src/Makefile.am

@@ -253,7 +253,6 @@ UTILS = \
 	utils/random.cc \
 	utils/regex.cc \
 	utils/sha1.cc \
-	utils/string.cc \
 	utils/system.cc \
 	utils/shared_files.cc
 

src/request_body_processor/multipart.cc

@@ -70,22 +70,20 @@ void MultipartPartTmpFile::Open() {
 
     localtime_r(&tt, &timeinfo);
 
-    char tstr[300] {};
+    char tstr[17];
-    strftime(tstr, 299, "/%Y%m%d-%H%M%S", &timeinfo);
+    strftime(tstr, std::size(tstr), "/%Y%m%d-%H%M%S", &timeinfo);
 
     std::string path = m_transaction->m_rules->m_uploadDirectory.m_value;
     path = path + tstr + "-" + *m_transaction->m_id.get();
     path += "-file-XXXXXX";
 
-    char* tmp = strdup(path.c_str());
 #ifndef WIN32
-    m_tmp_file_fd = mkstemp(tmp);
+    m_tmp_file_fd = mkstemp(path.data());
 #else
-    _mktemp_s(tmp, path.length()+1);
+    _mktemp_s(path.data(), path.length()+1);
-    m_tmp_file_fd = _open(tmp, _O_CREAT | _O_EXCL | _O_RDWR);
+    m_tmp_file_fd = _open(path.c_str(), _O_CREAT | _O_EXCL | _O_RDWR);
 #endif
-    m_tmp_file_name.assign(tmp);
+    m_tmp_file_name = path;
-    free(tmp);
     ms_dbg_a(m_transaction, 4, "MultipartPartTmpFile: Create filename= " + m_tmp_file_name);
 
     int mode = m_transaction->m_rules->m_uploadFileMode.m_value;
@@ -857,7 +855,7 @@ int Multipart::process_part_header(std::string *error, int offset) {
             }
 
             new_value = std::string(data);
-            utils::string::chomp(&new_value);
+            utils::string::chomp(new_value);
 
             /* update the header value in the table */
             header_value = m_mpp->m_headers.at(
@@ -926,7 +924,7 @@ int Multipart::process_part_header(std::string *error, int offset) {
                 i++;
             }
             header_value = std::string(data);
-            utils::string::chomp(&header_value);
+            utils::string::chomp(header_value);
 
             /* error if the name already exists */
             if (m_mpp->m_headers.count(header_name) > 0) {
@@ -1271,22 +1269,10 @@ int Multipart::multipart_complete(std::string *error) {
 
 
 int Multipart::count_boundary_params(const std::string& str_header_value) {
-    std::string lower = utils::string::tolower(str_header_value);
-    const char *header_value = lower.c_str();
-    char *duplicate = NULL;
-    char *s = NULL;
     int count = 0;
 
-    if (header_value == NULL) {
+    const auto lower = utils::string::tolower(str_header_value);
-        return -1;
+    const char *s = lower.c_str();
-    }
-
-    duplicate = strdup(header_value);
-    if (duplicate == NULL) {
-        return -1;
-    }
-
-    s = duplicate;
     while ((s = strstr(s, "boundary")) != NULL) {
         s += 8;
 
@@ -1295,7 +1281,6 @@ int Multipart::count_boundary_params(const std::string& str_header_value) {
         }
     }
 
-    free(duplicate);
     return count;
 }
 

src/transaction.cc

@@ -1520,7 +1520,7 @@ std::string Transaction::toOldAuditLogFormatIndex(const std::string &filename,
     ss << utils::string::dash_if_empty(
        m_variableRequestHeaders.resolveFirst("Host").get())
         << " ";
-    ss << utils::string::dash_if_empty(this->m_clientIpAddress->c_str()) << " ";
+    ss << utils::string::dash_if_empty(this->m_clientIpAddress.get()) << " ";
     /** TODO: Check variable */
     variables::RemoteUser *r = new variables::RemoteUser("REMOTE_USER");
     std::vector<const VariableValue *> l;
@@ -1531,7 +1531,7 @@ std::string Transaction::toOldAuditLogFormatIndex(const std::string &filename,
     delete r;
 
     ss << utils::string::dash_if_empty(
-        m_variableRemoteUser.c_str());
+        &m_variableRemoteUser);
     ss << " ";
     /** TODO: Check variable */
     //ss << utils::string::dash_if_empty(

src/utils/string.cc

@@ -1,268 +0,0 @@
-/*
- * ModSecurity, http://www.modsecurity.org/
- * Copyright (c) 2015 - 2023 Trustwave Holdings, Inc. (http://www.trustwave.com/)
- *
- * You may not use this file except in compliance with
- * the License.  You may obtain a copy of the License at
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * If any of the files related to licensing are missing or if you have any
- * other questions related to licensing please contact Trustwave Holdings, Inc.
- * directly using the email address security@modsecurity.org.
- *
- */
-
-#include <stdio.h>
-#include <stdlib.h>
-#include <stddef.h>
-#include <string.h>
-#include <stdint.h>
-#include <inttypes.h>
-
-#include <algorithm>
-#include <random>
-#include <memory>
-#include <functional>
-#include <string>
-#include <iostream>
-#include <sstream>
-#include <cstring>
-
-#if defined _MSC_VER
-#include <direct.h>
-#elif defined __GNUC__
-#include <sys/types.h>
-#include <sys/stat.h>
-#endif
-
-#include "modsecurity/modsecurity.h"
-
-#include "src/utils/string.h"
-
-
-namespace modsecurity {
-namespace utils {
-namespace string {
-
-
-std::string parserSanitizer(std::string a) {
-    a = removeWhiteSpacesIfNeeded(a);
-    a = removeBracketsIfNeeded(a);
-    return a;
-}
-
-
-std::string removeWhiteSpacesIfNeeded(std::string a) {
-    while (a.size() > 1 && a.at(0) == ' ') {
-        a.erase(0, 1);
-    }
-    while (a.size() > 1 && a.at(a.length()-1) == ' ') {
-        a.pop_back();
-    }
-    return a;
-}
-
-
-std::string ascTime(time_t *t) {
-    std::string ts = std::ctime(t);
-    ts.pop_back();
-    return ts;
-}
-
-
-std::string dash_if_empty(const std::string *str) {
-    if (str == NULL || str->empty()) {
-        return "-";
-    }
-
-    return *str;
-}
-
-
-std::string dash_if_empty(const char *str) {
-    if (str == NULL || strlen(str) == 0) {
-        return "-";
-    }
-
-    return std::string(str);
-}
-
-
-std::string limitTo(int amount, const std::string &str) {
-    std::string ret;
-
-    if (str.length() > amount) {
-        ret.assign(str, 0, amount);
-        ret = ret + " (" + std::to_string(str.length() - amount) + " " \
-            "characters omitted)";
-        return ret;
-    }
-
-    return str;
-}
-
-
-std::string removeBracketsIfNeeded(std::string a) {
-    if (a.length() > 1 && a.at(0) == '"' && a.at(a.length()-1) == '"') {
-        a.pop_back();
-        a.erase(0, 1);
-    }
-    if (a.length() > 1 && a.at(0) == '\'' && a.at(a.length()-1) == '\'') {
-        a.pop_back();
-        a.erase(0, 1);
-    }
-    return a;
-}
-
-
-std::string string_to_hex(const std::string& input) {
-    static const char* const lut = "0123456789ABCDEF";
-    size_t len = input.length();
-
-    std::string output;
-    output.reserve(2 * len);
-    for (size_t i = 0; i < len; ++i) {
-        const unsigned char c = input[i];
-        output.push_back(lut[c >> 4]);
-        output.push_back(lut[c & 15]);
-    }
-    return output;
-}
-
-std::string toHexIfNeeded(const std::string &str, bool escape_spec) {
-    // escape_spec: escape special chars or not
-    // spec chars: '"' (quotation mark, ascii 34), '\' (backslash, ascii 92)
-    std::stringstream res;
-
-    for (int i = 0; i < str.size(); i++) {
-        int c = (unsigned char)str.at(i);
-        if (c < 32 || c > 126 || (escape_spec == true && (c == 34 || c == 92))) {
-            res << "\\x" << std::setw(2) << std::setfill('0') << std::hex << c;
-        } else {
-            res << str.at(i);
-        }
-    }
-
-    return res.str();
-}
-
-
-std::string tolower(std::string str) {
-    std::string value;
-    value.resize(str.length());
-
-    std::transform(str.begin(),
-            str.end(),
-            value.begin(),
-            ::tolower);
-
-    return value;
-}
-
-
-std::string toupper(std::string str) {
-    std::string value;
-    value.resize(str.length());
-
-    std::transform(str.begin(),
-            str.end(),
-            value.begin(),
-            ::toupper);
-
-    return value;
-}
-
-
-std::vector<std::string> ssplit(std::string str, char delimiter) {
-    std::vector<std::string> internal;
-    std::stringstream ss(str);  // Turn the string into a stream.
-    std::string tok;
-
-    while (getline(ss, tok, delimiter)) {
-        internal.push_back(tok);
-    }
-
-    return internal;
-}
-
-
-std::pair<std::string, std::string> ssplit_pair(const std::string& str, char delimiter) {
-    std::stringstream ss(str);  // Turn the string into a stream.
-    std::string key;
-    std::string value;
-
-    getline(ss, key, delimiter);
-    if (key.length() < str.length()) {
-        value = str.substr(key.length()+1);
-    }
-
-    return std::make_pair(key, value);
-}
-
-
-std::vector<std::string> split(std::string str, char delimiter) {
-    std::vector<std::string> internal = ssplit(str, delimiter);
-
-    if (internal.size() == 0) {
-        internal.push_back(str);
-    }
-
-    return internal;
-}
-
-
-void chomp(std::string *str) {
-    std::string::size_type pos = str->find_last_not_of("\n\r");
-    if (pos != std::string::npos) {
-        str->erase(pos+1, str->length()-pos-1);
-    }
-}
-
-
-unsigned char x2c(const unsigned char *what) {
-    unsigned char digit;
-
-    digit = (what[0] >= 'A' ? ((what[0] & 0xdf) - 'A') + 10 : (what[0] - '0'));
-    digit *= 16;
-    digit += (what[1] >= 'A' ? ((what[1] & 0xdf) - 'A') + 10 : (what[1] - '0'));
-
-    return digit;
-}
-
-
-/**
- * Converts a single hexadecimal digit into a decimal value.
- */
-unsigned char xsingle2c(const unsigned char *what) {
-    unsigned char digit;
-
-    digit = (what[0] >= 'A' ? ((what[0] & 0xdf) - 'A') + 10 : (what[0] - '0'));
-
-    return digit;
-}
-
-
-unsigned char *c2x(unsigned what, unsigned char *where) {
-    static const char c2x_table[] = "0123456789abcdef";
-
-    what = what & 0xff;
-    *where++ = c2x_table[what >> 4];
-    *where++ = c2x_table[what & 0x0f];
-
-    return where;
-}
-
-
-void replaceAll(std::string &str, std::string_view from,
-    std::string_view to) {
-    size_t start_pos = 0;
-    while ((start_pos = str.find(from, start_pos)) != std::string::npos) {
-        str.replace(start_pos, from.length(), to);
-        start_pos += to.length();
-    }
-}
-
-}  // namespace string
-}  // namespace utils
-}  // namespace modsecurity

src/utils/string.h

@@ -14,9 +14,13 @@
  */
 
 #include <ctime>
-#include <iostream>
 #include <string>
+#include <cstring>
 #include <vector>
+#include <algorithm>
+#include <utility>
+#include <sstream>
+#include <iomanip>
 
 #ifndef SRC_UTILS_STRING_H_
 #define SRC_UTILS_STRING_H_
@@ -55,27 +59,210 @@ const char HEX2DEC[256] = {
 };
 
 
-std::string ascTime(time_t *t);
+inline std::string ascTime(const time_t *t) {
-std::string dash_if_empty(const char *str);
+    std::string ts = std::ctime(t);
-std::string dash_if_empty(const std::string *str);
+    ts.pop_back();
-std::string limitTo(int amount, const std::string &str);
+    return ts;
-std::string removeBracketsIfNeeded(std::string a);
+}
-std::string string_to_hex(const std::string& input);
+
-std::string toHexIfNeeded(const std::string &str, bool escape_spec = false);
+
-std::string tolower(std::string str);
+inline std::string dash_if_empty(const std::string *str) {
-std::string toupper(std::string str);
+    if (str == nullptr || str->empty()) {
-std::vector<std::string> ssplit(std::string str, char delimiter);
+        return "-";
-std::pair<std::string, std::string> ssplit_pair(const std::string& str, char delimiter);
+    }
-std::vector<std::string> split(std::string str, char delimiter);
+
-void chomp(std::string *str);
+    return *str;
-void replaceAll(std::string &str, std::string_view from,
+}
-    std::string_view to);
+
-std::string removeWhiteSpacesIfNeeded(std::string a);
+
-std::string parserSanitizer(std::string a);
+inline std::string limitTo(int amount, const std::string &str) {
+    std::string ret;
+
+    if (str.length() > amount) {
+        ret.assign(str, 0, amount);
+        ret = ret + " (" + std::to_string(str.length() - amount) + " " \
+            "characters omitted)";
+        return ret;
+    }
+
+    return str;
+}
+
+
+inline std::string toHexIfNeeded(const std::string &str, bool escape_spec = false) {
+    // escape_spec: escape special chars or not
+    // spec chars: '"' (quotation mark, ascii 34), '\' (backslash, ascii 92)
+    std::stringstream res;
+
+    for (int i = 0; i < str.size(); i++) {
+        int c = (unsigned char)str.at(i);
+        if (c < 32 || c > 126 || (escape_spec == true && (c == 34 || c == 92))) {
+            res << "\\x" << std::setw(2) << std::setfill('0') << std::hex << c;
+        } else {
+            res << str.at(i);
+        }
+    }
+
+    return res.str();
+}
+
+
+inline std::vector<std::string> ssplit(const std::string &str, char delimiter) {
+    std::vector<std::string> internal;
+    std::stringstream ss(str);  // Turn the string into a stream.
+    std::string tok;
+
+    while (getline(ss, tok, delimiter)) {
+        internal.push_back(tok);
+    }
+
+    return internal;
+}
+
+
+inline std::pair<std::string, std::string> ssplit_pair(const std::string& str, char delimiter) {
+    std::stringstream ss(str);  // Turn the string into a stream.
+    std::string key;
+    std::string value;
+
+    getline(ss, key, delimiter);
+    if (key.length() < str.length()) {
+        value = str.substr(key.length()+1);
+    }
+
+    return std::make_pair(key, value);
+}
+
+
+inline std::vector<std::string> split(const std::string &str, char delimiter) {
+    std::vector<std::string> internal = ssplit(str, delimiter);
+
+    if (internal.empty()) {
+        internal.push_back(str);
+    }
+
+    return internal;
+}
+
+
+inline void chomp(std::string &str) {
+    std::string::size_type pos = str.find_last_not_of("\n\r");
+    if (pos != std::string::npos) {
+        str.erase(pos+1, str.length()-pos-1);
+    }
+}
+
+
+inline void replaceAll(std::string &str, std::string_view from,
+    std::string_view to) {
+    size_t start_pos = 0;
+    while ((start_pos = str.find(from, start_pos)) != std::string::npos) {
+        str.replace(start_pos, from.length(), to);
+        start_pos += to.length();
+    }
+}
+
+
+inline std::string removeWhiteSpacesIfNeeded(std::string a) {
+    while (a.size() > 1 && a.at(0) == ' ') {
+        a.erase(0, 1);
+    }
+    while (a.size() > 1 && a.at(a.length()-1) == ' ') {
+        a.pop_back();
+    }
+    return a;
+}
+
+
+inline std::string removeBracketsIfNeeded(std::string a) {
+    if (a.length() > 1 && a.at(0) == '"' && a.at(a.length()-1) == '"') {
+        a.pop_back();
+        a.erase(0, 1);
+    }
+    if (a.length() > 1 && a.at(0) == '\'' && a.at(a.length()-1) == '\'') {
+        a.pop_back();
+        a.erase(0, 1);
+    }
+    return a;
+}
+
+
+inline std::string parserSanitizer(std::string a) {
+    a = removeWhiteSpacesIfNeeded(a);
+    a = removeBracketsIfNeeded(a);
+    return a;
+}
+
+
+/**
+ * Converts a single hexadecimal digit into a decimal value.
+ */
+inline unsigned char xsingle2c(const unsigned char *what) {
+    unsigned char digit;
+
+    digit = (what[0] >= 'A' ? ((what[0] & 0xdf) - 'A') + 10 : (what[0] - '0'));
+
+    return digit;
+}
+
+
+inline unsigned char x2c(const unsigned char *what) {
+    unsigned char digit;
+
+    digit = xsingle2c(what);
+    digit *= 16;
+    digit += xsingle2c(what+1);
+
+    return digit;
+}
+
+
+inline unsigned char *c2x(unsigned what, unsigned char *where) {
+    static const char c2x_table[] = "0123456789abcdef";
+
+    what = what & 0xff;
+    *where++ = c2x_table[what >> 4];
+    *where++ = c2x_table[what & 0x0f];
+
+    return where;
+}
+
+
+inline std::string string_to_hex(const std::string& input) {
+    static const char* const lut = "0123456789ABCDEF";
+    size_t len = input.length();
+
+    std::string output;
+    output.reserve(2 * len);
+    for (size_t i = 0; i < len; ++i) {
+        const unsigned char c = input[i];
+        output.push_back(lut[c >> 4]);
+        output.push_back(lut[c & 15]);
+    }
+    return output;
+}
+
+
+template<typename Operation>
+inline std::string toCaseHelper(std::string str, Operation op) {
+    std::transform(str.begin(),
+            str.end(),
+            str.begin(),
+            op);
+
+    return str;
+}
+
+
+inline std::string tolower(std::string str) { // cppcheck-suppress passedByValue ; copied value is used for in-place transformation
+    return toCaseHelper(str, ::tolower);
+}
+
+
+inline std::string toupper(std::string str) { // cppcheck-suppress passedByValue ; copied value is used for in-place transformation
+    return toCaseHelper(str, ::toupper);
+}
 
-unsigned char x2c(const unsigned char *what);
-unsigned char xsingle2c(const unsigned char *what);
-unsigned char *c2x(unsigned what, unsigned char *where);
 
 }  // namespace string
 }  // namespace utils

src/variables/rule.h

@@ -35,7 +35,7 @@ namespace variables {
 class Rule_DictElement : public VariableDictElement { \
  public:
     explicit Rule_DictElement(const std::string &dictElement)
-        : VariableDictElement(std::string("RULE"), dictElement) { }
+        : VariableDictElement(m_rule, dictElement) { }
 
     static void id(Transaction *t,
         RuleWithActions *rule,
@@ -49,13 +49,8 @@ class Rule_DictElement : public VariableDictElement { \
         if (!r || r->m_ruleId == 0) {
             return;
         }
-        std::string *a = new std::string(std::to_string(r->m_ruleId));
+
-        VariableValue *var = new VariableValue(&m_rule, &m_rule_id,
+        addVariableOrigin(m_rule_id, std::to_string(r->m_ruleId), l);
-            a
-        );
-        delete a;
-        var->addOrigin();
-        l->push_back(var);
     }
 
 
@@ -72,13 +67,7 @@ class Rule_DictElement : public VariableDictElement { \
             return;
         }
 
-        std::string *a = new std::string(r->m_rev);
+        addVariableOrigin(m_rule_rev, r->m_rev, l);
-        VariableValue *var = new VariableValue(&m_rule, &m_rule_rev,
-            a
-        );
-        delete a;
-        var->addOrigin();
-        l->push_back(var);
     }
 
 
@@ -92,13 +81,7 @@ class Rule_DictElement : public VariableDictElement { \
         }
 
         if (r && r->hasSeverity()) {
-            std::string *a = new std::string(std::to_string(r->severity()));
+            addVariableOrigin(m_rule_severity, std::to_string(r->severity()), l);
-            VariableValue *var = new VariableValue(&m_rule, &m_rule_severity,
-                a
-            );
-            delete a;
-            var->addOrigin();
-            l->push_back(var);
         }
     }
 
@@ -113,13 +96,7 @@ class Rule_DictElement : public VariableDictElement { \
         }
 
         if (r && r->hasLogData()) {
-            std::string *a = new std::string(r->logData(t));
+            addVariableOrigin(m_rule_logdata, r->logData(t), l);
-            VariableValue *var = new VariableValue(&m_rule, &m_rule_logdata,
-                a
-            );
-            delete a;
-            var->addOrigin();
-            l->push_back(var);
         }
     }
 
@@ -133,36 +110,30 @@ class Rule_DictElement : public VariableDictElement { \
         }
 
         if (r && r->hasMsg()) {
-            std::string *a = new std::string(r->msg(t));
+            addVariableOrigin(m_rule_msg, r->msg(t), l);
-            VariableValue *var = new VariableValue(&m_rule, &m_rule_msg,
-                a
-            );
-            delete a;
-            var->addOrigin();
-            l->push_back(var);
         }
     }
 
     void evaluate(Transaction *t,
         RuleWithActions *rule,
         std::vector<const VariableValue *> *l) override {
-        if (m_dictElement == "id") {
+        if (m_dictElement == m_rule_id) {
             id(t, rule, l);
             return;
         }
-        if (rule && m_dictElement == "rev") {
+        if (rule && m_dictElement == m_rule_rev) {
             rev(t, rule, l);
             return;
         }
-        if (rule && m_dictElement == "severity") {
+        if (rule && m_dictElement == m_rule_severity) {
             severity(t, rule, l);
             return;
         }
-        if (m_dictElement == "logdata") {
+        if (m_dictElement == m_rule_logdata) {
             logData(t, rule, l);
             return;
         }
-        if (m_dictElement == "msg") {
+        if (m_dictElement == m_rule_msg) {
             msg(t, rule, l);
             return;
         }
@@ -174,6 +145,18 @@ class Rule_DictElement : public VariableDictElement { \
     static const std::string m_rule_severity;
     static const std::string m_rule_logdata;
     static const std::string m_rule_msg;
+
+private:
+
+    static inline void addVariableOrigin(const std::string &key,
+        const std::string &value,
+        std::vector<const VariableValue *> *l) {
+        auto var = new VariableValue(&m_rule, &key,
+            &value
+        );
+        var->addOrigin();
+        l->push_back(var);
+    }
 };
 
 

src/variables/variable.h

@@ -707,9 +707,8 @@ class VariableModificatorCount : public Variable {
         }
         reslIn.clear();
 
-        std::string *res = new std::string(std::to_string(count));
+        auto res = std::to_string(count);
-        val = new VariableValue(m_fullName.get(), res);
+        val = new VariableValue(m_fullName.get(), &res);
-        delete res;
 
         l->push_back(val);
         return;

src/variables/xml.cc

@@ -124,13 +124,12 @@ void XML::evaluate(Transaction *t,
         content = reinterpret_cast<char *>(
             xmlNodeGetContent(nodes->nodeTab[i]));
         if (content != NULL) {
-            std::string *a = new std::string(content);
+            auto a = std::string(content);
             VariableValue *var = new VariableValue(m_fullName.get(),
-                a);
+                &a);
             if (!m_keyExclusion.toOmit(*m_fullName)) {
                 l->push_back(var);
             }
-            delete a;
             xmlFree(content);
          }
     }