[Tarantool-patches] [PATCH 0/2] Swim misusage crashes

[Tarantool-patches] [PATCH 0/2] Swim misusage crashes

[Vladislav Shpilevoy via Tarantool-patches]

The patchset fixes 2 crashes happened when SWIM API was misused.

Branch: http://github.com/tarantool/tarantool/tree/gerold103/swim-misusage-crashes-5951-5952
Issue: https://github.com/tarantool/tarantool/issues/5951
Issue: https://github.com/tarantool/tarantool/issues/5952

Vladislav Shpilevoy (2):
  swim: fix crash on bad member_by_uuid() call
  swim: check types in __serialize methods

 .../unreleased/swim-member_by_uuid-crash.md   |   4 +
 changelogs/unreleased/swim-serialize-crash.md |   4 +
 src/lib/swim/swim.c                           |   2 +
 src/lua/swim.lua                              |  41 +++++--
 test/swim/gh-5952-swim-serialize.result       | 101 ++++++++++++++++++
 test/swim/gh-5952-swim-serialize.test.lua     |  33 ++++++
 test/swim/swim.result                         |   9 ++
 test/swim/swim.test.lua                       |   3 +
 test/unit/swim.c                              |  23 +++-
 test/unit/swim.result                         |   9 +-
 10 files changed, 217 insertions(+), 12 deletions(-)
 create mode 100644 changelogs/unreleased/swim-member_by_uuid-crash.md
 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

-- 
2.24.3 (Apple Git-128)

[Tarantool-patches] [PATCH 1/2] swim: fix crash on bad member_by_uuid() call

[Vladislav Shpilevoy via Tarantool-patches]

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)

[Tarantool-patches] [PATCH 2/2] swim: check types in __serialize methods

[Vladislav Shpilevoy via Tarantool-patches]

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)

Re: [Tarantool-patches] [PATCH 0/2] Swim misusage crashes

[Cyrill Gorcunov via Tarantool-patches]

On Sun, Apr 04, 2021 at 05:06:41PM +0200, Vladislav Shpilevoy wrote:
> The patchset fixes 2 crashes happened when SWIM API was misused.
> 

Can't comment on architectural details since I'm not familiar with
swim internals, but from code view patch looks reasonable. Thanks!

Re: [Tarantool-patches] [PATCH 0/2] Swim misusage crashes

[Serge Petrenko via Tarantool-patches]

04.04.2021 18:06, Vladislav Shpilevoy пишет:
> The patchset fixes 2 crashes happened when SWIM API was misused.
>
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/swim-misusage-crashes-5951-5952
> Issue: https://github.com/tarantool/tarantool/issues/5951
> Issue: https://github.com/tarantool/tarantool/issues/5952

Hi! Thanks for the patches! LGTM.

> Vladislav Shpilevoy (2):
>    swim: fix crash on bad member_by_uuid() call
>    swim: check types in __serialize methods
>
>   .../unreleased/swim-member_by_uuid-crash.md   |   4 +
>   changelogs/unreleased/swim-serialize-crash.md |   4 +
>   src/lib/swim/swim.c                           |   2 +
>   src/lua/swim.lua                              |  41 +++++--
>   test/swim/gh-5952-swim-serialize.result       | 101 ++++++++++++++++++
>   test/swim/gh-5952-swim-serialize.test.lua     |  33 ++++++
>   test/swim/swim.result                         |   9 ++
>   test/swim/swim.test.lua                       |   3 +
>   test/unit/swim.c                              |  23 +++-
>   test/unit/swim.result                         |   9 +-
>   10 files changed, 217 insertions(+), 12 deletions(-)
>   create mode 100644 changelogs/unreleased/swim-member_by_uuid-crash.md
>   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
>

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 0/2] Swim misusage crashes

[Kirill Yukhin via Tarantool-patches]

Hello,

On 04 апр 17:06, Vladislav Shpilevoy via Tarantool-patches wrote:
> The patchset fixes 2 crashes happened when SWIM API was misused.
> 
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/swim-misusage-crashes-5951-5952
> Issue: https://github.com/tarantool/tarantool/issues/5951
> Issue: https://github.com/tarantool/tarantool/issues/5952
> 
> Vladislav Shpilevoy (2):
>   swim: fix crash on bad member_by_uuid() call
>   swim: check types in __serialize methods

I've checked your patchset into 2.6, 2.7 and master.

--
Regards, Kirill Yukhin