[Tarantool-patches] [PATCH small 1/1] lsregion: fix slab_unmap() called on malloced slab

[Tarantool-patches] [PATCH small 1/1] lsregion: fix slab_unmap() called on malloced slab

[Vladislav Shpilevoy]

Lsregion allocates slabs using either
- Slab_map() from slab arena, when allocation size is smaller,
  than slab size;
- Using cached slab, stored in the lsregion as a protection from
  oscillation;
- Using malloc(), when requested size is too big.

Malloc() was used when allocation size was >= fixed slab size -
meta size. However free() was used, when real slab size was >
fixed slab size - meta size. So if an allocation was exactly of
size 'fixed slab size - meta size', it was allocated using
malloc(), but freed using slab_unmap(). That lead to a crash, if
'lucky'. But as it is a memory corruption, could lead to anything.
---
Branch: http://github.com/tarantool/small/tree/gerold103/fix-lsregion-crash-or-leak

This led to at least leaks in vinyl. Since it used lsregion very
extensively for 0 level of LSM trees.

 small/lsregion.c | 2 +-
 test/lsregion.c  | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/small/lsregion.c b/small/lsregion.c
index b7f2684..6bcc043 100644
--- a/small/lsregion.c
+++ b/small/lsregion.c
@@ -45,7 +45,7 @@ lsregion_alloc_slow(struct lsregion *lsregion, size_t size, int64_t id)
 	}
 	if ((slab != NULL && size > lslab_unused(slab)) ||
 	    slab == NULL) {
-		if (size + lslab_sizeof() >= slab_size) {
+		if (size + lslab_sizeof() > slab_size) {
 			/* Large allocation, use malloc() */
 			slab_size = size + lslab_sizeof();
 			struct quota *quota = lsregion->arena->quota;
diff --git a/test/lsregion.c b/test/lsregion.c
index 1ed6a3a..90ad060 100644
--- a/test/lsregion.c
+++ b/test/lsregion.c
@@ -140,6 +140,13 @@ test_basic()
 	is(lsregion_slab_count(&allocator), 0,
 	   "slab count after large gc()");
 
+	/*
+	 * Allocate exactly slab size.
+	 */
+	++id;
+	data = lsregion_alloc(&allocator, arena.slab_size - lslab_sizeof(), id);
+	lsregion_gc(&allocator, id);
+
 	lsregion_destroy(&allocator);
 	/* Sic: slabs are cached by arena */
 	is(arena.used, 2 * arena.slab_size, "arena used after destroy");
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH small 1/1] lsregion: fix slab_unmap() called on malloced slab

[Aleksandr Lyapunov]

Thank for the patch! nice catch, lgtm!

On 5/15/20 1:31 AM, Vladislav Shpilevoy wrote:
> Lsregion allocates slabs using either
> - Slab_map() from slab arena, when allocation size is smaller,
>    than slab size;
> - Using cached slab, stored in the lsregion as a protection from
>    oscillation;
> - Using malloc(), when requested size is too big.
>
> Malloc() was used when allocation size was >= fixed slab size -
> meta size. However free() was used, when real slab size was >
> fixed slab size - meta size. So if an allocation was exactly of
> size 'fixed slab size - meta size', it was allocated using
> malloc(), but freed using slab_unmap(). That lead to a crash, if
> 'lucky'. But as it is a memory corruption, could lead to anything.
> ---
> Branch: http://github.com/tarantool/small/tree/gerold103/fix-lsregion-crash-or-leak
>
> This led to at least leaks in vinyl. Since it used lsregion very
> extensively for 0 level of LSM trees.

Re: [Tarantool-patches] [PATCH small 1/1] lsregion: fix slab_unmap() called on malloced slab

[Konstantin Osipov]

* Aleksandr Lyapunov <alyapunov@tarantool.org> [20/05/19 19:24]:
> Thank for the patch! nice catch, lgtm!

Is this cherry-picked to 1.10?

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH small 1/1] lsregion: fix slab_unmap() called on malloced slab

[Vladislav Shpilevoy]

It is not pushed yet.

On 23/05/2020 20:13, Konstantin Osipov wrote:
> * Aleksandr Lyapunov <alyapunov@tarantool.org> [20/05/19 19:24]:
>> Thank for the patch! nice catch, lgtm!
> 
> Is this cherry-picked to 1.10?