Skip to content

Commit

Permalink
MB-45599: Use stack buffer in SendResponse
Browse files Browse the repository at this point in the history
ep-engine copies the response handler and calls it from a background
thread in some cases and it'll end up racing on the thread local
scratch buffer we used to format the package header into.

To work around this problem use a stack allocated buffer as the
buffer is only 27 bytes big (24 for the header, 3 for frame info)

Change-Id: I405409e4fc4c565fc9eca3ae2d0355d1ff3103c7
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/151030
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Daniel Owen <owend@couchbase.com>
  • Loading branch information
trondn committed Apr 13, 2021
1 parent fe096c2 commit a16d072
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 12 deletions.
20 changes: 16 additions & 4 deletions daemon/connection.cc
Expand Up @@ -1598,14 +1598,20 @@ void Connection::setDcpFlowControlBufferSize(std::size_t size) {
}

std::string_view Connection::formatResponseHeaders(Cookie& cookie,
std::array<char, 2048>& dest,
cb::char_buffer dest,
cb::mcbp::Status status,
std::size_t extras_len,
std::size_t key_len,
std::size_t value_len,
uint8_t datatype) {
if (dest.size() < sizeof(cb::mcbp::Response) + MCBP_TRACING_RESPONSE_SIZE) {
throw std::runtime_error(
"Connection::formatResponseHeaders: The provided buffer must "
"be big enough to hold header and tracing frame info");
}

const auto& request = cookie.getRequest();
auto wbuf = cb::char_buffer{dest.data(), dest.size()};
auto wbuf = dest;
auto& response = *reinterpret_cast<cb::mcbp::Response*>(wbuf.data());

response.setOpcode(request.getClientOpcode());
Expand Down Expand Up @@ -1677,8 +1683,11 @@ void Connection::sendResponseHeaders(Cookie& cookie,
std::string_view key,
std::size_t value_len,
uint8_t datatype) {
std::array<char, sizeof(cb::mcbp::Response) + MCBP_TRACING_RESPONSE_SIZE>
buffer;

auto wbuf = formatResponseHeaders(cookie,
thread.scratch_buffer,
{buffer.data(), buffer.size()},
status,
extras.size(),
key.size(),
Expand All @@ -1704,8 +1713,11 @@ void Connection::sendResponse(Cookie& cookie,
cookie, status, extras, key, value.size(), datatype);
chainDataToOutputStream(std::move(sendbuffer));
} else {
std::array<char,
sizeof(cb::mcbp::Response) + MCBP_TRACING_RESPONSE_SIZE>
buffer;
auto wbuf = formatResponseHeaders(cookie,
thread.scratch_buffer,
{buffer.data(), buffer.size()},
status,
extras.size(),
key.size(),
Expand Down
10 changes: 2 additions & 8 deletions daemon/connection.h
Expand Up @@ -868,13 +868,7 @@ class Connection : public DcpMessageProducersIface {
* Format the response header into the provided buffer.
*
* @param cookie The command to create a response for
* @param dest The destination buffer (the reason the buffer is huge
* is that we'll be using the front-end-threads scratch
* buffer and I don't want to do runtime checks to verify
* that it is big enough when we can use a compile time
* check (if someone change the size of the front-end-threads
* scratch buffer we'll fail compile time when we pass it
* in here)
* @param dest The destination buffer
* @param status The status code
* @param extras_len The length of the extras section
* @param key_len The length of the key
Expand All @@ -883,7 +877,7 @@ class Connection : public DcpMessageProducersIface {
* @return A view into the destination buffer containing the header
*/
std::string_view formatResponseHeaders(Cookie& cookie,
std::array<char, 2048>& dest,
cb::char_buffer dest,
cb::mcbp::Status status,
std::size_t extras_len,
std::size_t key_len,
Expand Down

0 comments on commit a16d072

Please sign in to comment.