From 2049b9f39ef7e47ebcf0a2022c408d3311b4c8bc Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov@tarantool.org>
Date: Mon, 16 May 2022 14:50:23 +0300
Subject: [PATCH] iproto: log error before iproto_write_error
iproto_write_error() may reset diag (for example, if the client closes
the socket), thus invalidating the error we are going to log and leading
to a use after free bug. We must log the error before trying to send it
to the client.
The bug was introduced by commit 9b4ab9fe0ef05 ("iproto: use iostream
abstraction"), which switched iproto_write_error() from plain write() to
the iostream_write(), which may set diag.
Closes #6890
NO_DOC=bug fix
NO_CHANGELOG=unreleased
(cherry picked from commit 731170591d00ad89752242ee8bb4572803d9aa55)
---
src/box/iproto.cc | 4 +--
..._6890_iproto_error_use_after_free_test.lua | 31 +++++++++++++++++++
2 files changed, 33 insertions(+), 2 deletions(-)
create mode 100644 test/box-luatest/gh_6890_iproto_error_use_after_free_test.lua
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 1b7ee72cf2..40154d7fe3 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1230,8 +1230,8 @@ iproto_connection_resume(struct iproto_connection *con)
*/
if (iproto_enqueue_batch(con, con->p_ibuf) != 0) {
struct error *e = box_error_last();
- iproto_write_error(&con->io, e, ::schema_version, 0);
error_log(e);
+ iproto_write_error(&con->io, e, ::schema_version, 0);
iproto_connection_close(con);
}
}
@@ -1317,9 +1317,9 @@ iproto_connection_on_input(ev_loop *loop, struct ev_io *watcher,
if (iproto_enqueue_batch(con, in) != 0)
diag_raise();
} catch (Exception *e) {
+ e->log();
/* Best effort at sending the error message to the client. */
iproto_write_error(io, e, ::schema_version, 0);
- e->log();
iproto_connection_close(con);
}
}
diff --git a/test/box-luatest/gh_6890_iproto_error_use_after_free_test.lua b/test/box-luatest/gh_6890_iproto_error_use_after_free_test.lua
new file mode 100644
index 0000000000..4da08b1ca7
--- /dev/null
+++ b/test/box-luatest/gh_6890_iproto_error_use_after_free_test.lua
@@ -0,0 +1,31 @@
+local msgpack = require('msgpack')
+local server = require('test.luatest_helpers.server')
+local socket = require('socket')
+local uri = require('uri')
+local t = require('luatest')
+local g = t.group()
+
+g.before_all(function()
+ g.server = server:new({alias = 'master'})
+ g.server:start()
+end)
+
+g.after_all(function()
+ g.server:drop()
+end)
+
+g.test_iproto_error_use_after_free = function()
+ -- Connect to the server.
+ local u = uri.parse(g.server.net_box_uri)
+ local s = socket.tcp_connect(u.host, u.service)
+ t.assert_is_not(s, nil)
+ -- Skip the greeting.
+ t.assert_equals(#s:read(128), 128)
+ -- Send an invalid request and immediately close the socket so that
+ -- the server fails to send the error back.
+ local d = msgpack.encode('foo')
+ t.assert_equals(s:write(d), #d)
+ s:close()
+ -- Check that the server logs the proper error.
+ t.assert(g.server:grep_log('ER_INVALID_MSGPACK'))
+end
--
GitLab