diff options
author | dormando <dormando@rydia.net> | 2023-04-12 21:06:58 -0700 |
---|---|---|
committer | dormando <dormando@rydia.net> | 2023-04-12 21:06:58 -0700 |
commit | 4e310166bc1f633430f18f05ff4ac62f40124f31 (patch) | |
tree | 6d14ca981fc94a775e858250a8cdeb7e40754f46 | |
parent | 6a3c554972723e27905924c5db9605e3374a28d4 (diff) | |
download | memcached-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.c | 2 | ||||
-rw-r--r-- | proxy_lua.c | 9 |
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); } |