Skip to content
Snippets Groups Projects
Commit ed2d260f authored by Alexander Turenko's avatar Alexander Turenko Committed by Alexander Turenko
Browse files

merger: fix signed integer overflow

The `merger.new()` call has the following code in the
`luaT_merger_new_parse_sources()` function:

 | uint32_t source_count = lua_objlen(L, idx);
 | for (uint32_t i = 0; i < source_count; ++i) {
 |     <...>
 | }
 | lua_pop(L, source_count);

It is possible that zero amount of sources are passed:

 | merger.new(kd, {})

In this case the `source_count` variable is zero.

`lua_pop()` is a macro defined this way:

 | #define lua_pop(L,n)		lua_settop(L, -(n)-1)

It means that `n` in the `-(n)-1` expression is an unsigned 32 bit zero.
Unsigned overflow is okay: it has defined behavior by the C standard and
has the result 2^32-1 in the given case.

The `lua_settop()` function is defined as follows:

 | LUA_API void  (lua_settop) (lua_State *L, int idx);

We pass the `-(n)-1` value as `int idx` argument to `lua_settop()`. The
value has uint32_t type and it is out of the `int` range ([-2^31,
2^31]). Casting it to `int` has implementation defined behavior
according to the standard (n1256,
6.3.1.3.3).

In practice, we're building Tarantool only for architectures with two's
complement integers. The result of the cast is -1 and everything works
as expected: the stack top remains unchanged.

However, it is easy to eliminate the signed integer overflow, so it is
worthful to do. We can just save the stack top value and use
`lua_settop()` to restore it, which is quite common idiom.

The problem can be found by clang's undefined behavior sanitizer.

Apply the following patch:

NO_WRAP
 | --- a/cmake/compiler.cmake
 | +++ b/cmake/compiler.cmake
 | @@ -238,6 +238,7 @@ macro(enable_tnt_compile_flags)
 |                  alignment bool bounds builtin enum float-cast-overflow
 |                  float-divide-by-zero function integer-divide-by-zero return
 |                  shift unreachable vla-bound
 | +                implicit-integer-sign-change
 |              )
 |
 |              # Exclude "object-size".
 | @@ -272,7 +273,7 @@ macro(enable_tnt_compile_flags)
 |              # the typeof(*obj) when obj is NULL, even though there is nothing
 |              # related to return.
 |
 | -            set(SANITIZE_FLAGS "-fsanitize=${SANITIZE_FLAGS} -fno-sanitize-recover=${SANITIZE_FLAGS}")
 | +            set(SANITIZE_FLAGS "-fsanitize=${SANITIZE_FLAGS}")
 |
 |              add_compile_flags("C;CXX" "${SANITIZE_FLAGS}")
 |          endif()
NO_WRAP

Build Tarantool with the sanitizer:

 | CC=clang-15 CXX=clang++-15 cmake . \
 |     -DCMAKE_BUILD_TYPE=Debug       \
 |     -DENABLE_BACKTRACE=ON          \
 |     -DENABLE_DIST=ON               \
 |     -DENABLE_FEEDBACK_DAEMON=OFF   \
 |     -DENABLE_BUNDLED_LIBCURL=OFF   \
 |     -DENABLE_BUNDLED_LIBUNWIND=OFF \
 |     -DENABLE_UB_SANITIZER=ON && make -j

Run the interactive console and create a merger with zero sources:

 | tarantool> key_def = require('key_def')
 | tarantool> merger = require('merger')
 | tarantool> kd = key_def.new({{field = 1, type = 'number'}})
 | tarantool> m = merger.new(kd, {})

Observe the 2^32-1 cast to 32 bit signed integer:

 | <...>/src/box/lua/merger.c:334:2: runtime error: implicit conversion
 |     from type 'unsigned int' of value 4294967295 (32-bit, unsigned)
 |     to type 'int' changed the value to -1 (32-bit, signed)
 | SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
 |     <...>/src/box/lua/merger.c:334:2 in

The commit eliminates this report from the clang's sanitizer.

I've added a test case, which goes over the relevant code path. It
succeeds as before the commit as well as after it. If we'll enable a
relevant dynamic analysis in a future (such as clang's
`-fsanitize=implicit-integer-sign-change`), the test case may reveal
problems on the given code path.

Reported-in: https://github.com/tarantool/security/issues/103

NO_DOC=no user-visible behavior changes
NO_CHANGELOG=no user-visible behavior changes
parent 921a0717
No related branches found
No related tags found
No related merge requests found
......@@ -317,6 +317,7 @@ luaT_merger_new_parse_sources(struct lua_State *L, int idx,
}
/* Save all sources. */
int top = lua_gettop(L);
for (uint32_t i = 0; i < source_count; ++i) {
lua_pushinteger(L, i + 1);
lua_gettable(L, idx);
......@@ -331,7 +332,7 @@ luaT_merger_new_parse_sources(struct lua_State *L, int idx,
}
sources[i] = source;
}
lua_pop(L, source_count);
lua_settop(L, top);
*source_count_ptr = source_count;
return sources;
......
......@@ -550,7 +550,7 @@ end
local test = tap.test('merger')
test:plan(#bad_source_new_calls + #bad_chunks + #bad_merger_new_calls +
#bad_merger_select_calls + 6 + #schemas * 48)
#bad_merger_select_calls + 7 + #schemas * 48)
-- For collations.
box.cfg{}
......@@ -765,4 +765,13 @@ for _, input_type in ipairs({'buffer', 'table', 'tuple'}) do
end
end
-- merger.new(kd, {}) -- pass zero amount of sources.
test:test('no sources', function(test)
test:plan(1)
local m = merger.new(key_def, {})
local res = m:select()
test:is_deeply(res, {})
end)
os.exit(test:check() and 0 or 1)
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment