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

Dirty memory access in cases of passing FP values to integer iterator #4558

Closed
kshcherbatov opened this issue Oct 9, 2019 · 1 comment
Closed

Dirty memory access in cases of passing FP values to integer iterator #4558

kshcherbatov opened this issue Oct 9, 2019 · 1 comment
Assignees
@Korablev77
Labels
bug Something isn't working sql

Comments

@kshcherbatov
Copy link
Contributor

Tarantool version: 2.2.0, ASAN=ON Build

Code that produces such error:

box.execute("CREATE TABLE tt1 (id INT PRIMARY KEY);")
box.execute("CREATE TABLE tt2 (id INT PRIMARY KEY);")
box.execute("CREATE TRIGGER tr1 AFTER DELETE ON tt1 FOR EACH ROW BEGIN DELETE FROM tt2; END;")
box.execute("INSERT INTO tt1 VALUES (1), (2), (3);")
box.execute("INSERT INTO tt2 VALUES (1), (2), (3);")
box.execute("DELETE FROM tt1 WHERE id = 2;")

problem:
box/sql/wherecode.c,
sqlWhereCodeOneLoopStart

enum field_type ineq_type = idx_def->key_def->parts[nEq].type;
// nEq == 1 and is bigger than  idx_def->key_def->part_count == 1
if (pRangeStart != NULL && (ineq_type == FIELD_TYPE_INTEGER ||
			    ineq_type == FIELD_TYPE_UNSIGNED))
	force_integer_reg = regBase + nEq;
emit_apply_type(pParse, regBase, nConstraint - bSeekPastNull,
		start_types);

tarantool> box.execute("DELETE FROM tt1 WHERE id = 2;")
=================================================================
==6656==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61000003e4fc at pc 0x0000013463e7 bp 0x7fffef178f10 sp 0x7fffef178f08
READ of size 4 at 0x61000003e4fc thread T0
    #0 0x13463e6 in sqlWhereCodeOneLoopStart /home/kir/WORK/tarantool/src/box/sql/wherecode.c:1121:60
    #1 0x131f1c9 in sqlWhereBegin /home/kir/WORK/tarantool/src/box/sql/where.c:4642:14
    #2 0x11bcac8 in sql_table_delete_from /home/kir/WORK/tarantool/src/box/sql/delete.c:275:7
    #3 0x136517f in yy_reduce /home/kir/WORK/tarantool/src/box/sql/parse.y:807:3
    #4 0x135bc68 in sqlParser /home/kir/WORK/tarantool/src/box/sql/parse.c:4350:7
    #5 0x127cc9c in sqlRunParser /home/kir/WORK/tarantool/src/box/sql/tokenize.c:509:4
    #6 0x12200fd in sqlPrepare /home/kir/WORK/tarantool/src/box/sql/prepare.c:91:4
    #7 0x1220e3e in sql_prepare_v2 /home/kir/WORK/tarantool/src/box/sql/prepare.c:235:11
    #8 0x872d26 in sql_prepare_and_execute /home/kir/WORK/tarantool/src/box/execute.c:446:6
    #9 0x8bdcd6 in lbox_execute /home/kir/WORK/tarantool/src/box/lua/execute.c:261:6
    #10 0x9f869a in lj_BC_FUNCC (/home/kir/WORK/tarantool/src/tarantool+0x9f869a)
    #11 0xa86edc in lua_pcall /home/kir/WORK/tarantool/third_party/luajit/src/lj_api.c:1139:12
    #12 0x8f3ed2 in luaT_call /home/kir/WORK/tarantool/src/lua/utils.c:1036:6
    #13 0x8de5e3 in lua_main /home/kir/WORK/tarantool/src/lua/init.c:544:11
    #14 0x8de0d1 in run_script_f /home/kir/WORK/tarantool/src/lua/init.c:643:7
    #15 0x52e719 in fiber_cxx_invoke(int (*)(__va_list_tag*), __va_list_tag*) /home/kir/WORK/tarantool/src/lib/core/fiber.h:672:10
    #16 0x9385f2 in fiber_loop /home/kir/WORK/tarantool/src/lib/core/fiber.c:694:18
    #17 0x11897e1 in coro_init /home/kir/WORK/tarantool/third_party/coro/coro.c:110:3

