Perform UrlDecodeUni & UrlDecode transformations in-place

- Use std::string in UrlEncode transformation, instead of manually
  memory management. This avoids an additional copy after completing
  encoding by just swapping the encoded value and the input.
- Removed inplace helper function from the class, as it's only
  referenced by the implementation.

Author: eduar-hte

src/actions/transformations/url_decode.cc

@@ -27,25 +27,16 @@ UrlDecode::UrlDecode(const std::string &action)
 }
 
 bool UrlDecode::transform(std::string &value, const Transaction *trans) const {
-    unsigned char *val(NULL);
     int invalid_count = 0;
-    int _changed;
-
-    val = (unsigned char *) malloc(sizeof(char) * value.size() + 1);
-    memcpy(val, value.c_str(), value.size() + 1);
-    val[value.size()] = '\0';
-
-    int size = utils::urldecode_nonstrict_inplace(val, value.size(),
-        &invalid_count, &_changed);
-    std::string out;
-
-    out.append((const char *)val, size);
-
-    free(val);
-
-    const auto changed = out != value;
-    value = out;
-    return changed;
+    int changed;
+    const auto new_len = utils::urldecode_nonstrict_inplace(
+        (unsigned char*)value.data(),
+        value.length(),
+        &invalid_count,
+        &changed);
+
+    value.resize(new_len);
+    return changed != 0;
 }
 
 

src/actions/transformations/url_decode_uni.cc

@@ -22,41 +22,19 @@
 namespace modsecurity::actions::transformations {
 
 
-bool UrlDecodeUni::transform(std::string &value, const Transaction *t) const {
-    std::string ret;
-    unsigned char *input;
-
-    input = reinterpret_cast<unsigned char *>
-        (malloc(sizeof(char) * value.length()+1));
-
-    if (input == NULL) {
-        return "";
-    }
-
-    memcpy(input, value.c_str(), value.length()+1);
-
-    size_t i = inplace(input, value.length(), t);
-
-    ret.assign(reinterpret_cast<char *>(input), i);
-    free(input);
-
-    const auto changed = ret != value;
-    value = ret;
-    return changed;
-}
-
-
 /**
  *
  * IMP1 Assumes NUL-terminated
  */
-int UrlDecodeUni::inplace(unsigned char *input, uint64_t input_len,
+static inline bool inplace(std::string &value,
     const Transaction *t) {
-    unsigned char *d = input;
-    int64_t i, count, fact, j, xv;
-    int Code, hmap = -1;
+    bool changed = false;
+    auto d = reinterpret_cast<unsigned char*>(value.data());
+    const unsigned char *input = d;
+    const auto input_len = value.length();
 
-    if (input == NULL) return -1;
+    std::string::size_type i, count, fact, j, xv;
+    int Code, hmap = -1;
 
     i = count = 0;
     while (i < input_len) {
@@ -116,19 +94,17 @@ int UrlDecodeUni::inplace(unsigned char *input, uint64_t input_len,
                             }
                         }
                         d++;
-                        count++;
                         i += 6;
+                        changed = true;
                     } else {
                         /* Invalid data, skip %u. */
                         *d++ = input[i++];
                         *d++ = input[i++];
-                        count += 2;
                     }
                 } else {
                     /* Not enough bytes (4 data bytes), skip %u. */
                     *d++ = input[i++];
                     *d++ = input[i++];
-                    count += 2;
                 }
             } else {
                 /* Standard URL encoding. */
@@ -143,8 +119,8 @@ int UrlDecodeUni::inplace(unsigned char *input, uint64_t input_len,
 
                     if (VALID_HEX(c1) && VALID_HEX(c2)) {
                         *d++ = utils::string::x2c(&input[i + 1]);
-                        count++;
                         i += 3;
+                        changed = true;
                     } else {
                         /* Not a valid encoding, skip this % */
                         *d++ = input[i++];
@@ -153,25 +129,31 @@ int UrlDecodeUni::inplace(unsigned char *input, uint64_t input_len,
                 } else {
                     /* Not enough bytes available, skip this % */
                     *d++ = input[i++];
-                    count++;
                 }
             }
         } else {
             /* Character is not a percent sign. */
             if (input[i] == '+') {
                 *d++ = ' ';
+                changed = true;
             } else {
                 *d++ = input[i];
             }
 
-            count++;
             i++;
         }
     }
 
     *d = '\0';
 
-    return count;
+    value.resize(d - input);
+    return changed;
+}
+
+
+
+bool UrlDecodeUni::transform(std::string &value, const Transaction *trans) const {
+    return inplace(value, trans);
 }
 
 

src/actions/transformations/url_decode_uni.h

@@ -26,8 +26,6 @@ class UrlDecodeUni : public Transformation {
         : Transformation(action) { }
 
     bool transform(std::string &value, const Transaction *trans) const override;
-    static int inplace(unsigned char *input, uint64_t input_len,
-        const Transaction *transaction);
 };
 
 }  // namespace modsecurity::actions::transformations

src/actions/transformations/url_encode.cc

@@ -26,64 +26,42 @@ UrlEncode::UrlEncode(const std::string &action)
 }
 
 
-std::string UrlEncode::url_enc(const char *input,
-    unsigned int input_len, int *changed) {
-    char *rval, *d;
-    unsigned int i, len;
-    int count = 0;
+static inline bool url_enc(std::string &value) {
+    const auto len = value.size() * 3 + 1;
+    std::string ret(len, {});
 
-    *changed = 0;
-
-    len = input_len * 3 + 1;
-    d = rval = reinterpret_cast<char *>(malloc(len));
-    if (rval == NULL) {
-        return {};
-    }
+    bool changed = false;
 
     /* ENH Only encode the characters that really need to be encoded. */
 
-    for (i = 0; i < input_len; i++) {
-        unsigned char c = input[i];
-
+    char *d = ret.data();
+    for (const auto c : value) {
         if (c == ' ') {
             *d++ = '+';
-            *changed = 1;
-            count++;
+            changed = true;
         } else {
             if ( (c == 42) || ((c >= 48) && (c <= 57))
                 || ((c >= 65) && (c <= 90))
                 || ((c >= 97) && (c <= 122))) {
                 *d++ = c;
-                count++;
-            } else {
+            }
+            else
+            {
                 *d++ = '%';
-                count++;
-                utils::string::c2x(c, (unsigned char *)d);
-                d += 2;
-                count++;
-                count++;
-                *changed = 1;
+                d = (char *)utils::string::c2x(c, (unsigned char *)d);
+                changed = true;
             }
         }
     }
 
-    *d = '\0';
-
-    std::string ret("");
-    ret.append(rval, count);
-    free(rval);
-    return ret;
+    ret.resize(d - ret.c_str());
+    std::swap(value, ret);
+    return changed;
 }
 
 
 bool UrlEncode::transform(std::string &value, const Transaction *trans) const {
-    int _changed;
-
-    std::string ret = url_enc(value.c_str(), value.size(), &_changed);
-
-    const auto changed = ret != value;
-    value = ret;
-    return changed;
+    return url_enc(value);
 }
 
 

src/actions/transformations/url_encode.h

@@ -25,9 +25,6 @@ class UrlEncode : public Transformation {
     explicit UrlEncode(const std::string &action);
 
     bool transform(std::string &value, const Transaction *trans) const override;
-
-    static std::string url_enc(const char *input,
-        unsigned int input_len, int *changed);
 };
 
 }  // namespace modsecurity::actions::transformations