lua/msgpack: fix crash while encoding error extension
Whether errors are encoded as a msgpack extension or not is determined by the serializer_opts::error_marshaling_enabled flag. Although an instance of serizlier_opts is passed to luamp_encode(), it doesn't propagate it to luamp_encode_extension_box(). The latter encodes an error as a msgpack extension if the error_marshaling_enabled flag is set in serializer_opts of the current session. This leads to a bug when luamp_encode() is called with error_marshaling_enabled unset while the current session has the flag set: 1. luaL_tofield() sets field->type to MP_EXT and field->ext_type to MP_UNKNOWN_EXTENSION, because the error_marshaling_enabled flag is unset: https://github.com/tarantool/tarantool/blob/b0431cf8f47e9d081f6a402bc18edb1d6ad49847/src/lua/serializer.c#L548 2. Basing on the ext_type, luamp_encode_r() skips the MP_ERROR swtich-case branch for the default branch and calls the luamp_encode_extension callback: https://github.com/tarantool/tarantool/blob/b0431cf8f47e9d081f6a402bc18edb1d6ad49847/src/lua/msgpack.c#L203 3. The callback implementation (luamp_encode_extension_box()) encodes the error, because the error_marshaling_enabled flag is set in the current session settings, and returns MP_EXT: https://github.com/tarantool/tarantool/blob/b0431cf8f47e9d081f6a402bc18edb1d6ad49847/src/box/lua/init.c#L420 4. luamp_encode_r() assumes that the callback didn't encode the extension, because it returned MP_EXT, and encodes it again as a string: https://github.com/tarantool/tarantool/blob/b0431cf8f47e9d081f6a402bc18edb1d6ad49847/src/lua/msgpack.c#L209 This results in a broken msgpack content. To fix this bug, let's do the following: - luaL_tofield() now sets ext_type to MP_ERROR unconditionally, irrespective of serializer_opts::error_marshaling_enabled. - luamp_encode_r() invokes the luamp_encode_extension callback for a MP_ERROR field only if error_marshaling_enabled is set. If the flag is unset, it proceeds with converting the field to string. - luamp_encode_extension_box() doesn't check serializer_opts anymore. It doesn't need to, because it's called iff error_marshaling_enabled is set. - YAML and JSON encoders are patched to handle the MP_ERROR field type by appending error::errmsg to the output (they use luaL_tofield() internally to determine the field type so they have to handle MP_ERROR). This basically disables error encoding as msgpack extension everywhere except returning an error from a Lua CALL/EVAL, in particular: - when creating a tuple with box.tuple.new(), - when inserting an error into a space, - when encoding an error with the msgpack module. This is okay, because the functionality has always been broken anyway. We will introduce a separate msgpack encoder option to enable encoding errors as MP_ERROR msgpack extension. Looking at the code links above, one is likely to wonder why error encoding was implemented via the encode extension callback in the first place. The lua/msgpack module knows about the MP_ERROR extension and even partially handles it so it'd be only natural to call the error encoder function directly, as we do with decimals and uuids. Unfortunately, we can't do it, because the error encoder is (surprise!) a part of the box library. I filed a ticket to move it to the core lib, see #6432. Closes #6431
Showing
- changelogs/unreleased/gh-6431-fix-crash-while-encoding-error.md 3 additions, 0 deletions...logs/unreleased/gh-6431-fix-crash-while-encoding-error.md
- src/box/lua/init.c 1 addition, 3 deletionssrc/box/lua/init.c
- src/lua/msgpack.c 6 additions, 0 deletionssrc/lua/msgpack.c
- src/lua/serializer.c 4 additions, 4 deletionssrc/lua/serializer.c
- src/lua/serializer.h 1 addition, 0 deletionssrc/lua/serializer.h
- test/box/gh-6431-crash-while-encoding-error.result 57 additions, 0 deletionstest/box/gh-6431-crash-while-encoding-error.result
- test/box/gh-6431-crash-while-encoding-error.test.lua 16 additions, 0 deletionstest/box/gh-6431-crash-while-encoding-error.test.lua
- third_party/lua-cjson/lua_cjson.c 6 additions, 0 deletionsthird_party/lua-cjson/lua_cjson.c
- third_party/lua-yaml/lyaml.cc 5 additions, 0 deletionsthird_party/lua-yaml/lyaml.cc
Loading
Please register or sign in to comment