From ea715795a3c33da88dddfc9775ae08c58013f753 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Tue, 3 Nov 2026 16:22:33 +0100 Subject: [PATCH 1/2] change!: preallocate buffer for request and response bodies BREAKING CHANGE: Transaction::m_requestBody and Transaction::m_responseBody type changed from std::ostringstream to std::string --- headers/modsecurity/anchored_variable.h | 2 +- headers/modsecurity/transaction.h | 7 +- src/anchored_variable.cc | 13 +- src/transaction.cc | 159 ++++++++++++++++-------- 4 files changed, 126 insertions(+), 55 deletions(-) diff --git a/headers/modsecurity/anchored_variable.h b/headers/modsecurity/anchored_variable.h index b3cc101b35..32d1620033 100644 --- a/headers/modsecurity/anchored_variable.h +++ b/headers/modsecurity/anchored_variable.h @@ -51,6 +51,7 @@ class AnchoredVariable { void unset(); void set(const std::string &a, size_t offset); + void set(std::string &&a, size_t offset); void set(const std::string &a, size_t offset, size_t offsetLen); void evaluate(std::vector *l); @@ -72,4 +73,3 @@ class AnchoredVariable { #endif // HEADERS_MODSECURITY_ANCHORED_VARIABLE_H_ - diff --git a/headers/modsecurity/transaction.h b/headers/modsecurity/transaction.h index 5a98c1e336..af9d66d020 100644 --- a/headers/modsecurity/transaction.h +++ b/headers/modsecurity/transaction.h @@ -568,12 +568,12 @@ class Transaction : public TransactionAnchoredVariables, public TransactionSecMa /** * Holds the request body, in case of any. */ - std::ostringstream m_requestBody; + std::string m_requestBody; /** * Holds the response body, in case of any. */ - std::ostringstream m_responseBody; + std::string m_responseBody; /** * Contains the unique ID of the transaction. Use by the variable @@ -660,6 +660,9 @@ class Transaction : public TransactionAnchoredVariables, public TransactionSecMa Transaction(ModSecurity *ms, RulesSet *rules, const char *id, void *logCbData, const time_t timestamp); + void reserveBody(std::string &body, AnchoredSetVariable &headers, + double limit, size_t incoming); + /** * Pointer to the callback function that will be called to fill * the web server (connector) log. diff --git a/src/anchored_variable.cc b/src/anchored_variable.cc index 51860d1fe6..06123743a0 100644 --- a/src/anchored_variable.cc +++ b/src/anchored_variable.cc @@ -57,13 +57,22 @@ void AnchoredVariable::set(const std::string &a, size_t offset) { } +void AnchoredVariable::set(std::string &&a, size_t offset) { + m_offset = offset; + m_value = std::move(a); + m_var.addOrigin(m_value.size(), offset); +} + + void AnchoredVariable::evaluate(std::vector *l) { if (m_name.empty()) { return; } - m_var.setValue(m_value); - l->push_back(new VariableValue(&m_var)); + auto value = std::make_unique(&m_var); + value->setValue(m_value); + l->push_back(value.get()); + value.release(); } diff --git a/src/transaction.cc b/src/transaction.cc index 792ac0bd76..61d0663d76 100644 --- a/src/transaction.cc +++ b/src/transaction.cc @@ -23,12 +23,15 @@ #include #include +#include #include #include #include #include #include +#include #include +#include #include #include @@ -162,11 +165,9 @@ Transaction::Transaction(ModSecurity *ms, RulesSet *rules, const char *id, Transaction::~Transaction() { - m_responseBody.str(std::string()); - m_responseBody.clear(); + std::string().swap(m_responseBody); - m_requestBody.str(std::string()); - m_requestBody.clear(); + std::string().swap(m_requestBody); m_rulesMessages.clear(); @@ -686,7 +687,7 @@ int Transaction::processRequestBody() { /* * Process the request body even if there is nothing to be done. * - * if (m_requestBody.tellp() <= 0) { + * if (m_requestBody.empty()) { * return true; * } * @@ -699,7 +700,7 @@ int Transaction::processRequestBody() { (m_requestBodyProcessor == JSONRequestBody) || (m_requestBodyProcessor == XMLRequestBody)) { if ((m_rules->m_requestBodyNoFilesLimit.m_set) - && (m_requestBody.str().size() > m_rules->m_requestBodyNoFilesLimit.m_value)) { + && (m_requestBody.size() > m_rules->m_requestBodyNoFilesLimit.m_value)) { m_variableReqbodyError.set("1", 0); m_variableReqbodyErrorMsg.set("Request body excluding files is bigger than the maximum expected.", 0); m_variableInboundDataError.set("1", m_variableOffset); @@ -715,8 +716,8 @@ int Transaction::processRequestBody() { if (!requestBodyNoFilesLimitExceeded) { std::string error; if (m_xml->init() == true) { - m_xml->processChunk(m_requestBody.str().c_str(), - m_requestBody.str().size(), + m_xml->processChunk(m_requestBody.c_str(), + m_requestBody.size(), &error); m_xml->complete(&error); } @@ -746,12 +747,12 @@ int Transaction::processRequestBody() { m_json->setMaxDepth(m_rules->m_requestBodyJsonDepthLimit.m_value); } if (m_json->init() == true) { - m_json->processChunk(m_requestBody.str().c_str(), - m_requestBody.str().size(), + m_json->processChunk(m_requestBody.c_str(), + m_requestBody.size(), &error); m_json->complete(&error); } - if (error.empty() == false && m_requestBody.str().size() > 0) { + if (error.empty() == false && m_requestBody.size() > 0) { m_variableReqbodyError.set("1", m_variableOffset); m_variableReqbodyProcessorError.set("1", m_variableOffset); m_variableReqbodyErrorMsg.set("JSON parsing error: " + error, @@ -774,7 +775,7 @@ int Transaction::processRequestBody() { if (a != NULL) { Multipart m(*a, this); if (m.init(&error) == true) { - m.process(m_requestBody.str(), &error, m_variableOffset); + m.process(m_requestBody, &error, m_variableOffset); } reqbodyNoFilesLength = m.m_reqbody_no_files_length; m.multipart_complete(&error); @@ -801,7 +802,7 @@ int Transaction::processRequestBody() { m_variableOffset++; // large size might cause issues in the parsing itself; omit if exceeded if (!requestBodyNoFilesLimitExceeded) { - extractArguments("POST", m_requestBody.str(), m_variableOffset); + extractArguments("POST", m_requestBody, m_variableOffset); } } else if (m_requestBodyType != UnknownFormat) { /** @@ -855,16 +856,16 @@ int Transaction::processRequestBody() { } fullRequest = fullRequest + "\n\n"; - fullRequest = fullRequest + m_requestBody.str(); - m_variableFullRequest.set(fullRequest, m_variableOffset); - m_variableFullRequestLength.set(std::to_string(fullRequest.size()), + fullRequest.append(m_requestBody); + const size_t fullRequestLength = fullRequest.size(); + m_variableFullRequest.set(std::move(fullRequest), m_variableOffset); + m_variableFullRequestLength.set(std::to_string(fullRequestLength), m_variableOffset); - if (m_requestBody.tellp() > 0) { - m_variableRequestBody.set(m_requestBody.str(), m_variableOffset); - m_variableRequestBodyLength.set(std::to_string( - m_requestBody.str().size()), - m_variableOffset, m_requestBody.str().size()); + if (!m_requestBody.empty()) { + m_variableRequestBody.set(m_requestBody, m_variableOffset); + m_variableRequestBodyLength.set(std::to_string(m_requestBody.size()), + m_variableOffset, m_requestBody.size()); } this->m_rules->evaluate(modsecurity::RequestBodyPhase, this); @@ -927,7 +928,7 @@ int Transaction::requestBodyFromFile(const char *path) { } int Transaction::appendRequestBody(const unsigned char *buf, size_t len) { - int current_size = this->m_requestBody.tellp(); + size_t current_size = this->m_requestBody.size(); ms_dbg(9, "Appending request body: " + std::to_string(len) + " bytes. " \ "Limit set to: " @@ -942,7 +943,10 @@ int Transaction::appendRequestBody(const unsigned char *buf, size_t len) { RulesSet::BodyLimitAction::ProcessPartialBodyLimitAction) { size_t spaceLeft = this->m_rules->m_requestBodyLimit.m_value - current_size; - this->m_requestBody.write(reinterpret_cast(buf), + reserveBody(m_requestBody, m_variableRequestHeaders, + m_rules->m_requestBodyLimit.m_set + ? m_rules->m_requestBodyLimit.m_value : 0, spaceLeft); + this->m_requestBody.append(reinterpret_cast(buf), spaceLeft); ms_dbg(5, "Request body limit is marked to process partial"); return false; @@ -966,12 +970,69 @@ int Transaction::appendRequestBody(const unsigned char *buf, size_t len) { } } - this->m_requestBody.write(reinterpret_cast(buf), len); + reserveBody(m_requestBody, m_variableRequestHeaders, + m_rules->m_requestBodyLimit.m_set + ? m_rules->m_requestBodyLimit.m_value : 0, len); + this->m_requestBody.append(reinterpret_cast(buf), len); return true; } +/* + * Reserve body storage on the first non-empty append. Content-Length is only + * a client-provided hint, so the reservation is always capped by + * the corresponding body limit and is skipped when no limit is configured. + */ +void Transaction::reserveBody(std::string &body, AnchoredSetVariable &headers, + double limit, size_t incoming) { + if (incoming == 0 || !body.empty()) { + return; + } + + if (!(limit > 0)) { + return; + } + + std::unique_ptr contentLength = + headers.resolveFirst("Content-Length"); + if (contentLength == nullptr || contentLength->empty() + || contentLength->size() > 20) { + return; + } + + size_t expected = 0; + for (const char c : *contentLength) { + if (c < '0' || c > '9') { + return; + } + const size_t digit = static_cast(c - '0'); + if (expected > (SIZE_MAX - digit) / 10) { + return; + } + expected = expected * 10 + digit; + } + + if (static_cast(expected) > limit) { + expected = static_cast(limit); + } + + if (expected <= incoming || expected <= body.capacity()) { + return; + } + + try { + body.reserve(expected); + } catch (const std::bad_alloc &) { + ms_dbg(5, "Failed to reserve body storage, continuing " \ + "without reservation."); + } catch (const std::length_error &) { + ms_dbg(5, "Body reservation exceeds string capacity, continuing " \ + "without reservation."); + } +} + + /** * @name processResponseHeaders * @brief Perform the analysis on the response readers. @@ -1099,13 +1160,13 @@ int Transaction::addResponseHeader(const unsigned char *key, size_t key_n, /** * @name processResponseBody - * @brief Perform the request body (if any) + * @brief Perform the response body (if any) * - * This method perform the analysis on the request body. It is optional to + * This method perform the analysis on the response body. It is optional to * call that method. If this API consumer already know that there isn't a * body for inspect it is recommended to skip this step. * - * @note It is necessary to "append" the request body prior to the execution + * @note It is necessary to "append" the response body prior to the execution * of this method. * @note Remember to check for a possible intervention. * @@ -1148,9 +1209,9 @@ int Transaction::processResponseBody() { m_variableOutboundDataError.set("0", m_variableOffset); } - m_variableResponseBody.set(m_responseBody.str(), m_variableOffset); + m_variableResponseBody.set(m_responseBody, m_variableOffset); m_variableResponseContentLength.set(std::to_string( - m_responseBody.str().size()), m_variableOffset); + m_responseBody.size()), m_variableOffset); m_rules->evaluate(modsecurity::ResponseBodyPhase, this); return true; @@ -1176,7 +1237,7 @@ int Transaction::processResponseBody() { * */ int Transaction::appendResponseBody(const unsigned char *buf, size_t len) { - int current_size = this->m_responseBody.tellp(); + size_t current_size = this->m_responseBody.size(); const std::set &bi = \ this->m_rules->m_responseBodyTypeToBeInspected.m_value; @@ -1201,7 +1262,10 @@ int Transaction::appendResponseBody(const unsigned char *buf, size_t len) { RulesSet::BodyLimitAction::ProcessPartialBodyLimitAction) { size_t spaceLeft = this->m_rules->m_responseBodyLimit.m_value \ - current_size; - this->m_responseBody.write(reinterpret_cast(buf), + reserveBody(m_responseBody, m_variableResponseHeaders, + m_rules->m_responseBodyLimit.m_set + ? m_rules->m_responseBodyLimit.m_value : 0, spaceLeft); + this->m_responseBody.append(reinterpret_cast(buf), spaceLeft); ms_dbg(5, "Response body limit is marked to process partial"); return false; @@ -1225,7 +1289,10 @@ int Transaction::appendResponseBody(const unsigned char *buf, size_t len) { } } - this->m_responseBody.write(reinterpret_cast(buf), len); + reserveBody(m_responseBody, m_variableResponseHeaders, + m_rules->m_responseBodyLimit.m_set + ? m_rules->m_responseBodyLimit.m_value : 0, len); + this->m_responseBody.append(reinterpret_cast(buf), len); return true; } @@ -1245,7 +1312,7 @@ int Transaction::appendResponseBody(const unsigned char *buf, size_t len) { * */ const char *Transaction::getResponseBody() const { - return strdup(this->m_responseBody.str().c_str()); + return strdup(this->m_responseBody.c_str()); } @@ -1260,11 +1327,7 @@ const char *Transaction::getResponseBody() const { * */ size_t Transaction::getResponseBodyLength() { - size_t size = 0; - m_responseBody.seekp(0, std::ios::end); - size = m_responseBody.tellp(); - - return size; + return m_responseBody.size(); } /** @@ -1282,8 +1345,7 @@ size_t Transaction::getResponseBodyLength() { size_t Transaction::getRequestBodyLength() { size_t size = 0; - m_requestBody.seekp(0, std::ios::end); - size = m_requestBody.tellp(); + size = m_requestBody.size(); return size; } @@ -1430,7 +1492,7 @@ std::string Transaction::toOldAuditLogFormatIndex(const std::string &filename, ss << "\" "; ss << this->m_httpCodeReturned << " "; - ss << this->m_responseBody.tellp() << " "; + ss << this->m_responseBody.size() << " "; /** TODO: Check variable */ ss << utils::string::dash_if_empty( m_variableRequestHeaders.resolveFirst("REFERER").get()) << " "; @@ -1492,12 +1554,9 @@ std::string Transaction::toOldAuditLogFormat(int parts, audit_log << prefix << std::endl; } if (parts & audit_log::AuditLog::CAuditLogPart - && m_requestBody.tellp() > 0) { - std::string body = m_requestBody.str(); + && !m_requestBody.empty()) { audit_log << prefix << "--" << trailer << "-" << "C--" << std::endl; - if (body.size() > 0) { - audit_log << prefix << body << std::endl; - } + audit_log << prefix << m_requestBody << std::endl; audit_log << prefix << std::endl; } if (parts & audit_log::AuditLog::DAuditLogPart) { @@ -1506,8 +1565,8 @@ std::string Transaction::toOldAuditLogFormat(int parts, /** TODO: write audit_log D part. */ } if (parts & audit_log::AuditLog::EAuditLogPart - && m_responseBody.tellp() > 0) { - std::string body = utils::string::toHexIfNeeded(m_responseBody.str()); + && !m_responseBody.empty()) { + std::string body = utils::string::toHexIfNeeded(m_responseBody); audit_log << prefix << "--" << trailer << "-" << "E--" << std::endl; if (body.size() > 0) { audit_log << prefix << body << std::endl; @@ -1615,7 +1674,7 @@ std::string Transaction::toJSON(int parts) { if (parts & audit_log::AuditLog::CAuditLogPart) { // FIXME: check for the binary content size. - LOGFY_ADD("body", utils::string::toHexIfNeeded(this->m_requestBody.str())); + LOGFY_ADD("body", utils::string::toHexIfNeeded(this->m_requestBody)); } /* request headers */ @@ -1644,7 +1703,7 @@ std::string Transaction::toJSON(int parts) { yajl_gen_map_open(g); if (parts & audit_log::AuditLog::EAuditLogPart) { - LOGFY_ADD("body", this->m_responseBody.str()); + LOGFY_ADD("body", this->m_responseBody); } LOGFY_ADD_NUM("http_code", m_httpCodeReturned); From b5da27f130b16a83929a714806bdb789944a11ef Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Tue, 3 Nov 2026 16:43:57 +0100 Subject: [PATCH 2/2] fix: new const Transaction::{getResponseBodyLength,getRequestBodyLength}() methods --- headers/modsecurity/transaction.h | 2 ++ src/transaction.cc | 40 +++++++++++++++++++++++++------ 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/headers/modsecurity/transaction.h b/headers/modsecurity/transaction.h index af9d66d020..02337b117f 100644 --- a/headers/modsecurity/transaction.h +++ b/headers/modsecurity/transaction.h @@ -410,6 +410,8 @@ class Transaction : public TransactionAnchoredVariables, public TransactionSecMa const char *getResponseBody() const; size_t getResponseBodyLength(); size_t getRequestBodyLength(); + size_t getResponseBodyLength() const; + size_t getRequestBodyLength() const; #ifndef NO_LOGS void debug(int level, const std::string& message) const; diff --git a/src/transaction.cc b/src/transaction.cc index 61d0663d76..45c2bdc128 100644 --- a/src/transaction.cc +++ b/src/transaction.cc @@ -1315,6 +1315,19 @@ const char *Transaction::getResponseBody() const { return strdup(this->m_responseBody.c_str()); } +/** + * @name getResponseBodyLength + * @brief Retrieve the length of the response body. + * + * This method returns the size of the response body buffer. + * + * + * @return Size of the update response body. + * + */ +size_t Transaction::getResponseBodyLength() const { + return m_responseBody.size(); +} /** * @name getResponseBodyLength @@ -1326,8 +1339,9 @@ const char *Transaction::getResponseBody() const { * @return Size of the update response body. * */ +// cppcheck-suppress functionConst - keep the old signatures for ABI compatibility size_t Transaction::getResponseBodyLength() { - return m_responseBody.size(); + return static_cast(this)->getResponseBodyLength(); } /** @@ -1342,14 +1356,26 @@ size_t Transaction::getResponseBodyLength() { * @return Size of the request body. * */ -size_t Transaction::getRequestBodyLength() { - size_t size = 0; - - size = m_requestBody.size(); - - return size; +size_t Transaction::getRequestBodyLength() const { + return m_requestBody.size(); } +/** + * @name getRequestBodyLength + * @brief Retrieve the length of the request body. + * + * This method returns the size of the request body buffer, notice + * however, that most likely there isn't an update. Thus, this method will + * return 0. + * + * + * @return Size of the request body. + * + */ +// cppcheck-suppress functionConst - keep the old signatures for ABI compatibility +size_t Transaction::getRequestBodyLength() { + return static_cast(this)->getRequestBodyLength(); +} /** * @name processLogging