summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authordormando <dormando@rydia.net>2023-04-12 21:06:58 -0700
committerdormando <dormando@rydia.net>2023-04-12 21:06:58 -0700
commit4e310166bc1f633430f18f05ff4ac62f40124f31 (patch)
tree6d14ca981fc94a775e858250a8cdeb7e40754f46
parent6a3c554972723e27905924c5db9605e3374a28d4 (diff)
downloadmemcached-4e310166bc1f633430f18f05ff4ac62f40124f31.tar.gz
proxy: fix data corruption bug
Bug introduced in 6c80728: use after free for response buffer while under concurrency. The await code has a different method of wrapping up a lua coroutine than a standard response, so it was not managing the lifecycle of the response object properly, causing data buffers to be reused before being written back to the client. This fix separates the accounting of memory from the freeing of the buffer, so there is no more race. Further restructuring is needed to both make this less bug prone and make memory accounting be lock step with the memory freeing.
-rw-r--r--proto_proxy.c2
-rw-r--r--proxy_lua.c9
2 files changed, 8 insertions, 3 deletions
diff --git a/proto_proxy.c b/proto_proxy.c
index e604b40..2e1a2df 100644
--- a/proto_proxy.c
+++ b/proto_proxy.c
@@ -613,7 +613,9 @@ int proxy_run_coroutine(lua_State *Lc, mc_resp *resp, io_pending_proxy_t *p, con
// associated io_pending's of its own later.
} else if (r->buf) {
// response set from C.
+ resp->write_and_free = r->buf;
resp_add_iov(resp, r->buf, r->blen);
+ r->buf = NULL;
} else if (lua_getiuservalue(Lc, 1, 1) != LUA_TNIL) {
// uservalue slot 1 is pre-created, so we get TNIL instead of
// TNONE when nothing was set into it.
diff --git a/proxy_lua.c b/proxy_lua.c
index 7f969c5..c3a2af8 100644
--- a/proxy_lua.c
+++ b/proxy_lua.c
@@ -85,12 +85,15 @@ static int mcplib_response_gc(lua_State *L) {
LIBEVENT_THREAD *t = PROXY_GET_THR(L);
mcp_resp_t *r = luaL_checkudata(L, -1, "mcp.response");
+ // FIXME: we handle the accounting here, but the actual response buffer is
+ // freed elsewhere, after the network write.
+ pthread_mutex_lock(&t->proxy_limit_lock);
+ t->proxy_buffer_memory_used -= r->blen;
+ pthread_mutex_unlock(&t->proxy_limit_lock);
+
// On error/similar we might be holding the read buffer.
// If the buf is handed off to mc_resp for return, this pointer is NULL
if (r->buf != NULL) {
- pthread_mutex_lock(&t->proxy_limit_lock);
- t->proxy_buffer_memory_used -= r->blen;
- pthread_mutex_unlock(&t->proxy_limit_lock);
free(r->buf);
}