0x61000003e4fc is located 4 bytes to the right of 184-byte region [0x61000003e440,0x61000003e4f8)
allocated by thread T0 here:
    #0 0x4ee7a8 in __interceptor_malloc (/home/kir/WORK/tarantool/src/tarantool+0x4ee7a8)
    #1 0xe9ae51 in key_def_dup /home/kir/WORK/tarantool/src/box/key_def.c:134:24
    #2 0x55b45b in index_def_dup /home/kir/WORK/tarantool/src/box/index_def.c:138:17
    #3 0x556743 in index_create /home/kir/WORK/tarantool/src/box/index.cc:484:8
    #4 0x56a13e in memtx_tree_index_new /home/kir/WORK/tarantool/src/box/memtx_tree.c:1418:6
    #5 0x5c5274 in memtx_space_create_index /home/kir/WORK/tarantool/src/box/memtx_space.c:777:10
    #6 0x753141 in space_create_index /home/kir/WORK/tarantool/src/box/space.h:406:9
    #7 0x752151 in space_create /home/kir/WORK/tarantool/src/box/space.c:168:25
    #8 0x5be47d in memtx_space_new /home/kir/WORK/tarantool/src/box/memtx_space.c:1190:6
    #9 0x5b498c in memtx_engine_create_space /home/kir/WORK/tarantool/src/box/memtx_engine.c:327:9
    #10 0x7535d9 in engine_create_space /home/kir/WORK/tarantool/src/box/engine.h:263:9
    #11 0x7533a3 in space_new /home/kir/WORK/tarantool/src/box/space.c:230:9
    #12 0x78cfcc in space_new_xc(space_def*, rlist*) /home/kir/WORK/tarantool/src/box/space.h:544:24
    #13 0x78e9c4 in alter_space_do(txn_stmt*, alter_space*) /home/kir/WORK/tarantool/src/box/alter.cc:985:21
    #14 0x79930e in on_replace_dd_index(lua_trigger*, void*) /home/kir/WORK/tarantool/src/box/alter.cc:2363:2
    #15 0x99296f in trigger_run /home/kir/WORK/tarantool/src/lib/core/trigger.cc:41:4
    #16 0x7e1c64 in txn_commit_stmt /home/kir/WORK/tarantool/src/box/txn.c:349:9
    #17 0x7eb07a in box_process_rw /home/kir/WORK/tarantool/src/box/box.cc:201:6
    #18 0x860dec in insertOrReplace /home/kir/WORK/tarantool/src/box/sql.c:411:9
    #19 0x8609c9 in tarantoolsqlInsert /home/kir/WORK/tarantool/src/box/sql.c:417:9
    #20 0x12c341c in sqlVdbeExec /home/kir/WORK/tarantool/src/box/sql/vdbe.c:4473:6
    #21 0x12d674d in sqlStep /home/kir/WORK/tarantool/src/box/sql/vdbeapi.c:449:8
    #22 0x12d6069 in sql_step /home/kir/WORK/tarantool/src/box/sql/vdbeapi.c:477:9
    #23 0x87316c in sql_execute /home/kir/WORK/tarantool/src/box/execute.c:431:8
    #24 0x872e57 in sql_prepare_and_execute /home/kir/WORK/tarantool/src/box/execute.c:451:6
    #25 0x8bdcd6 in lbox_execute /home/kir/WORK/tarantool/src/box/lua/execute.c:261:6
    #2 0x11bcac8 in sql_table_delete_from /home/kir/WORK/tarantool/src/box/sql/delete.c:275:7                                                                           [45/4657]
    #3 0x136517f in yy_reduce /home/kir/WORK/tarantool/src/box/sql/parse.y:807:3
    #4 0x135bc68 in sqlParser /home/kir/WORK/tarantool/src/box/sql/parse.c:4350:7
    #5 0x127cc9c in sqlRunParser /home/kir/WORK/tarantool/src/box/sql/tokenize.c:509:4
    #6 0x12200fd in sqlPrepare /home/kir/WORK/tarantool/src/box/sql/prepare.c:91:4
    #7 0x1220e3e in sql_prepare_v2 /home/kir/WORK/tarantool/src/box/sql/prepare.c:235:11
    #8 0x872d26 in sql_prepare_and_execute /home/kir/WORK/tarantool/src/box/execute.c:446:6
    #9 0x8bdcd6 in lbox_execute /home/kir/WORK/tarantool/src/box/lua/execute.c:261:6
    #10 0x9f869a in lj_BC_FUNCC (/home/kir/WORK/tarantool/src/tarantool+0x9f869a)
    #11 0xa86edc in lua_pcall /home/kir/WORK/tarantool/third_party/luajit/src/lj_api.c:1139:12
    #12 0x8f3ed2 in luaT_call /home/kir/WORK/tarantool/src/lua/utils.c:1036:6
    #13 0x8de5e3 in lua_main /home/kir/WORK/tarantool/src/lua/init.c:544:11
    #14 0x8de0d1 in run_script_f /home/kir/WORK/tarantool/src/lua/init.c:643:7
    #15 0x52e719 in fiber_cxx_invoke(int (*)(__va_list_tag*), __va_list_tag*) /home/kir/WORK/tarantool/src/lib/core/fiber.h:672:10
    #16 0x9385f2 in fiber_loop /home/kir/WORK/tarantool/src/lib/core/fiber.c:694:18
    #17 0x11897e1 in coro_init /home/kir/WORK/tarantool/third_party/coro/coro.c:110:3

