Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lua: <key_def_object>:compare_with_key() may leak a tuple #5388

Closed
Totktonada opened this issue Oct 6, 2020 · 1 comment · Fixed by #5935
Closed

lua: <key_def_object>:compare_with_key() may leak a tuple #5388

Totktonada opened this issue Oct 6, 2020 · 1 comment · Fixed by #5935
Assignees
@Totktonada
Labels
bug Something isn't working lua
Milestone
2.6.3

Comments

@Totktonada
Copy link
Member

Tarantool version: 2.6.0-136-g2711797be.

Case (constructed similarly to one from #5382).

tarantool> key_def = require('key_def')
tarantool> kd = key_def.new({{fieldno = 1, type = 'string'}})
tarantool> large_str = string.rep('x', 2^29)
tarantool> for i = 1, 20 do print(i, pcall(kd.compare_with_key, kd, {large_str}, {function() end})) collectgarbage() end

Code:

tarantool/src/box/lua/key_def.c

Lines 352 to 359 in 2711797

struct tuple *tuple = luaT_key_def_check_tuple(L, key_def, 2);
if (tuple == NULL)
return luaT_error(L);
struct region *region = &fiber()->gc;
size_t region_svp = region_used(region);
size_t key_len;
const char *key_end, *key = lbox_encode_tuple_on_gc(L, 3, &key_len);

The function lbox_encode_tuple_on_gc() raises a Lua error at a serialization failure (for key) and so a tuple created or referenced in luaT_key_def_check_tuple() will leak.
@Totktonada Totktonada added bug Something isn't working lua labels Oct 6, 2020
@Totktonada Totktonada self-assigned this Oct 6, 2020
@Totktonada
Copy link
Member Author

To be fixed by using non-throwing luaT_tuple_encode(), which will be introduced within #5273.

Totktonada added a commit to tarantool/tuple-keydef that referenced this issue Oct 10, 2020
@Totktonada
API update: luaT_tuple_encode() now uses box region
d31a5c9 
Aside of this, added validation of its return value.

Borrowed the test case from fix of the issue [1].

[1]: tarantool/tarantool#5388
Totktonada added a commit to tarantool/tuple-keydef that referenced this issue Oct 19, 2020
@Totktonada
API update: luaT_tuple_encode() now uses box region
7317664 
Aside of this, added validation of its return value.

Borrowed the test case from fix of the issue [1].

[1]: tarantool/tarantool#5388
Totktonada added a commit that referenced this issue Mar 23, 2021
@Totktonada
lua: fix tuple leak in <key_def>.compare_with_key
d710717 
The key difference between lbox_encode_tuple_on_gc() and
luaT_tuple_encode() is that the latter never raises a Lua error, but
passes an error using the diagnostics area.

While I'm here, added a test case that just verifies correct behaviour
in case of a key serialization failure. The case does not verify whether
a tuple leaks and it is successful as before this patch as well after
the patch. I don't find a simple way to check the leak within a test.
Verified manually using the reproducer from the linked issue.

Fixes #5388
@Totktonada Totktonada mentioned this issue Mar 23, 2021
Merged
Totktonada added a commit that referenced this issue Mar 23, 2021
@Totktonada
lua: fix tuple leak in <key_def>.compare_with_key
381ecad 
The key difference between lbox_encode_tuple_on_gc() and
luaT_tuple_encode() is that the latter never raises a Lua error, but
passes an error using the diagnostics area.

While I'm here, added a test case that just verifies correct behaviour
in case of a key serialization failure. The case does not verify whether
a tuple leaks and it is successful as before this patch as well after
the patch. I don't find a simple way to check the leak within a test.
Verified manually using the reproducer from the linked issue.

Fixes #5388
Totktonada added a commit that referenced this issue Mar 29, 2021
@Totktonada
lua: fix tuple leak in <key_def>.compare_with_key
a6028a4 
The key difference between lbox_encode_tuple_on_gc() and
luaT_tuple_encode() is that the latter never raises a Lua error, but
passes an error using the diagnostics area.

Aside of the tuple leak, the patch fixes fiber region's memory 'leak'
(till fiber_gc()). Before the patch, the memory that is used for
serialization of the key is not freed (region_truncate()) when the
serialization fails. It is verified in the gh-5388-<...> test.

While I'm here, added a test case that just verifies correct behaviour
in case of a key serialization failure (added into key_def.test.lua).
The case does not verify whether a tuple leaks and it is successful as
before this patch as well after the patch. I don't find a simple way to
check the tuple leak within a test. Verified manually using the
reproducer from the linked issue.

Fixes #5388
Totktonada added a commit that referenced this issue Apr 1, 2021
@Totktonada
lua: fix tuple leak in <key_def>.compare_with_key
3d3ebfc 
The key difference between lbox_encode_tuple_on_gc() and
luaT_tuple_encode() is that the latter never raises a Lua error, but
passes an error using the diagnostics area.

Aside of the tuple leak, the patch fixes fiber region's memory 'leak'
(till fiber_gc()). Before the patch, the memory that is used for
serialization of the key is not freed (region_truncate()) when the
serialization fails. It is verified in the gh-5388-<...> test.

While I'm here, added a test case that just verifies correct behaviour
in case of a key serialization failure (added into key_def.test.lua).
The case does not verify whether a tuple leaks and it is successful as
before this patch as well after the patch. I don't find a simple way to
check the tuple leak within a test. Verified manually using the
reproducer from the linked issue.

Fixes #5388
@kyukhin kyukhin added this to the 1.10.10 milestone Apr 5, 2021
@kyukhin kyukhin added the teamE label Apr 5, 2021
kyukhin pushed a commit that referenced this issue Apr 5, 2021
@Totktonada @kyukhin
lua: fix tuple leak in <key_def>.compare_with_key
db766c5 
The key difference between lbox_encode_tuple_on_gc() and
luaT_tuple_encode() is that the latter never raises a Lua error, but
passes an error using the diagnostics area.

Aside of the tuple leak, the patch fixes fiber region's memory 'leak'
(till fiber_gc()). Before the patch, the memory that is used for
serialization of the key is not freed (region_truncate()) when the
serialization fails. It is verified in the gh-5388-<...> test.

While I'm here, added a test case that just verifies correct behaviour
in case of a key serialization failure (added into key_def.test.lua).
The case does not verify whether a tuple leaks and it is successful as
before this patch as well after the patch. I don't find a simple way to
check the tuple leak within a test. Verified manually using the
reproducer from the linked issue.

Fixes #5388
kyukhin pushed a commit that referenced this issue Apr 5, 2021
@Totktonada @kyukhin
lua: fix tuple leak in <key_def>.compare_with_key
d5de598 
The key difference between lbox_encode_tuple_on_gc() and
luaT_tuple_encode() is that the latter never raises a Lua error, but
passes an error using the diagnostics area.

Aside of the tuple leak, the patch fixes fiber region's memory 'leak'
(till fiber_gc()). Before the patch, the memory that is used for
serialization of the key is not freed (region_truncate()) when the
serialization fails. It is verified in the gh-5388-<...> test.

While I'm here, added a test case that just verifies correct behaviour
in case of a key serialization failure (added into key_def.test.lua).
The case does not verify whether a tuple leaks and it is successful as
before this patch as well after the patch. I don't find a simple way to
check the tuple leak within a test. Verified manually using the
reproducer from the linked issue.

Fixes #5388

(cherry picked from commit db766c5)
kyukhin pushed a commit that referenced this issue Apr 5, 2021
@Totktonada @kyukhin
lua: fix tuple leak in <key_def>.compare_with_key
2f03c1a 
The key difference between lbox_encode_tuple_on_gc() and
luaT_tuple_encode() is that the latter never raises a Lua error, but
passes an error using the diagnostics area.

Aside of the tuple leak, the patch fixes fiber region's memory 'leak'
(till fiber_gc()). Before the patch, the memory that is used for
serialization of the key is not freed (region_truncate()) when the
serialization fails. It is verified in the gh-5388-<...> test.

While I'm here, added a test case that just verifies correct behaviour
in case of a key serialization failure (added into key_def.test.lua).
The case does not verify whether a tuple leaks and it is successful as
before this patch as well after the patch. I don't find a simple way to
check the tuple leak within a test. Verified manually using the
reproducer from the linked issue.

Fixes #5388

(cherry picked from commit db766c5)
@kyukhin kyukhin modified the milestones: 1.10.10, 2.6.3 Apr 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Totktonada Totktonada

Labels
bug Something isn't working lua
Projects
None yet
Milestone
2.6.3
Development

Successfully merging a pull request may close this issue.

lua: fix tuple leak in <key_def>.compare_with_key tarantool/tarantool
2 participants
@Totktonada @kyukhin