* [Tarantool-patches] [PATCH 1/2] swim: fix crash on bad member_by_uuid() call
2021-04-04 15:06 [Tarantool-patches] [PATCH 0/2] Swim misusage crashes Vladislav Shpilevoy via Tarantool-patches
@ 2021-04-04 15:06 ` Vladislav Shpilevoy via Tarantool-patches
2021-04-04 15:06 ` [Tarantool-patches] [PATCH 2/2] swim: check types in __serialize methods Vladislav Shpilevoy via Tarantool-patches
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-04 15:06 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
In Lua swim object's method member_by_uuid() could crash if called
with no arguments. UUID was then passed as NULL, and dereferenced.
The patch makes member_by_uuid() treat NULL like nil UUID and
return NULL (member not found). The reason is that
swim_member_by_uuid() can't fail. It can only return a member or
not. It never sets a diag error.
Closes #5951
---
.../unreleased/swim-member_by_uuid-crash.md | 4 ++++
src/lib/swim/swim.c | 2 ++
test/swim/swim.result | 9 ++++++++
test/swim/swim.test.lua | 3 +++
test/unit/swim.c | 23 ++++++++++++++++++-
test/unit/swim.result | 9 +++++++-
6 files changed, 48 insertions(+), 2 deletions(-)
create mode 100644 changelogs/unreleased/swim-member_by_uuid-crash.md
diff --git a/changelogs/unreleased/swim-member_by_uuid-crash.md b/changelogs/unreleased/swim-member_by_uuid-crash.md
new file mode 100644
index 000000000..d710aaa73
--- /dev/null
+++ b/changelogs/unreleased/swim-member_by_uuid-crash.md
@@ -0,0 +1,4 @@
+## bugfix/swim
+
+* Fix the crash on an attempt to call `swim:member_by_uuid()` with no arguments
+ or with `nil`/`box.NULL` (gh-5951).
diff --git a/src/lib/swim/swim.c b/src/lib/swim/swim.c
index 9b5458635..1ecc90414 100644
--- a/src/lib/swim/swim.c
+++ b/src/lib/swim/swim.c
@@ -2337,6 +2337,8 @@ struct swim_member *
swim_member_by_uuid(struct swim *swim, const struct tt_uuid *uuid)
{
assert(swim_is_configured(swim));
+ if (uuid == NULL)
+ return NULL;
return swim_find_member(swim, uuid);
}
diff --git a/test/swim/swim.result b/test/swim/swim.result
index bfc83c143..bab8d20eb 100644
--- a/test/swim/swim.result
+++ b/test/swim/swim.result
@@ -487,6 +487,15 @@ s1:member_by_uuid(50)
- error: 'builtin/swim.lua:<line>: swim:member_by_uuid: expected string UUID or struct
tt_uuid'
...
+-- gh-5951: could crash with no arguments or NULL.
+s1:member_by_uuid()
+---
+- null
+...
+s1:member_by_uuid(box.NULL)
+---
+- null
+...
s1:member_by_uuid(uuid(2))
---
- null
diff --git a/test/swim/swim.test.lua b/test/swim/swim.test.lua
index 1593d8833..bec8b9cca 100644
--- a/test/swim/swim.test.lua
+++ b/test/swim/swim.test.lua
@@ -159,6 +159,9 @@ s.is_dropped()
s1:member_by_uuid(uuid(1)) ~= nil
s1:member_by_uuid(50)
+-- gh-5951: could crash with no arguments or NULL.
+s1:member_by_uuid()
+s1:member_by_uuid(box.NULL)
s1:member_by_uuid(uuid(2))
-- UUID can be cdata.
diff --git a/test/unit/swim.c b/test/unit/swim.c
index a506d04e9..a495f9648 100644
--- a/test/unit/swim.c
+++ b/test/unit/swim.c
@@ -1071,10 +1071,30 @@ swim_test_suspect_new_members(void)
swim_finish_test();
}
+static void
+swim_test_member_by_uuid(void)
+{
+ swim_start_test(3);
+ struct swim_cluster *cluster = swim_cluster_new(1);
+
+ struct swim *s1 = swim_cluster_member(cluster, 0);
+ const struct swim_member *s1_self = swim_self(s1);
+ is(swim_member_by_uuid(s1, swim_member_uuid(s1_self)), s1_self,
+ "found by UUID")
+
+ struct tt_uuid uuid = uuid_nil;
+ uuid.time_low = 1000;
+ is(swim_member_by_uuid(s1, &uuid), NULL, "not found by valid UUID");
+ is(swim_member_by_uuid(s1, NULL), NULL, "not found by NULL UUID");
+
+ swim_cluster_delete(cluster);
+ swim_finish_test();
+}
+
static int
main_f(va_list ap)
{
- swim_start_test(22);
+ swim_start_test(23);
(void) ap;
fakeev_init();
@@ -1102,6 +1122,7 @@ main_f(va_list ap)
swim_test_generation();
swim_test_dissemination_speed();
swim_test_suspect_new_members();
+ swim_test_member_by_uuid();
fakenet_free();
fakeev_free();
diff --git a/test/unit/swim.result b/test/unit/swim.result
index d4b1dba49..e1af4d616 100644
--- a/test/unit/swim.result
+++ b/test/unit/swim.result
@@ -1,5 +1,5 @@
*** main_f ***
-1..22
+1..23
*** swim_test_one_link ***
1..6
ok 1 - no rounds - no fullmesh
@@ -225,4 +225,11 @@ ok 21 - subtests
ok 2 - S3 didn't add S1 from S2's messages, because S1 didn't answer on a ping
ok 22 - subtests
*** swim_test_suspect_new_members: done ***
+ *** swim_test_member_by_uuid ***
+ 1..3
+ ok 1 - found by UUID
+ ok 2 - not found by valid UUID
+ ok 3 - not found by NULL UUID
+ok 23 - subtests
+ *** swim_test_member_by_uuid: done ***
*** main_f: done ***
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 6+ messages in thread* [Tarantool-patches] [PATCH 2/2] swim: check types in __serialize methods
2021-04-04 15:06 [Tarantool-patches] [PATCH 0/2] Swim misusage crashes Vladislav Shpilevoy via Tarantool-patches
2021-04-04 15:06 ` [Tarantool-patches] [PATCH 1/2] swim: fix crash on bad member_by_uuid() call Vladislav Shpilevoy via Tarantool-patches
@ 2021-04-04 15:06 ` Vladislav Shpilevoy via Tarantool-patches
2021-04-04 20:42 ` [Tarantool-patches] [PATCH 0/2] Swim misusage crashes Cyrill Gorcunov via Tarantool-patches
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-04 15:06 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
In swim Lua code none of the __serialize methods checked the
argument type assuming that nobody would call them directly and
mess with the types. But it happened, and is not hard to fix, so
the patch does it.
The serialization functions are sanitized for the swim object,
swim member, and member event.
Closes #5952
---
changelogs/unreleased/swim-serialize-crash.md | 4 +
src/lua/swim.lua | 41 +++++--
test/swim/gh-5952-swim-serialize.result | 101 ++++++++++++++++++
test/swim/gh-5952-swim-serialize.test.lua | 33 ++++++
4 files changed, 169 insertions(+), 10 deletions(-)
create mode 100644 changelogs/unreleased/swim-serialize-crash.md
create mode 100644 test/swim/gh-5952-swim-serialize.result
create mode 100644 test/swim/gh-5952-swim-serialize.test.lua
diff --git a/changelogs/unreleased/swim-serialize-crash.md b/changelogs/unreleased/swim-serialize-crash.md
new file mode 100644
index 000000000..e48b1f518
--- /dev/null
+++ b/changelogs/unreleased/swim-serialize-crash.md
@@ -0,0 +1,4 @@
+## bugfix/swim
+
+* Fix the crash on an attempt to pass an object of a wrong type to `__serialize`
+ method of a swim member in Lua (gh-5952).
diff --git a/src/lua/swim.lua b/src/lua/swim.lua
index f61aa6b58..027f07038 100644
--- a/src/lua/swim.lua
+++ b/src/lua/swim.lua
@@ -322,6 +322,16 @@ local function swim_check_member(m, func_name)
return error(func_name..': first argument is not a SWIM member')
end
+local function swim_check_event(event, func_name)
+ if type(event) == 'table' then
+ local value = event[1]
+ if type(value) == 'number' then
+ return value
+ end
+ end
+ return error(func_name..': first argument is not a SWIM event')
+end
+
--
-- Member methods. Most of them are one-liners, not much to
-- comment.
@@ -435,7 +445,8 @@ local function swim_member_uuid(m)
end
local function swim_member_serialize(m)
- local _, size = swim_member_payload_raw(m.ptr)
+ local ptr = swim_check_member(m, 'member:__serialize()')
+ local _, size = swim_member_payload_raw(ptr)
return {
status = swim_member_status(m),
uuid = swim_member_uuid(m),
@@ -543,6 +554,7 @@ end
-- serialized, for example, in a console.
--
local function swim_serialize(s)
+ swim_check_instance(s, 'swim:__serialize()')
return s.cfg.index
end
@@ -739,31 +751,40 @@ end
local swim_member_event_index = {
is_new = function(self)
- return bit.band(self[1], capi.SWIM_EV_NEW) ~= 0
+ local value = swim_check_event(self, 'event:is_new()')
+ return bit.band(value, capi.SWIM_EV_NEW) ~= 0
end,
is_drop = function(self)
- return bit.band(self[1], capi.SWIM_EV_DROP) ~= 0
+ local value = swim_check_event(self, 'event:is_drop()')
+ return bit.band(value, capi.SWIM_EV_DROP) ~= 0
end,
is_update = function(self)
- return bit.band(self[1], capi.SWIM_EV_UPDATE) ~= 0
+ local value = swim_check_event(self, 'event:is_update()')
+ return bit.band(value, capi.SWIM_EV_UPDATE) ~= 0
end,
is_new_status = function(self)
- return bit.band(self[1], capi.SWIM_EV_NEW_STATUS) ~= 0
+ local value = swim_check_event(self, 'event:is_new_status()')
+ return bit.band(value, capi.SWIM_EV_NEW_STATUS) ~= 0
end,
is_new_uri = function(self)
- return bit.band(self[1], capi.SWIM_EV_NEW_URI) ~= 0
+ local value = swim_check_event(self, 'event:is_new_uri()')
+ return bit.band(value, capi.SWIM_EV_NEW_URI) ~= 0
end,
is_new_incarnation = function(self)
- return bit.band(self[1], capi.SWIM_EV_NEW_INCARNATION) ~= 0
+ local value = swim_check_event(self, 'event:is_new_incarnation()')
+ return bit.band(value, capi.SWIM_EV_NEW_INCARNATION) ~= 0
end,
is_new_generation = function(self)
- return bit.band(self[1], capi.SWIM_EV_NEW_GENERATION) ~= 0
+ local value = swim_check_event(self, 'event:is_new_generation()')
+ return bit.band(value, capi.SWIM_EV_NEW_GENERATION) ~= 0
end,
is_new_version = function(self)
- return bit.band(self[1], capi.SWIM_EV_NEW_VERSION) ~= 0
+ local value = swim_check_event(self, 'event:is_new_version()')
+ return bit.band(value, capi.SWIM_EV_NEW_VERSION) ~= 0
end,
is_new_payload = function(self)
- return bit.band(self[1], capi.SWIM_EV_NEW_PAYLOAD) ~= 0
+ local value = swim_check_event(self, 'event:is_new_payload()')
+ return bit.band(value, capi.SWIM_EV_NEW_PAYLOAD) ~= 0
end,
}
diff --git a/test/swim/gh-5952-swim-serialize.result b/test/swim/gh-5952-swim-serialize.result
new file mode 100644
index 000000000..0f9c3250b
--- /dev/null
+++ b/test/swim/gh-5952-swim-serialize.result
@@ -0,0 +1,101 @@
+-- test-run result file version 2
+fiber = require('fiber')
+ | ---
+ | ...
+swim = require('swim')
+ | ---
+ | ...
+test_run = require('test_run').new()
+ | ---
+ | ...
+test_run:cmd("push filter '\\.lua.*:[0-9]+: ' to '.lua:<line>: '")
+ | ---
+ | - true
+ | ...
+test_run:cmd("push filter '127.0.0.1:[0-9]+$' to '127.0.0.1:<port>'")
+ | ---
+ | - true
+ | ...
+
+--
+-- gh-5952: invalid types in __serialize methods could lead to a crash.
+--
+
+s = swim.new({generation = 0})
+ | ---
+ | ...
+getmetatable(s):__serialize()
+ | ---
+ | - error: 'builtin/swim.lua:<line>: swim:__serialize(): first argument is not a SWIM instance'
+ | ...
+getmetatable(s).__serialize(s)
+ | ---
+ | - []
+ | ...
+
+s:cfg({uuid = uuid(1), uri = uri()})
+ | ---
+ | - true
+ | ...
+getmetatable(s):__serialize()
+ | ---
+ | - error: 'builtin/swim.lua:<line>: swim:__serialize(): first argument is not a SWIM instance'
+ | ...
+getmetatable(s).__serialize(s)
+ | ---
+ | - uuid: 00000000-0000-1000-8000-000000000001
+ | uri: 127.0.0.1:<port>
+ | ...
+
+self = s:self()
+ | ---
+ | ...
+getmetatable(self):__serialize()
+ | ---
+ | - error: 'builtin/swim.lua:<line>: member:__serialize(): first argument is not a SWIM
+ | member'
+ | ...
+getmetatable(self).__serialize(self)
+ | ---
+ | - uri: 127.0.0.1:<port>
+ | status: alive
+ | incarnation: cdata {generation = 0ULL, version = 1ULL}
+ | uuid: 00000000-0000-1000-8000-000000000001
+ | payload_size: 0
+ | ...
+
+event = nil
+ | ---
+ | ...
+_ = s:on_member_event(function(m, e) event = e end)
+ | ---
+ | ...
+s:set_payload(1)
+ | ---
+ | - true
+ | ...
+test_run:wait_cond(function() return event ~= nil end)
+ | ---
+ | - true
+ | ...
+
+getmetatable(event):__serialize()
+ | ---
+ | - error: 'builtin/swim.lua:<line>: event:is_update(): first argument is not a SWIM event'
+ | ...
+getmetatable(event).__serialize(event)
+ | ---
+ | - is_new_payload: true
+ | is_new_version: true
+ | is_new_incarnation: true
+ | is_update: true
+ | ...
+
+s:delete()
+ | ---
+ | ...
+
+test_run:cmd("clear filter")
+ | ---
+ | - true
+ | ...
diff --git a/test/swim/gh-5952-swim-serialize.test.lua b/test/swim/gh-5952-swim-serialize.test.lua
new file mode 100644
index 000000000..7bf75ba4f
--- /dev/null
+++ b/test/swim/gh-5952-swim-serialize.test.lua
@@ -0,0 +1,33 @@
+fiber = require('fiber')
+swim = require('swim')
+test_run = require('test_run').new()
+test_run:cmd("push filter '\\.lua.*:[0-9]+: ' to '.lua:<line>: '")
+test_run:cmd("push filter '127.0.0.1:[0-9]+$' to '127.0.0.1:<port>'")
+
+--
+-- gh-5952: invalid types in __serialize methods could lead to a crash.
+--
+
+s = swim.new({generation = 0})
+getmetatable(s):__serialize()
+getmetatable(s).__serialize(s)
+
+s:cfg({uuid = uuid(1), uri = uri()})
+getmetatable(s):__serialize()
+getmetatable(s).__serialize(s)
+
+self = s:self()
+getmetatable(self):__serialize()
+getmetatable(self).__serialize(self)
+
+event = nil
+_ = s:on_member_event(function(m, e) event = e end)
+s:set_payload(1)
+test_run:wait_cond(function() return event ~= nil end)
+
+getmetatable(event):__serialize()
+getmetatable(event).__serialize(event)
+
+s:delete()
+
+test_run:cmd("clear filter")
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 6+ messages in thread