0x61000003e4fc is located 4 bytes to the right of 184-byte region [0x61000003e440,0x61000003e4f8)
allocated by thread T0 here:
    #0 0x4ee7a8 in __interceptor_malloc (/home/kir/WORK/tarantool/src/tarantool+0x4ee7a8)
    #1 0xe9ae51 in key_def_dup /home/kir/WORK/tarantool/src/box/key_def.c:134:24
    #2 0x55b45b in index_def_dup /home/kir/WORK/tarantool/src/box/index_def.c:138:17
    #3 0x556743 in index_create /home/kir/WORK/tarantool/src/box/index.cc:484:8
    #4 0x56a13e in memtx_tree_index_new /home/kir/WORK/tarantool/src/box/memtx_tree.c:1418:6
    #5 0x5c5274 in memtx_space_create_index /home/kir/WORK/tarantool/src/box/memtx_space.c:777:10
    #6 0x753141 in space_create_index /home/kir/WORK/tarantool/src/box/space.h:406:9
    #7 0x752151 in space_create /home/kir/WORK/tarantool/src/box/space.c:168:25
    #8 0x5be47d in memtx_space_new /home/kir/WORK/tarantool/src/box/memtx_space.c:1190:6
    #9 0x5b498c in memtx_engine_create_space /home/kir/WORK/tarantool/src/box/memtx_engine.c:327:9
    #10 0x7535d9 in engine_create_space /home/kir/WORK/tarantool/src/box/engine.h:263:9
    #11 0x7533a3 in space_new /home/kir/WORK/tarantool/src/box/space.c:230:9
    #12 0x78cfcc in space_new_xc(space_def*, rlist*) /home/kir/WORK/tarantool/src/box/space.h:544:24
    #13 0x78e9c4 in alter_space_do(txn_stmt*, alter_space*) /home/kir/WORK/tarantool/src/box/alter.cc:985:21
    #14 0x79930e in on_replace_dd_index(lua_trigger*, void*) /home/kir/WORK/tarantool/src/box/alter.cc:2363:2
    #15 0x99296f in trigger_run /home/kir/WORK/tarantool/src/lib/core/trigger.cc:41:4
    #16 0x7e1c64 in txn_commit_stmt /home/kir/WORK/tarantool/src/box/txn.c:349:9
    #17 0x7eb07a in box_process_rw /home/kir/WORK/tarantool/src/box/box.cc:201:6
    #18 0x860dec in insertOrReplace /home/kir/WORK/tarantool/src/box/sql.c:411:9
    #19 0x8609c9 in tarantoolsqlInsert /home/kir/WORK/tarantool/src/box/sql.c:417:9
    #20 0x12c341c in sqlVdbeExec /home/kir/WORK/tarantool/src/box/sql/vdbe.c:4473:6
    #21 0x12d674d in sqlStep /home/kir/WORK/tarantool/src/box/sql/vdbeapi.c:449:8
    #22 0x12d6069 in sql_step /home/kir/WORK/tarantool/src/box/sql/vdbeapi.c:477:9
    #23 0x87316c in sql_execute /home/kir/WORK/tarantool/src/box/execute.c:431:8
    #24 0x872e57 in sql_prepare_and_execute /home/kir/WORK/tarantool/src/box/execute.c:451:6
    #25 0x8bdcd6 in lbox_execute /home/kir/WORK/tarantool/src/box/lua/execute.c:261:6
    #26 0x9f869a in lj_BC_FUNCC (/home/kir/WORK/tarantool/src/tarantool+0x9f869a)

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/kir/WORK/tarantool/src/box/sql/wherecode.c:1121:60 in sqlWhereCodeOneLoopStart
Shadow bytes around the buggy address:
  0x0c207ffffc40: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c207ffffc50: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa
  0x0c207ffffc60: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c207ffffc70: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa
  0x0c207ffffc80: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
