From b367fb9874f20c223d49989207d94eb654165535 Mon Sep 17 00:00:00 2001
From: Ilya Verbin <iverbin@tarantool.org>
Date: Mon, 21 Aug 2023 15:37:44 +0300
Subject: [PATCH] box: fix memory leak on error_unpack_unsafe() failure
Memory is leaked in the following scenario:
- MP_ERROR_STACK with 2 errors is passed to error_unpack_unsafe():
1. A correct MP_MAP with MP_ERROR_* fields;
2. Something unexpected, e.g. MP_INT;
- This first call to mp_decode_error_one() allocates memory for the first
error in error_build_xc() -> `new ClientError()`;
- The second call to mp_decode_error_one() returns NULL, and
error_unpack_unsafe() returns NULL too. Memory from the previous step
is leaked.
Closes #8921
NO_DOC=bugfix
---
...h-8921-memory-leak-in-xrow_decode_error.md | 4 +
src/box/mp_error.cc | 14 +-
test/unit/xrow.cc | 144 +++++++++++++++++-
3 files changed, 158 insertions(+), 4 deletions(-)
create mode 100644 changelogs/unreleased/gh-8921-memory-leak-in-xrow_decode_error.md
diff --git a/changelogs/unreleased/gh-8921-memory-leak-in-xrow_decode_error.md b/changelogs/unreleased/gh-8921-memory-leak-in-xrow_decode_error.md
new file mode 100644
index 0000000000..5e766ff3a3
--- /dev/null
+++ b/changelogs/unreleased/gh-8921-memory-leak-in-xrow_decode_error.md
@@ -0,0 +1,4 @@
+## bugfix/core
+
+* Fixed the memory leak on unpacking an invalid MsgPack error extension
+ (gh-8921).
diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
index a5d2ade401..03b3625224 100644
--- a/src/box/mp_error.cc
+++ b/src/box/mp_error.cc
@@ -482,7 +482,7 @@ error_unpack_unsafe(const char **data)
if (mp_typeof(**data) != MP_UINT) {
diag_set(ClientError, ER_INVALID_MSGPACK,
"Invalid MP_ERROR MsgPack format");
- return NULL;
+ goto error;
}
uint64_t key = mp_decode_uint(data);
switch(key) {
@@ -490,14 +490,14 @@ error_unpack_unsafe(const char **data)
if (mp_typeof(**data) != MP_ARRAY) {
diag_set(ClientError, ER_INVALID_MSGPACK,
"Invalid MP_ERROR MsgPack format");
- return NULL;
+ goto error;
}
uint32_t stack_sz = mp_decode_array(data);
struct error *effect = NULL;
for (uint32_t i = 0; i < stack_sz; i++) {
struct error *cur = mp_decode_error_one(data);
if (cur == NULL)
- return NULL;
+ goto error;
if (err == NULL) {
err = cur;
effect = cur;
@@ -514,6 +514,14 @@ error_unpack_unsafe(const char **data)
}
}
return err;
+
+error:
+ if (err != NULL) {
+ /* A hack to delete the error. */
+ error_ref(err);
+ error_unref(err);
+ }
+ return NULL;
}
struct error *
diff --git a/test/unit/xrow.cc b/test/unit/xrow.cc
index 4b2e68c170..25ea54db63 100644
--- a/test/unit/xrow.cc
+++ b/test/unit/xrow.cc
@@ -540,13 +540,151 @@ test_xrow_decode_unknown_key(void)
footer();
}
+static void
+test_xrow_decode_error_1(void)
+{
+ header();
+ plan(1);
+
+ uint8_t data[] = {
+ 0x81, /* MP_MAP of 1 element */
+ 0x52, /* IPROTO_ERROR: */
+ 0x00 /* MP_INT instead of MP_MAP */
+ };
+
+ struct iovec body;
+ body.iov_base = (void *)data;
+ body.iov_len = sizeof(data);
+
+ struct xrow_header row;
+ row.type = IPROTO_TYPE_ERROR | 111;
+ row.body[0] = body;
+ row.bodycnt = 1;
+
+ xrow_decode_error(&row);
+
+ struct error *e = diag_last_error(diag_get());
+ is(e->code, 111, "xrow_decode_error");
+ diag_destroy(diag_get());
+
+ check_plan();
+ footer();
+}
+
+static void
+test_xrow_decode_error_2(void)
+{
+ header();
+ plan(1);
+
+ uint8_t data[] = {
+ 0x81, /* MP_MAP of 1 element */
+ 0x52, /* IPROTO_ERROR: */
+ 0x81, /* MP_MAP of 1 element */
+ 0xa1 /* MP_STR instead of MP_UINT */
+ };
+
+ struct iovec body;
+ body.iov_base = (void *)data;
+ body.iov_len = sizeof(data);
+
+ struct xrow_header row;
+ row.type = IPROTO_TYPE_ERROR | 222;
+ row.body[0] = body;
+ row.bodycnt = 1;
+
+ xrow_decode_error(&row);
+
+ struct error *e = diag_last_error(diag_get());
+ is(e->code, 222, "xrow_decode_error");
+ diag_destroy(diag_get());
+
+ check_plan();
+ footer();
+}
+
+static void
+test_xrow_decode_error_3(void)
+{
+ header();
+ plan(1);
+
+ uint8_t data[] = {
+ 0x81, /* MP_MAP of 1 element */
+ 0x52, /* IPROTO_ERROR: */
+ 0x81, /* MP_MAP of 1 element */
+ 0x00, /* MP_ERROR_STACK: */
+ 0x00 /* MP_INT instead of MP_ARRAY */
+ };
+
+ struct iovec body;
+ body.iov_base = (void *)data;
+ body.iov_len = sizeof(data);
+
+ struct xrow_header row;
+ row.type = IPROTO_TYPE_ERROR | 333;
+ row.body[0] = body;
+ row.bodycnt = 1;
+
+ xrow_decode_error(&row);
+
+ struct error *e = diag_last_error(diag_get());
+ is(e->code, 333, "xrow_decode_error");
+ diag_destroy(diag_get());
+
+ check_plan();
+ footer();
+}
+
+static void
+test_xrow_decode_error_4(void)
+{
+ header();
+ plan(1);
+
+ uint8_t data[] = {
+ 0x81, /* MP_MAP of 1 element */
+ 0x52, /* IPROTO_ERROR: */
+ 0x81, /* MP_MAP of 1 element */
+ 0x00, /* MP_ERROR_STACK: */
+ 0x93, /* MP_ARRAY of 3 elements */
+ 0x83, /* MP_MAP of 3 elements */
+ 0x00, 0xa1, 0x00, /* MP_ERROR_TYPE: "" */
+ 0x01, 0xa1, 0x00, /* MP_ERROR_FILE: "" */
+ 0x03, 0xa1, 0x00, /* MP_ERROR_MESSAGE: "" */
+ 0x83, /* MP_MAP of 3 elements */
+ 0x00, 0xa1, 0x00, /* MP_ERROR_TYPE: "" */
+ 0x01, 0xa1, 0x00, /* MP_ERROR_FILE: "" */
+ 0x03, 0xa1, 0x00, /* MP_ERROR_MESSAGE: "" */
+ 0x00 /* MP_INT instead of MP_MAP */
+ };
+
+ struct iovec body;
+ body.iov_base = (void *)data;
+ body.iov_len = sizeof(data);
+
+ struct xrow_header row;
+ row.type = IPROTO_TYPE_ERROR | 444;
+ row.body[0] = body;
+ row.bodycnt = 1;
+
+ xrow_decode_error(&row);
+
+ struct error *e = diag_last_error(diag_get());
+ is(e->code, 444, "xrow_decode_error");
+ diag_destroy(diag_get());
+
+ check_plan();
+ footer();
+}
+
int
main(void)
{
memory_init();
fiber_init(fiber_c_invoke);
header();
- plan(6);
+ plan(10);
random_init();
@@ -557,6 +695,10 @@ main(void)
test_xrow_fields();
test_xrow_encode_dml();
test_xrow_decode_unknown_key();
+ test_xrow_decode_error_1();
+ test_xrow_decode_error_2();
+ test_xrow_decode_error_3();
+ test_xrow_decode_error_4();
random_free();
fiber_free();
--
GitLab