=>0x0c207ffffc90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00[fa]
  0x0c207ffffca0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c207ffffcb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fa
  0x0c207ffffcc0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c207ffffcd0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa
  0x0c207ffffce0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==6656==ABORTING
@kshcherbatov kshcherbatov added bug Something isn't working sql labels Oct 9, 2019
@kshcherbatov
Copy link
Contributor Author

The problem was introduced with 8fac697

@Korablev77 , consider the following diff, please

diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index de51c0ba5..05d6d1c6d 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -1118,7 +1118,9 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,      /* Complete information about the W
                        }
                }
                /* Inequality constraint comes always at the end of list. */
-               enum field_type ineq_type = idx_def->key_def->parts[nEq].type;
+               part_count = idx_def->key_def->part_count;
+               enum field_type ineq_type =
+                       idx_def->key_def->parts[MIN(part_count - 1, nEq)].type;
                if (pRangeStart != NULL && (ineq_type == FIELD_TYPE_INTEGER ||
                                            ineq_type == FIELD_TYPE_UNSIGNED))
                        force_integer_reg = regBase + nEq;

@Korablev77 Korablev77 self-assigned this Oct 14, 2019
Korablev77 added a commit that referenced this issue Oct 21, 2019
@Korablev77
sql: fix off-by-one error in QP
88803b0 
In scope of 8fac697 it was fixed misbehavior while passing floating
point values to integer iterator. Unfortunately, that patch introduced
off-by-one error. Number of constraints (equalities and inequalities)
can not be greater than number of key parts (obviously, each constraint
can be tested against at most one index part). Inequality constraint can
involve only field representing last key part. To get it number of
constraints was used as index. However, since array is 0-based, the last
key part should be calculated as parts[eq_numb - 1]. Before this patch
it was parts[eq_numb].

Closes #4558
Korablev77 added a commit that referenced this issue Dec 30, 2019
@Korablev77
sql: fix off-by-one error in QP
feca08c 
In scope of 8fac697 it was fixed misbehavior while passing floating
point values to integer iterator. Unfortunately, that patch introduced
off-by-one error. Number of constraints (equalities and inequalities)
can not be greater than number of key parts (obviously, each constraint
can be tested against at most one index part). Inequality constraint can
involve only field representing last key part. To get it number of
constraints was used as index. However, since array is 0-based, the last
key part should be calculated as parts[eq_numb - 1]. Before this patch
it was parts[eq_numb].

Closes #4558

(cherry picked from commit bb905d1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Korablev77 Korablev77

Labels
bug Something isn't working sql
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@kshcherbatov @Korablev77