* [Tarantool-patches] [PATCH 0/2] Msgpack wrong extension type in error message
@ 2020-06-15 15:56 Sergey Kaplun
2020-06-15 15:56 ` [Tarantool-patches] [PATCH 1/2] lib: update msgpuck library Sergey Kaplun
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Sergey Kaplun @ 2020-06-15 15:56 UTC (permalink / raw)
To: tarantool-patches; +Cc: Alexander Turenko
This patch updates msgpucks library to be able to support encoding of
extension type for tarantool version 1.10-2.2 and fixes incorrect
extension type inside error message at msgpack decoding inside default
handler.
Branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5017-msgpack-wrong-ext-type
Issue: https://github.com/tarantool/tarantool/issues/5017
Sergey Kaplun (2):
lib: update msgpuck library
msgpack: fix wrong mp_ext type in error message
src/lib/msgpuck | 2 +-
src/lua/msgpack.c | 5 +-
test/unit/CMakeLists.txt | 5 +
test/unit/mplua.c | 47 ++++
test/unit/mplua.result | 5 +
test/unit/msgpack.result | 480 +++++++++++++++++++++++++++++++++++++--
6 files changed, 528 insertions(+), 16 deletions(-)
create mode 100644 test/unit/mplua.c
create mode 100644 test/unit/mplua.result
--
2.24.1
^ permalink raw reply [flat|nested] 14+ messages in thread* [Tarantool-patches] [PATCH 1/2] lib: update msgpuck library 2020-06-15 15:56 [Tarantool-patches] [PATCH 0/2] Msgpack wrong extension type in error message Sergey Kaplun @ 2020-06-15 15:56 ` Sergey Kaplun 2020-06-15 15:56 ` [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message Sergey Kaplun 2020-07-17 11:27 ` [Tarantool-patches] [PATCH 0/2] Msgpack wrong extension " Kirill Yukhin 2 siblings, 0 replies; 14+ messages in thread From: Sergey Kaplun @ 2020-06-15 15:56 UTC (permalink / raw) To: tarantool-patches; +Cc: Alexander Turenko The ability to encode/decode ext types is added to msgpuck. Needed for #692 (cherry picked from commit 00d770a99f931dcad6f28eb97287e6c13675afed) --- src/lib/msgpuck | 2 +- test/unit/msgpack.result | 480 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 468 insertions(+), 14 deletions(-) diff --git a/src/lib/msgpuck b/src/lib/msgpuck index 3b8f3e59b..fcbe759d3 160000 --- a/src/lib/msgpuck +++ b/src/lib/msgpuck @@ -1 +1 @@ -Subproject commit 3b8f3e59b62d74f0198e01cbec0beb9c6a3082fb +Subproject commit fcbe759d36251017249d5fcbd37c0f59080109f8 diff --git a/test/unit/msgpack.result b/test/unit/msgpack.result index 12e8f8626..919d91615 100644 --- a/test/unit/msgpack.result +++ b/test/unit/msgpack.result @@ -1,4 +1,4 @@ -1..20 +1..22 1..135 # *** test_uints *** # uint 0U @@ -620,6 +620,206 @@ ok 7 - subtests ok 78 - mp_encode(0xffffffffU) == "\xc6\xff\xff\xff\xff" # *** test_binls: done *** ok 8 - subtests + 1..168 + # *** test_extls *** + # extl 0x01U + ok 1 - mp_check_extl(0x01U) == 0 + ok 2 - mp_decode(mp_encode(0x01U)) == 0x01U + ok 3 - len(mp_encode_extl(0x01U) + ok 4 - len(mp_decode_extl(0x01U)) + ok 5 - mp_sizeof_extl(0x01U) + ok 6 - mp_encode(0x01U) == "\xd4\x00" + # extl 0x02U + ok 7 - mp_check_extl(0x02U) == 0 + ok 8 - mp_decode(mp_encode(0x02U)) == 0x02U + ok 9 - len(mp_encode_extl(0x02U) + ok 10 - len(mp_decode_extl(0x02U)) + ok 11 - mp_sizeof_extl(0x02U) + ok 12 - mp_encode(0x02U) == "\xd5\x00" + # extl 0x04U + ok 13 - mp_check_extl(0x04U) == 0 + ok 14 - mp_decode(mp_encode(0x04U)) == 0x04U + ok 15 - len(mp_encode_extl(0x04U) + ok 16 - len(mp_decode_extl(0x04U)) + ok 17 - mp_sizeof_extl(0x04U) + ok 18 - mp_encode(0x04U) == "\xd6\x00" + # extl 0x08U + ok 19 - mp_check_extl(0x08U) == 0 + ok 20 - mp_decode(mp_encode(0x08U)) == 0x08U + ok 21 - len(mp_encode_extl(0x08U) + ok 22 - len(mp_decode_extl(0x08U)) + ok 23 - mp_sizeof_extl(0x08U) + ok 24 - mp_encode(0x08U) == "\xd7\x00" + # extl 0x10U + ok 25 - mp_check_extl(0x10U) == 0 + ok 26 - mp_decode(mp_encode(0x10U)) == 0x10U + ok 27 - len(mp_encode_extl(0x10U) + ok 28 - len(mp_decode_extl(0x10U)) + ok 29 - mp_sizeof_extl(0x10U) + ok 30 - mp_encode(0x10U) == "\xd8\x00" + # extl 0x11U + ok 31 - mp_check_extl(0x11U) == 0 + ok 32 - mp_decode(mp_encode(0x11U)) == 0x11U + ok 33 - len(mp_encode_extl(0x11U) + ok 34 - len(mp_decode_extl(0x11U)) + ok 35 - mp_sizeof_extl(0x11U) + ok 36 - mp_encode(0x11U) == "\xc7\x11\x00" + # extl 0xfeU + ok 37 - mp_check_extl(0xfeU) == 0 + ok 38 - mp_decode(mp_encode(0xfeU)) == 0xfeU + ok 39 - len(mp_encode_extl(0xfeU) + ok 40 - len(mp_decode_extl(0xfeU)) + ok 41 - mp_sizeof_extl(0xfeU) + ok 42 - mp_encode(0xfeU) == "\xc7\xfe\x00" + # extl 0xffU + ok 43 - mp_check_extl(0xffU) == 0 + ok 44 - mp_decode(mp_encode(0xffU)) == 0xffU + ok 45 - len(mp_encode_extl(0xffU) + ok 46 - len(mp_decode_extl(0xffU)) + ok 47 - mp_sizeof_extl(0xffU) + ok 48 - mp_encode(0xffU) == "\xc7\xff\x00" + # extl 0x00U + ok 49 - mp_check_extl(0x00U) == 0 + ok 50 - mp_decode(mp_encode(0x00U)) == 0x00U + ok 51 - len(mp_encode_extl(0x00U) + ok 52 - len(mp_decode_extl(0x00U)) + ok 53 - mp_sizeof_extl(0x00U) + ok 54 - mp_encode(0x00U) == "\xc7\x00\x00" + # extl 0x03U + ok 55 - mp_check_extl(0x03U) == 0 + ok 56 - mp_decode(mp_encode(0x03U)) == 0x03U + ok 57 - len(mp_encode_extl(0x03U) + ok 58 - len(mp_decode_extl(0x03U)) + ok 59 - mp_sizeof_extl(0x03U) + ok 60 - mp_encode(0x03U) == "\xc7\x03\x00" + # extl 0x05U + ok 61 - mp_check_extl(0x05U) == 0 + ok 62 - mp_decode(mp_encode(0x05U)) == 0x05U + ok 63 - len(mp_encode_extl(0x05U) + ok 64 - len(mp_decode_extl(0x05U)) + ok 65 - mp_sizeof_extl(0x05U) + ok 66 - mp_encode(0x05U) == "\xc7\x05\x00" + # extl 0x06U + ok 67 - mp_check_extl(0x06U) == 0 + ok 68 - mp_decode(mp_encode(0x06U)) == 0x06U + ok 69 - len(mp_encode_extl(0x06U) + ok 70 - len(mp_decode_extl(0x06U)) + ok 71 - mp_sizeof_extl(0x06U) + ok 72 - mp_encode(0x06U) == "\xc7\x06\x00" + # extl 0x07U + ok 73 - mp_check_extl(0x07U) == 0 + ok 74 - mp_decode(mp_encode(0x07U)) == 0x07U + ok 75 - len(mp_encode_extl(0x07U) + ok 76 - len(mp_decode_extl(0x07U)) + ok 77 - mp_sizeof_extl(0x07U) + ok 78 - mp_encode(0x07U) == "\xc7\x07\x00" + # extl 0x09U + ok 79 - mp_check_extl(0x09U) == 0 + ok 80 - mp_decode(mp_encode(0x09U)) == 0x09U + ok 81 - len(mp_encode_extl(0x09U) + ok 82 - len(mp_decode_extl(0x09U)) + ok 83 - mp_sizeof_extl(0x09U) + ok 84 - mp_encode(0x09U) == "\xc7\x09\x00" + # extl 0x0aU + ok 85 - mp_check_extl(0x0aU) == 0 + ok 86 - mp_decode(mp_encode(0x0aU)) == 0x0aU + ok 87 - len(mp_encode_extl(0x0aU) + ok 88 - len(mp_decode_extl(0x0aU)) + ok 89 - mp_sizeof_extl(0x0aU) + ok 90 - mp_encode(0x0aU) == "\xc7\x0a\x00" + # extl 0x0bU + ok 91 - mp_check_extl(0x0bU) == 0 + ok 92 - mp_decode(mp_encode(0x0bU)) == 0x0bU + ok 93 - len(mp_encode_extl(0x0bU) + ok 94 - len(mp_decode_extl(0x0bU)) + ok 95 - mp_sizeof_extl(0x0bU) + ok 96 - mp_encode(0x0bU) == "\xc7\x0b\x00" + # extl 0x0cU + ok 97 - mp_check_extl(0x0cU) == 0 + ok 98 - mp_decode(mp_encode(0x0cU)) == 0x0cU + ok 99 - len(mp_encode_extl(0x0cU) + ok 100 - len(mp_decode_extl(0x0cU)) + ok 101 - mp_sizeof_extl(0x0cU) + ok 102 - mp_encode(0x0cU) == "\xc7\x0c\x00" + # extl 0x0dU + ok 103 - mp_check_extl(0x0dU) == 0 + ok 104 - mp_decode(mp_encode(0x0dU)) == 0x0dU + ok 105 - len(mp_encode_extl(0x0dU) + ok 106 - len(mp_decode_extl(0x0dU)) + ok 107 - mp_sizeof_extl(0x0dU) + ok 108 - mp_encode(0x0dU) == "\xc7\x0d\x00" + # extl 0x0eU + ok 109 - mp_check_extl(0x0eU) == 0 + ok 110 - mp_decode(mp_encode(0x0eU)) == 0x0eU + ok 111 - len(mp_encode_extl(0x0eU) + ok 112 - len(mp_decode_extl(0x0eU)) + ok 113 - mp_sizeof_extl(0x0eU) + ok 114 - mp_encode(0x0eU) == "\xc7\x0e\x00" + # extl 0x0fU + ok 115 - mp_check_extl(0x0fU) == 0 + ok 116 - mp_decode(mp_encode(0x0fU)) == 0x0fU + ok 117 - len(mp_encode_extl(0x0fU) + ok 118 - len(mp_decode_extl(0x0fU)) + ok 119 - mp_sizeof_extl(0x0fU) + ok 120 - mp_encode(0x0fU) == "\xc7\x0f\x00" + # extl 0x0100U + ok 121 - mp_check_extl(0x0100U) == 0 + ok 122 - mp_decode(mp_encode(0x0100U)) == 0x0100U + ok 123 - len(mp_encode_extl(0x0100U) + ok 124 - len(mp_decode_extl(0x0100U)) + ok 125 - mp_sizeof_extl(0x0100U) + ok 126 - mp_encode(0x0100U) == "\xc8\x01\x00\x00" + # extl 0x0101U + ok 127 - mp_check_extl(0x0101U) == 0 + ok 128 - mp_decode(mp_encode(0x0101U)) == 0x0101U + ok 129 - len(mp_encode_extl(0x0101U) + ok 130 - len(mp_decode_extl(0x0101U)) + ok 131 - mp_sizeof_extl(0x0101U) + ok 132 - mp_encode(0x0101U) == "\xc8\x01\x01\x00" + # extl 0xfffeU + ok 133 - mp_check_extl(0xfffeU) == 0 + ok 134 - mp_decode(mp_encode(0xfffeU)) == 0xfffeU + ok 135 - len(mp_encode_extl(0xfffeU) + ok 136 - len(mp_decode_extl(0xfffeU)) + ok 137 - mp_sizeof_extl(0xfffeU) + ok 138 - mp_encode(0xfffeU) == "\xc8\xff\xfe\x00" + # extl 0xffffU + ok 139 - mp_check_extl(0xffffU) == 0 + ok 140 - mp_decode(mp_encode(0xffffU)) == 0xffffU + ok 141 - len(mp_encode_extl(0xffffU) + ok 142 - len(mp_decode_extl(0xffffU)) + ok 143 - mp_sizeof_extl(0xffffU) + ok 144 - mp_encode(0xffffU) == "\xc8\xff\xff\x00" + # extl 0x00010000U + ok 145 - mp_check_extl(0x00010000U) == 0 + ok 146 - mp_decode(mp_encode(0x00010000U)) == 0x00010000U + ok 147 - len(mp_encode_extl(0x00010000U) + ok 148 - len(mp_decode_extl(0x00010000U)) + ok 149 - mp_sizeof_extl(0x00010000U) + ok 150 - mp_encode(0x00010000U) == "\xc9\x00\x01\x00\x00\x00" + # extl 0x00010001U + ok 151 - mp_check_extl(0x00010001U) == 0 + ok 152 - mp_decode(mp_encode(0x00010001U)) == 0x00010001U + ok 153 - len(mp_encode_extl(0x00010001U) + ok 154 - len(mp_decode_extl(0x00010001U)) + ok 155 - mp_sizeof_extl(0x00010001U) + ok 156 - mp_encode(0x00010001U) == "\xc9\x00\x01\x00\x01\x00" + # extl 0xfffffffeU + ok 157 - mp_check_extl(0xfffffffeU) == 0 + ok 158 - mp_decode(mp_encode(0xfffffffeU)) == 0xfffffffeU + ok 159 - len(mp_encode_extl(0xfffffffeU) + ok 160 - len(mp_decode_extl(0xfffffffeU)) + ok 161 - mp_sizeof_extl(0xfffffffeU) + ok 162 - mp_encode(0xfffffffeU) == "\xc9\xff\xff\xff\xfe\x00" + # extl 0xffffffffU + ok 163 - mp_check_extl(0xffffffffU) == 0 + ok 164 - mp_decode(mp_encode(0xffffffffU)) == 0xffffffffU + ok 165 - len(mp_encode_extl(0xffffffffU) + ok 166 - len(mp_decode_extl(0xffffffffU)) + ok 167 - mp_sizeof_extl(0xffffffffU) + ok 168 - mp_encode(0xffffffffU) == "\xc9\xff\xff\xff\xff\x00" + # *** test_extls: done *** +ok 9 - subtests 1..96 # *** test_strs *** # str len=0x01 @@ -731,7 +931,7 @@ ok 8 - subtests ok 95 - mp_sizeof_str(0x10001) ok 96 - mp_encode_str(x, 0x10001) == x # *** test_strs: done *** -ok 9 - subtests +ok 10 - subtests 1..96 # *** test_bins *** # bin len=0x01 @@ -843,7 +1043,261 @@ ok 9 - subtests ok 95 - mp_sizeof_bin(0x10001) ok 96 - mp_encode_bin(x, 0x10001) == x # *** test_bins: done *** -ok 10 - subtests +ok 11 - subtests + 1..225 + # *** test_exts *** + # ext len=0x01 + ok 1 - len(mp_decode_ext(x, 1)) + ok 2 - type(mp_decode_ext(x)) + ok 3 - len(mp_decode_strbin(x, 1)) + ok 4 - mp_check_ext(mp_encode_ext(x, 0x01)) + ok 5 - len(mp_decode_ext(x, 0x01) + ok 6 - len(mp_next_ext(x, 0x01) + ok 7 - len(mp_check_ext(x, 0x01) + ok 8 - mp_sizeof_ext(0x01) + ok 9 - mp_encode_ext(x, 0x01) == x + # ext len=0x02 + ok 10 - len(mp_decode_ext(x, 2)) + ok 11 - type(mp_decode_ext(x)) + ok 12 - len(mp_decode_strbin(x, 2)) + ok 13 - mp_check_ext(mp_encode_ext(x, 0x02)) + ok 14 - len(mp_decode_ext(x, 0x02) + ok 15 - len(mp_next_ext(x, 0x02) + ok 16 - len(mp_check_ext(x, 0x02) + ok 17 - mp_sizeof_ext(0x02) + ok 18 - mp_encode_ext(x, 0x02) == x + # ext len=0x03 + ok 19 - len(mp_decode_ext(x, 3)) + ok 20 - type(mp_decode_ext(x)) + ok 21 - len(mp_decode_strbin(x, 3)) + ok 22 - mp_check_ext(mp_encode_ext(x, 0x03)) + ok 23 - len(mp_decode_ext(x, 0x03) + ok 24 - len(mp_next_ext(x, 0x03) + ok 25 - len(mp_check_ext(x, 0x03) + ok 26 - mp_sizeof_ext(0x03) + ok 27 - mp_encode_ext(x, 0x03) == x + # ext len=0x04 + ok 28 - len(mp_decode_ext(x, 4)) + ok 29 - type(mp_decode_ext(x)) + ok 30 - len(mp_decode_strbin(x, 4)) + ok 31 - mp_check_ext(mp_encode_ext(x, 0x04)) + ok 32 - len(mp_decode_ext(x, 0x04) + ok 33 - len(mp_next_ext(x, 0x04) + ok 34 - len(mp_check_ext(x, 0x04) + ok 35 - mp_sizeof_ext(0x04) + ok 36 - mp_encode_ext(x, 0x04) == x + # ext len=0x05 + ok 37 - len(mp_decode_ext(x, 5)) + ok 38 - type(mp_decode_ext(x)) + ok 39 - len(mp_decode_strbin(x, 5)) + ok 40 - mp_check_ext(mp_encode_ext(x, 0x05)) + ok 41 - len(mp_decode_ext(x, 0x05) + ok 42 - len(mp_next_ext(x, 0x05) + ok 43 - len(mp_check_ext(x, 0x05) + ok 44 - mp_sizeof_ext(0x05) + ok 45 - mp_encode_ext(x, 0x05) == x + # ext len=0x06 + ok 46 - len(mp_decode_ext(x, 6)) + ok 47 - type(mp_decode_ext(x)) + ok 48 - len(mp_decode_strbin(x, 6)) + ok 49 - mp_check_ext(mp_encode_ext(x, 0x06)) + ok 50 - len(mp_decode_ext(x, 0x06) + ok 51 - len(mp_next_ext(x, 0x06) + ok 52 - len(mp_check_ext(x, 0x06) + ok 53 - mp_sizeof_ext(0x06) + ok 54 - mp_encode_ext(x, 0x06) == x + # ext len=0x07 + ok 55 - len(mp_decode_ext(x, 7)) + ok 56 - type(mp_decode_ext(x)) + ok 57 - len(mp_decode_strbin(x, 7)) + ok 58 - mp_check_ext(mp_encode_ext(x, 0x07)) + ok 59 - len(mp_decode_ext(x, 0x07) + ok 60 - len(mp_next_ext(x, 0x07) + ok 61 - len(mp_check_ext(x, 0x07) + ok 62 - mp_sizeof_ext(0x07) + ok 63 - mp_encode_ext(x, 0x07) == x + # ext len=0x08 + ok 64 - len(mp_decode_ext(x, 8)) + ok 65 - type(mp_decode_ext(x)) + ok 66 - len(mp_decode_strbin(x, 8)) + ok 67 - mp_check_ext(mp_encode_ext(x, 0x08)) + ok 68 - len(mp_decode_ext(x, 0x08) + ok 69 - len(mp_next_ext(x, 0x08) + ok 70 - len(mp_check_ext(x, 0x08) + ok 71 - mp_sizeof_ext(0x08) + ok 72 - mp_encode_ext(x, 0x08) == x + # ext len=0x09 + ok 73 - len(mp_decode_ext(x, 9)) + ok 74 - type(mp_decode_ext(x)) + ok 75 - len(mp_decode_strbin(x, 9)) + ok 76 - mp_check_ext(mp_encode_ext(x, 0x09)) + ok 77 - len(mp_decode_ext(x, 0x09) + ok 78 - len(mp_next_ext(x, 0x09) + ok 79 - len(mp_check_ext(x, 0x09) + ok 80 - mp_sizeof_ext(0x09) + ok 81 - mp_encode_ext(x, 0x09) == x + # ext len=0x0a + ok 82 - len(mp_decode_ext(x, 10)) + ok 83 - type(mp_decode_ext(x)) + ok 84 - len(mp_decode_strbin(x, 10)) + ok 85 - mp_check_ext(mp_encode_ext(x, 0x0a)) + ok 86 - len(mp_decode_ext(x, 0x0a) + ok 87 - len(mp_next_ext(x, 0x0a) + ok 88 - len(mp_check_ext(x, 0x0a) + ok 89 - mp_sizeof_ext(0x0a) + ok 90 - mp_encode_ext(x, 0x0a) == x + # ext len=0x0b + ok 91 - len(mp_decode_ext(x, 11)) + ok 92 - type(mp_decode_ext(x)) + ok 93 - len(mp_decode_strbin(x, 11)) + ok 94 - mp_check_ext(mp_encode_ext(x, 0x0b)) + ok 95 - len(mp_decode_ext(x, 0x0b) + ok 96 - len(mp_next_ext(x, 0x0b) + ok 97 - len(mp_check_ext(x, 0x0b) + ok 98 - mp_sizeof_ext(0x0b) + ok 99 - mp_encode_ext(x, 0x0b) == x + # ext len=0x0c + ok 100 - len(mp_decode_ext(x, 12)) + ok 101 - type(mp_decode_ext(x)) + ok 102 - len(mp_decode_strbin(x, 12)) + ok 103 - mp_check_ext(mp_encode_ext(x, 0x0c)) + ok 104 - len(mp_decode_ext(x, 0x0c) + ok 105 - len(mp_next_ext(x, 0x0c) + ok 106 - len(mp_check_ext(x, 0x0c) + ok 107 - mp_sizeof_ext(0x0c) + ok 108 - mp_encode_ext(x, 0x0c) == x + # ext len=0x0d + ok 109 - len(mp_decode_ext(x, 13)) + ok 110 - type(mp_decode_ext(x)) + ok 111 - len(mp_decode_strbin(x, 13)) + ok 112 - mp_check_ext(mp_encode_ext(x, 0x0d)) + ok 113 - len(mp_decode_ext(x, 0x0d) + ok 114 - len(mp_next_ext(x, 0x0d) + ok 115 - len(mp_check_ext(x, 0x0d) + ok 116 - mp_sizeof_ext(0x0d) + ok 117 - mp_encode_ext(x, 0x0d) == x + # ext len=0x0e + ok 118 - len(mp_decode_ext(x, 14)) + ok 119 - type(mp_decode_ext(x)) + ok 120 - len(mp_decode_strbin(x, 14)) + ok 121 - mp_check_ext(mp_encode_ext(x, 0x0e)) + ok 122 - len(mp_decode_ext(x, 0x0e) + ok 123 - len(mp_next_ext(x, 0x0e) + ok 124 - len(mp_check_ext(x, 0x0e) + ok 125 - mp_sizeof_ext(0x0e) + ok 126 - mp_encode_ext(x, 0x0e) == x + # ext len=0x0f + ok 127 - len(mp_decode_ext(x, 15)) + ok 128 - type(mp_decode_ext(x)) + ok 129 - len(mp_decode_strbin(x, 15)) + ok 130 - mp_check_ext(mp_encode_ext(x, 0x0f)) + ok 131 - len(mp_decode_ext(x, 0x0f) + ok 132 - len(mp_next_ext(x, 0x0f) + ok 133 - len(mp_check_ext(x, 0x0f) + ok 134 - mp_sizeof_ext(0x0f) + ok 135 - mp_encode_ext(x, 0x0f) == x + # ext len=0x10 + ok 136 - len(mp_decode_ext(x, 16)) + ok 137 - type(mp_decode_ext(x)) + ok 138 - len(mp_decode_strbin(x, 16)) + ok 139 - mp_check_ext(mp_encode_ext(x, 0x10)) + ok 140 - len(mp_decode_ext(x, 0x10) + ok 141 - len(mp_next_ext(x, 0x10) + ok 142 - len(mp_check_ext(x, 0x10) + ok 143 - mp_sizeof_ext(0x10) + ok 144 - mp_encode_ext(x, 0x10) == x + # ext len=0x11 + ok 145 - len(mp_decode_ext(x, 17)) + ok 146 - type(mp_decode_ext(x)) + ok 147 - len(mp_decode_strbin(x, 17)) + ok 148 - mp_check_ext(mp_encode_ext(x, 0x11)) + ok 149 - len(mp_decode_ext(x, 0x11) + ok 150 - len(mp_next_ext(x, 0x11) + ok 151 - len(mp_check_ext(x, 0x11) + ok 152 - mp_sizeof_ext(0x11) + ok 153 - mp_encode_ext(x, 0x11) == x + # ext len=0xfe + ok 154 - len(mp_decode_ext(x, 254)) + ok 155 - type(mp_decode_ext(x)) + ok 156 - len(mp_decode_strbin(x, 254)) + ok 157 - mp_check_ext(mp_encode_ext(x, 0xfe)) + ok 158 - len(mp_decode_ext(x, 0xfe) + ok 159 - len(mp_next_ext(x, 0xfe) + ok 160 - len(mp_check_ext(x, 0xfe) + ok 161 - mp_sizeof_ext(0xfe) + ok 162 - mp_encode_ext(x, 0xfe) == x + # ext len=0xff + ok 163 - len(mp_decode_ext(x, 255)) + ok 164 - type(mp_decode_ext(x)) + ok 165 - len(mp_decode_strbin(x, 255)) + ok 166 - mp_check_ext(mp_encode_ext(x, 0xff)) + ok 167 - len(mp_decode_ext(x, 0xff) + ok 168 - len(mp_next_ext(x, 0xff) + ok 169 - len(mp_check_ext(x, 0xff) + ok 170 - mp_sizeof_ext(0xff) + ok 171 - mp_encode_ext(x, 0xff) == x + # ext len=0x0100 + ok 172 - len(mp_decode_ext(x, 256)) + ok 173 - type(mp_decode_ext(x)) + ok 174 - len(mp_decode_strbin(x, 256)) + ok 175 - mp_check_ext(mp_encode_ext(x, 0x0100)) + ok 176 - len(mp_decode_ext(x, 0x0100) + ok 177 - len(mp_next_ext(x, 0x0100) + ok 178 - len(mp_check_ext(x, 0x0100) + ok 179 - mp_sizeof_ext(0x0100) + ok 180 - mp_encode_ext(x, 0x0100) == x + # ext len=0x0101 + ok 181 - len(mp_decode_ext(x, 257)) + ok 182 - type(mp_decode_ext(x)) + ok 183 - len(mp_decode_strbin(x, 257)) + ok 184 - mp_check_ext(mp_encode_ext(x, 0x0101)) + ok 185 - len(mp_decode_ext(x, 0x0101) + ok 186 - len(mp_next_ext(x, 0x0101) + ok 187 - len(mp_check_ext(x, 0x0101) + ok 188 - mp_sizeof_ext(0x0101) + ok 189 - mp_encode_ext(x, 0x0101) == x + # ext len=0xfffe + ok 190 - len(mp_decode_ext(x, 65534)) + ok 191 - type(mp_decode_ext(x)) + ok 192 - len(mp_decode_strbin(x, 65534)) + ok 193 - mp_check_ext(mp_encode_ext(x, 0xfffe)) + ok 194 - len(mp_decode_ext(x, 0xfffe) + ok 195 - len(mp_next_ext(x, 0xfffe) + ok 196 - len(mp_check_ext(x, 0xfffe) + ok 197 - mp_sizeof_ext(0xfffe) + ok 198 - mp_encode_ext(x, 0xfffe) == x + # ext len=0xffff + ok 199 - len(mp_decode_ext(x, 65535)) + ok 200 - type(mp_decode_ext(x)) + ok 201 - len(mp_decode_strbin(x, 65535)) + ok 202 - mp_check_ext(mp_encode_ext(x, 0xffff)) + ok 203 - len(mp_decode_ext(x, 0xffff) + ok 204 - len(mp_next_ext(x, 0xffff) + ok 205 - len(mp_check_ext(x, 0xffff) + ok 206 - mp_sizeof_ext(0xffff) + ok 207 - mp_encode_ext(x, 0xffff) == x + # ext len=0x00010000 + ok 208 - len(mp_decode_ext(x, 65536)) + ok 209 - type(mp_decode_ext(x)) + ok 210 - len(mp_decode_strbin(x, 65536)) + ok 211 - mp_check_ext(mp_encode_ext(x, 0x00010000)) + ok 212 - len(mp_decode_ext(x, 0x00010000) + ok 213 - len(mp_next_ext(x, 0x00010000) + ok 214 - len(mp_check_ext(x, 0x00010000) + ok 215 - mp_sizeof_ext(0x00010000) + ok 216 - mp_encode_ext(x, 0x00010000) == x + # ext len=0x00010001 + ok 217 - len(mp_decode_ext(x, 65537)) + ok 218 - type(mp_decode_ext(x)) + ok 219 - len(mp_decode_strbin(x, 65537)) + ok 220 - mp_check_ext(mp_encode_ext(x, 0x00010001)) + ok 221 - len(mp_decode_ext(x, 0x00010001) + ok 222 - len(mp_next_ext(x, 0x00010001) + ok 223 - len(mp_check_ext(x, 0x00010001) + ok 224 - mp_sizeof_ext(0x00010001) + ok 225 - mp_encode_ext(x, 0x00010001) == x + # *** test_exts: done *** +ok 12 - subtests 1..54 # *** test_arrays *** # array 0 @@ -910,7 +1364,7 @@ ok 10 - subtests ok 53 - mp_sizeof_array(0xffffffffU) ok 54 - mp_encode(0xffffffffU) == "\xdd\xff\xff\xff\xff" # *** test_arrays: done *** -ok 11 - subtests +ok 13 - subtests 1..54 # *** test_maps *** # map 0 @@ -977,7 +1431,7 @@ ok 11 - subtests ok 53 - mp_sizeof_map(0xffffffffU) ok 54 - mp_encode(0xffffffffU) == "\xdf\xff\xff\xff\xff" # *** test_maps: done *** -ok 12 - subtests +ok 14 - subtests 1..52 # *** test_next_on_arrays *** # next/check on array(0) @@ -1046,7 +1500,7 @@ ok 12 - subtests ok 51 - len(mp_check(array 65537)) == 65542 ok 52 - len(mp_next(array 65537)) == 65542 # *** test_next_on_arrays: done *** -ok 13 - subtests +ok 15 - subtests 1..52 # *** test_next_on_maps *** # next/check on map(0) @@ -1115,7 +1569,7 @@ ok 13 - subtests ok 51 - len(mp_check(map 65537)) == 131079 ok 52 - len(mp_next(map 65537)) == 131079 # *** test_next_on_maps: done *** -ok 14 - subtests +ok 16 - subtests 1..227 # *** test_compare_uints *** ok 1 - mp_compare_uint(0, 0) == 0 @@ -1346,7 +1800,7 @@ ok 14 - subtests ok 226 - mp_compare_uint(18446744073709551615, 18446744073709551614) > 0 ok 227 - mp_compare_uint(18446744073709551615, 18446744073709551615) == 0 # *** test_compare_uints: done *** -ok 15 - subtests +ok 17 - subtests 1..282 # *** test_format *** ok 1 - Test type on step 0 @@ -1632,7 +2086,7 @@ ok 15 - subtests ok 281 - return value on step 70 ok 282 - buffer overflow on step 70 # *** test_format: done *** -ok 16 - subtests +ok 18 - subtests 1..10 # *** test_mp_print *** ok 1 - mp_snprint return value @@ -1646,7 +2100,7 @@ ok 16 - subtests ok 9 - mp_fprint result ok 10 - mp_fprint I/O error # *** test_mp_print: done *** -ok 17 - subtests +ok 19 - subtests 1..65 # *** test_mp_check *** ok 1 - invalid fixmap 1 @@ -1715,7 +2169,7 @@ ok 17 - subtests ok 64 - invalid map32 2 ok 65 - invalid map32 3 # *** test_mp_check: done *** -ok 18 - subtests +ok 20 - subtests 1..96 # *** test_numbers *** ok 1 - mp_read_int32(mp_encode_uint(123)) check success @@ -1815,7 +2269,7 @@ ok 18 - subtests ok 95 - mp_read_double(mp_encode_strl(100)) check fail ok 96 - mp_read_double(mp_encode_strl(100)) check pos unchanged # *** test_numbers: done *** -ok 19 - subtests +ok 21 - subtests 1..4 # *** test_overflow *** ok 1 - mp_check array overflow @@ -1823,4 +2277,4 @@ ok 19 - subtests ok 3 - mp_check str overflow ok 4 - mp_check bin overflow # *** test_overflow: done *** -ok 20 - subtests +ok 22 - subtests -- 2.24.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message 2020-06-15 15:56 [Tarantool-patches] [PATCH 0/2] Msgpack wrong extension type in error message Sergey Kaplun 2020-06-15 15:56 ` [Tarantool-patches] [PATCH 1/2] lib: update msgpuck library Sergey Kaplun @ 2020-06-15 15:56 ` Sergey Kaplun 2020-07-14 15:46 ` Igor Munkin 2020-07-17 11:27 ` [Tarantool-patches] [PATCH 0/2] Msgpack wrong extension " Kirill Yukhin 2 siblings, 1 reply; 14+ messages in thread From: Sergey Kaplun @ 2020-06-15 15:56 UTC (permalink / raw) To: tarantool-patches; +Cc: Alexander Turenko This patch adds decoding of mp_ext type and inserts it into error message when an error is raised Closes #5017 --- src/lua/msgpack.c | 5 +++-- test/unit/CMakeLists.txt | 5 +++++ test/unit/mplua.c | 47 ++++++++++++++++++++++++++++++++++++++++ test/unit/mplua.result | 5 +++++ 4 files changed, 60 insertions(+), 2 deletions(-) create mode 100644 test/unit/mplua.c create mode 100644 test/unit/mplua.result diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c index b7a2955fe..e3935d383 100644 --- a/src/lua/msgpack.c +++ b/src/lua/msgpack.c @@ -224,8 +224,9 @@ luamp_set_encode_extension(luamp_encode_extension_f handler) static void luamp_decode_extension_default(struct lua_State *L, const char **data) { - luaL_error(L, "msgpack.decode: unsupported extension: %u", - (unsigned char) **data); + int8_t ext_type; + mp_decode_extl(data, &ext_type); + luaL_error(L, "msgpack.decode: unsupported extension: %u", ext_type); } diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt index 6dfb10f46..906422e82 100644 --- a/test/unit/CMakeLists.txt +++ b/test/unit/CMakeLists.txt @@ -205,3 +205,8 @@ target_link_libraries(coll.test core unit ${ICU_LIBRARIES} misc) add_executable(tuple_bigref.test tuple_bigref.c) target_link_libraries(tuple_bigref.test tuple unit) + +add_executable(mplua.test mplua.c) +target_link_libraries(mplua.test unit box server core + ${CURL_LIBRARIES} ${LIBYAML_LIBRARIES} ${READLINE_LIBRARIES} + ${ICU_LIBRARIES} ${LUAJIT_LIBRARIES}) diff --git a/test/unit/mplua.c b/test/unit/mplua.c new file mode 100644 index 000000000..3b7f4de76 --- /dev/null +++ b/test/unit/mplua.c @@ -0,0 +1,47 @@ +#include <lua.h> /* lua_*() */ +#include <lauxlib.h> /* luaL_*() */ +#include <lualib.h> /* luaL_openlibs() */ +#include <string.h> /* memcmp() */ + +#include "lua/msgpack.h" +#include "unit.h" + +/* + * test for https://github.com/tarantool/tarantool/issues/5017 + */ + + +static int +lua_encode_unknown_mp(struct lua_State *L) +{ + const char test_str[] = "\xd4\x0f\x00"; + const char *data = (char *)test_str; + luamp_decode(L, luaL_msgpack_default, &data); + return 0; +} + + +static void +test_luamp_encode_extension_default(struct lua_State *L) +{ + plan(2); + lua_pushcfunction(L, lua_encode_unknown_mp); + ok(lua_pcall(L, 0, 0, 0) != 0, + "mplua_decode: unsupported extension raise error"); + const char err_msg[] = "msgpack.decode: unsupported extension: 15"; + ok(memcmp(lua_tostring(L, -1), err_msg, sizeof(err_msg)) == 0, + "mplua_decode: unsupported extension correct type"); +} + + +int +main(void) +{ + header(); + + struct lua_State *L = luaL_newstate(); + test_luamp_encode_extension_default(L); + + footer(); + check_plan(); +} diff --git a/test/unit/mplua.result b/test/unit/mplua.result new file mode 100644 index 000000000..6dc9d2ee3 --- /dev/null +++ b/test/unit/mplua.result @@ -0,0 +1,5 @@ + *** main *** +1..2 +ok 1 - mplua_decode: unsupported extension raise error +ok 2 - mplua_decode: unsupported extension correct type + *** main: done *** -- 2.24.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message 2020-06-15 15:56 ` [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message Sergey Kaplun @ 2020-07-14 15:46 ` Igor Munkin 2020-07-15 9:32 ` Sergey Kaplun 0 siblings, 1 reply; 14+ messages in thread From: Igor Munkin @ 2020-07-14 15:46 UTC (permalink / raw) To: Sergey Kaplun; +Cc: tarantool-patches, Alexander Turenko Sergey, Thanks for the patch! Please consider my comments below. On 15.06.20, Sergey Kaplun wrote: > This patch adds decoding of mp_ext type and inserts it into error > message when an error is raised But what was the problem? I can read various binary sequences, but I'm still not good at MessagePack. Could you please describe what exactly was wrong prior to your fix? > > Closes #5017 > --- > src/lua/msgpack.c | 5 +++-- > test/unit/CMakeLists.txt | 5 +++++ > test/unit/mplua.c | 47 ++++++++++++++++++++++++++++++++++++++++ > test/unit/mplua.result | 5 +++++ > 4 files changed, 60 insertions(+), 2 deletions(-) > create mode 100644 test/unit/mplua.c > create mode 100644 test/unit/mplua.result > <snipped> > diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt > index 6dfb10f46..906422e82 100644 > --- a/test/unit/CMakeLists.txt > +++ b/test/unit/CMakeLists.txt > @@ -205,3 +205,8 @@ target_link_libraries(coll.test core unit ${ICU_LIBRARIES} misc) > > add_executable(tuple_bigref.test tuple_bigref.c) > target_link_libraries(tuple_bigref.test tuple unit) > + > +add_executable(mplua.test mplua.c) > +target_link_libraries(mplua.test unit box server core > + ${CURL_LIBRARIES} ${LIBYAML_LIBRARIES} ${READLINE_LIBRARIES} > + ${ICU_LIBRARIES} ${LUAJIT_LIBRARIES}) Why should this be done in such complex way? Do your test binary need *all* mentioned libs? I see no usage for libcurl, libreadline, libicu, libyaml in the test. Furthermore, IIRC LuaJIT symbols are provided by libserver archive. Well, I faced the following linkage error when the test is build in Release mode: | [ 87%] Linking CXX executable mplua.test | /usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/../../../../x86_64-pc-linux-gnu/bin/ld: ../../src/libcore.a(evio.cc.o): in function `evio_service_bind(evio_service*, char const*)': | evio.cc:(.text+0x9e2): undefined reference to `uri_parse' | collect2: error: ld returned 1 exit status I don't know what to say... > diff --git a/test/unit/mplua.c b/test/unit/mplua.c > new file mode 100644 > index 000000000..3b7f4de76 > --- /dev/null > +++ b/test/unit/mplua.c > @@ -0,0 +1,47 @@ > +#include <lua.h> /* lua_*() */ > +#include <lauxlib.h> /* luaL_*() */ Minor: Since we use bundled LuaJIT you need to lookup "local" headers prior to the system-wide ones[1]. But I've run make with more verbose output and seen we don't use "-I-" in preprocessor flags[2], so I see no difference how to specify such includes (IMHO it's not the proper way): | [100%] Building C object test/unit/CMakeFiles/mplua.test.dir/mplua.c.o | cd /tarantool/test/unit && /usr/bin/cc -DCORO_ASM | -DLUAJIT_SMART_STRINGS=1 -DLUA_USE_APICHECK=1 -DLUA_USE_ASSERT=1 | -DNVALGRIND=1 -DYAML_DECLARE_STATIC -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE | -D__STDC_CONSTANT_MACROS=1 -D__STDC_FORMAT_MACROS=1 | -D__STDC_LIMIT_MACROS=1 -I/tarantool/src -I/tarantool/src/lib | -I/tarantool/src/lib/small -I/tarantool/src/lib/small/third_party | -I/tarantool -I/tarantool/third_party/zstd/lib | -I/tarantool/third_party/zstd/lib/common | -I/tarantool/third_party/luajit/src -I/tarantool/src/lib/msgpuck | -I/tarantool/src/box -I/tarantool/third_party | -I/tarantool/build/curl/dest/include | -I/tarantool/third_party/libyaml/include -fexceptions -funwind-tables | -fno-omit-frame-pointer -fno-stack-protector -fno-common -fopenmp -msse2 | -std=c11 -Wall -Wextra -Wno-strict-aliasing -Wno-char-subscripts | -Wno-format-truncation -fno-gnu89-inline -Wno-cast-function-type -Werror | -Wno-unused-parameter -Wno-unused -Wno-unused-result | -Wno-tautological-compare -g -ggdb -O0 -o | CMakeFiles/mplua.test.dir/mplua.c.o -c /tarantool/test/unit/mplua.c > +#include <lualib.h> /* luaL_openlibs() */ This include is excess. > +#include <string.h> /* memcmp() */ > + > +#include "lua/msgpack.h" > +#include "unit.h" > + > +/* > + * test for https://github.com/tarantool/tarantool/issues/5017 > + */ > + > + > +static int > +lua_encode_unknown_mp(struct lua_State *L) > +{ > + const char test_str[] = "\xd4\x0f\x00"; > + const char *data = (char *)test_str; Why do you create a distinct pointer for to be passed to <luamp_decode>? You can just properly declare <test_str>: | const char *test_str = "\xd4\x0f\00"; > + luamp_decode(L, luaL_msgpack_default, &data); > + return 0; > +} > + > + > +static void > +test_luamp_encode_extension_default(struct lua_State *L) > +{ > + plan(2); > + lua_pushcfunction(L, lua_encode_unknown_mp); > + ok(lua_pcall(L, 0, 0, 0) != 0, You can check that exactly <LUA_ERRRUN> is yielded here. > + "mplua_decode: unsupported extension raise error"); You can simply use <lua_cpcall> here. > + const char err_msg[] = "msgpack.decode: unsupported extension: 15"; > + ok(memcmp(lua_tostring(L, -1), err_msg, sizeof(err_msg)) == 0, > + "mplua_decode: unsupported extension correct type"); > +} > + > + Well, it looks like you've mixed several approaches and it led to the following mess: * You set test plan right in the test case, but check it in the <main> routine. I guess these calls should be placed within the same function, otherwise they are hard maintained. * There are lots of tests using header/footer but no one except <base64.c> uses it right in <main>. I don't know, what these macros are for, but most of the tests use them in the test case. The test case is really small and I guess you can move everything right to the <main> as it's done in rlist.c Besides, the main question is why do you use C test instead of writing a small Lua test (similar to the one in the issue)? At least you can avoid all linkage issues. > +int > +main(void) > +{ > + header(); > + > + struct lua_State *L = luaL_newstate(); > + test_luamp_encode_extension_default(L); > + > + footer(); > + check_plan(); The function should return <int> but returns nothing. All other tests have the following idiom: | return check_plan(); > +} <snipped> > -- > 2.24.1 > [1]: https://gcc.gnu.org/onlinedocs/gcc-10.1.0/cpp/Include-Syntax.html#Include-Syntax [2]: https://gcc.gnu.org/onlinedocs/gcc-10.1.0/cpp/Invocation.html#Invocation -- Best regards, IM ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message 2020-07-14 15:46 ` Igor Munkin @ 2020-07-15 9:32 ` Sergey Kaplun 2020-07-15 15:21 ` Sergey Kaplun 2020-07-16 20:53 ` Igor Munkin 0 siblings, 2 replies; 14+ messages in thread From: Sergey Kaplun @ 2020-07-15 9:32 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches, Alexander Turenko Hi Igor! Thanks for the review! On 14.07.20, Igor Munkin wrote: > Sergey, > > Thanks for the patch! Please consider my comments below. > > On 15.06.20, Sergey Kaplun wrote: > > This patch adds decoding of mp_ext type and inserts it into error > > message when an error is raised > > But what was the problem? I can read various binary sequences, but I'm > still not good at MessagePack. Could you please describe what exactly > was wrong prior to your fix? Well, I had to provide more detailed desciption in commit message. Our msgpack library doesn't support deconding/encoding of ext types until commit 8ae606 (00d770 commit adds this functionality in tarantool). As you can see from issue [1] \xd4 or \xd5 are leading to the error at decoding. Nevertheless they are valid msgpack keys that mean extension format family [2]. User should see a type of unsupported extension as 15 (\xf0). This bug is hidden at master because since 2.4.1 (commit 345877) we have additional error handler (luamp_decode_extension_box) that correctly decode extension type. The main idea of the patch is to cherry-pick commits e476ad1 and 8ae606a to separate msgpack branch for tarantool-1.10 (only them because we don't want to add a lot of commits to LTS version). Also we should cherry-pick commit 00d770 to latest tarantool 1.10 and after that add corresponding decoding of extension type. For versions higher than 1.10 (IIRC) we can just cherry-pick this patch and a test. > > > > Closes #5017 > > --- > > src/lua/msgpack.c | 5 +++-- > > test/unit/CMakeLists.txt | 5 +++++ > > test/unit/mplua.c | 47 ++++++++++++++++++++++++++++++++++++++++ > > test/unit/mplua.result | 5 +++++ > > 4 files changed, 60 insertions(+), 2 deletions(-) > > create mode 100644 test/unit/mplua.c > > create mode 100644 test/unit/mplua.result > > > > <snipped> > > > diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt > > index 6dfb10f46..906422e82 100644 > > --- a/test/unit/CMakeLists.txt > > +++ b/test/unit/CMakeLists.txt > > @@ -205,3 +205,8 @@ target_link_libraries(coll.test core unit ${ICU_LIBRARIES} misc) > > > > add_executable(tuple_bigref.test tuple_bigref.c) > > target_link_libraries(tuple_bigref.test tuple unit) > > + > > +add_executable(mplua.test mplua.c) > > +target_link_libraries(mplua.test unit box server core > > + ${CURL_LIBRARIES} ${LIBYAML_LIBRARIES} ${READLINE_LIBRARIES} > > + ${ICU_LIBRARIES} ${LUAJIT_LIBRARIES}) > > Why should this be done in such complex way? Do your test binary need > *all* mentioned libs? I see no usage for libcurl, libreadline, libicu, > libyaml in the test. Furthermore, IIRC LuaJIT symbols are provided by > libserver archive. When I had tried to build my test I found out that lua/msgpack.c requires build with tarantool_lua_ibuf (from lua/utils.c). | /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../x86_64-pc-linux-gnu/bin/ld: | CMakeFiles/mplua.test.dir/__/__/src/lua/msgpack.c.o: in function | `lua_msgpack_encode': msgpack.c:(.text+0x1673): undefined reference to | `tarantool_lua_ibuf' Building utils requires build with httpc_lua (from lua/httpc.c). And it requires curl library. | /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../x86_64-pc-linux-gnu/bin/ld: | CMakeFiles/mplua.test.dir/__/__/src/lua/init.c.o:(.data.rel+0x1b8): undefined | reference to `httpc_lua' And so on. I suppose that building with libserver and its linkage with necessary libraries would be less bulky than list all necessary sources inside add_executable. It would be nice if you will offer a prettier approach. Unfortunately libserver doesn't provides necessary symbols, but requires it to be built. | nm -g src/libserver.a | grep lua_type | sort | uniq | U lua_type | U lua_typename > > Well, I faced the following linkage error when the test is build in > Release mode: > | [ 87%] Linking CXX executable mplua.test > | /usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/../../../../x86_64-pc-linux-gnu/bin/ld: ../../src/libcore.a(evio.cc.o): in function `evio_service_bind(evio_service*, char const*)': > | evio.cc:(.text+0x9e2): undefined reference to `uri_parse' > | collect2: error: ld returned 1 exit status Hmm, I build successfully with Debug/RelWithDebInfo/Release mode locally. How did you get this error? Did you build from my branch or applied diff to master? > > diff --git a/test/unit/mplua.c b/test/unit/mplua.c > > new file mode 100644 > > index 000000000..3b7f4de76 > > --- /dev/null > > +++ b/test/unit/mplua.c > > @@ -0,0 +1,47 @@ > > +#include <lua.h> /* lua_*() */ > > +#include <lauxlib.h> /* luaL_*() */ > > Minor: Since we use bundled LuaJIT you need to lookup "local" headers > prior to the system-wide ones[1]. But I've run make with more verbose > output and seen we don't use "-I-" in preprocessor flags[2], so I see no > difference how to specify such includes (IMHO it's not the proper way): > | [100%] Building C object test/unit/CMakeFiles/mplua.test.dir/mplua.c.o > | cd /tarantool/test/unit && /usr/bin/cc -DCORO_ASM > | -DLUAJIT_SMART_STRINGS=1 -DLUA_USE_APICHECK=1 -DLUA_USE_ASSERT=1 > | -DNVALGRIND=1 -DYAML_DECLARE_STATIC -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE > | -D__STDC_CONSTANT_MACROS=1 -D__STDC_FORMAT_MACROS=1 > | -D__STDC_LIMIT_MACROS=1 -I/tarantool/src -I/tarantool/src/lib > | -I/tarantool/src/lib/small -I/tarantool/src/lib/small/third_party > | -I/tarantool -I/tarantool/third_party/zstd/lib > | -I/tarantool/third_party/zstd/lib/common > | -I/tarantool/third_party/luajit/src -I/tarantool/src/lib/msgpuck > | -I/tarantool/src/box -I/tarantool/third_party > | -I/tarantool/build/curl/dest/include > | -I/tarantool/third_party/libyaml/include -fexceptions -funwind-tables > | -fno-omit-frame-pointer -fno-stack-protector -fno-common -fopenmp -msse2 > | -std=c11 -Wall -Wextra -Wno-strict-aliasing -Wno-char-subscripts > | -Wno-format-truncation -fno-gnu89-inline -Wno-cast-function-type -Werror > | -Wno-unused-parameter -Wno-unused -Wno-unused-result > | -Wno-tautological-compare -g -ggdb -O0 -o > | CMakeFiles/mplua.test.dir/mplua.c.o -c /tarantool/test/unit/mplua.c It's very important comment! Of course I should use "" here. Thanks! > > +#include <lualib.h> /* luaL_openlibs() */ > > This include is excess. Thanks! Nice catch! > > +#include <string.h> /* memcmp() */ <snipped> > > + const char test_str[] = "\xd4\x0f\x00"; > > + const char *data = (char *)test_str; > > Why do you create a distinct pointer for to be passed to <luamp_decode>? > You can just properly declare <test_str>: > | const char *test_str = "\xd4\x0f\00"; > Ok! Sure! > > + luamp_decode(L, luaL_msgpack_default, &data); > > + return 0; > > +} > > + > > + > > +static void > > +test_luamp_encode_extension_default(struct lua_State *L) > > +{ > > + plan(2); > > + lua_pushcfunction(L, lua_encode_unknown_mp); > > + ok(lua_pcall(L, 0, 0, 0) != 0, > > You can check that exactly <LUA_ERRRUN> is yielded here. Thanks! Good idea! > > + "mplua_decode: unsupported extension raise error"); > > You can simply use <lua_cpcall> here. Ok! Thanks! > > + const char err_msg[] = "msgpack.decode: unsupported extension: 15"; > > + ok(memcmp(lua_tostring(L, -1), err_msg, sizeof(err_msg)) == 0, > > + "mplua_decode: unsupported extension correct type"); > > +} > > + > > + > > Well, it looks like you've mixed several approaches and it led to the > following mess: > * You set test plan right in the test case, but check it in the <main> > routine. I guess these calls should be placed within the same > function, otherwise they are hard maintained. > * There are lots of tests using header/footer but no one except > <base64.c> uses it right in <main>. I don't know, what these macros > are for, but most of the tests use them in the test case. > > The test case is really small and I guess you can move everything right > to the <main> as it's done in rlist.c Ok, I will move body of the test to the <main> directly. > Besides, the main question is why do you use C test instead of writing a > small Lua test (similar to the one in the issue)? At least you can avoid > all linkage issues. As I say above this error is hidden from lua at tarantool >= 2.4.1. So I suppose it would be nice to have one test to 1.10 and 2.4.1. > > +int > > +main(void) > > +{ > > + header(); > > + > > + struct lua_State *L = luaL_newstate(); > > + test_luamp_encode_extension_default(L); > > + > > + footer(); > > + check_plan(); > > The function should return <int> but returns nothing. All other tests > have the following idiom: > | return check_plan(); > It is correct if we are talking about master. But at 1.10 AFAIS test adhere to other way (I use for example test/unit/tuple_bigref.c as one of the oldest). Should I use approach from master here or I should use old code style here? > > +} > > <snipped> > > > -- > > 2.24.1 > > > > [1]: https://gcc.gnu.org/onlinedocs/gcc-10.1.0/cpp/Include-Syntax.html#Include-Syntax > [2]: https://gcc.gnu.org/onlinedocs/gcc-10.1.0/cpp/Invocation.html#Invocation > > -- > Best regards, > IM [1]: https://github.com/tarantool/tarantool/issues/5017 [2]: https://github.com/msgpack/msgpack/blob/master/spec.md#ext-format-family -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message 2020-07-15 9:32 ` Sergey Kaplun @ 2020-07-15 15:21 ` Sergey Kaplun 2020-07-16 20:53 ` Igor Munkin 1 sibling, 0 replies; 14+ messages in thread From: Sergey Kaplun @ 2020-07-15 15:21 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches, Alexander Turenko On 15.07.20, Sergey Kaplun wrote: > Hi Igor! Thanks for the review! <snipped> > > > +target_link_libraries(mplua.test unit box server core > > > + ${CURL_LIBRARIES} ${LIBYAML_LIBRARIES} ${READLINE_LIBRARIES} > > > + ${ICU_LIBRARIES} ${LUAJIT_LIBRARIES}) > > > > Why should this be done in such complex way? Do your test binary need > > *all* mentioned libs? I see no usage for libcurl, libreadline, libicu, > > libyaml in the test. Furthermore, IIRC LuaJIT symbols are provided by > > libserver archive. > > When I had tried to build my test I found out that lua/msgpack.c requires build > with tarantool_lua_ibuf (from lua/utils.c). > > | /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../x86_64-pc-linux-gnu/bin/ld: > | CMakeFiles/mplua.test.dir/__/__/src/lua/msgpack.c.o: in function > | `lua_msgpack_encode': msgpack.c:(.text+0x1673): undefined reference to > | `tarantool_lua_ibuf' > > Building utils requires build with httpc_lua (from lua/httpc.c). And it > requires curl library. > > | /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../x86_64-pc-linux-gnu/bin/ld: > | CMakeFiles/mplua.test.dir/__/__/src/lua/init.c.o:(.data.rel+0x1b8): undefined > | reference to `httpc_lua' > > And so on. I suppose that building with libserver and its linkage with > necessary libraries would be less bulky than list all necessary sources > inside add_executable. It would be nice if you will offer a prettier > approach. > > Unfortunately libserver doesn't provides necessary symbols, but requires it to > be built. > | nm -g src/libserver.a | grep lua_type | sort | uniq > | U lua_type > | U lua_typename > > > > > Well, I faced the following linkage error when the test is build in > > Release mode: > > | [ 87%] Linking CXX executable mplua.test > > | /usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/../../../../x86_64-pc-linux-gnu/bin/ld: ../../src/libcore.a(evio.cc.o): in function `evio_service_bind(evio_service*, char const*)': > > | evio.cc:(.text+0x9e2): undefined reference to `uri_parse' > > | collect2: error: ld returned 1 exit status > > Hmm, I build successfully with Debug/RelWithDebInfo/Release mode > locally. How did you get this error? Did you build from my branch or > applied diff to master? I can offer something like that: | target_link_libraries(mplua.test server box core unit) > > > diff --git a/test/unit/mplua.c b/test/unit/mplua.c <snipped> > > [1]: https://github.com/tarantool/tarantool/issues/5017 > [2]: https://github.com/msgpack/msgpack/blob/master/spec.md#ext-format-family > > -- > Best regards, > Sergey Kaplun -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message 2020-07-15 9:32 ` Sergey Kaplun 2020-07-15 15:21 ` Sergey Kaplun @ 2020-07-16 20:53 ` Igor Munkin 2020-07-17 8:35 ` Sergey Kaplun 1 sibling, 1 reply; 14+ messages in thread From: Igor Munkin @ 2020-07-16 20:53 UTC (permalink / raw) To: Sergey Kaplun; +Cc: tarantool-patches, Alexander Turenko Sergey, On 15.07.20, Sergey Kaplun wrote: > Hi Igor! Thanks for the review! > > On 14.07.20, Igor Munkin wrote: > > Sergey, > > > > Thanks for the patch! Please consider my comments below. > > > > On 15.06.20, Sergey Kaplun wrote: > > > This patch adds decoding of mp_ext type and inserts it into error > > > message when an error is raised > > > > But what was the problem? I can read various binary sequences, but I'm > > still not good at MessagePack. Could you please describe what exactly > > was wrong prior to your fix? > > Well, I had to provide more detailed desciption in commit message. > Our msgpack library doesn't support deconding/encoding of ext types > until commit 8ae606 (00d770 commit adds this functionality in > tarantool). As you can see from issue [1] \xd4 or \xd5 are leading to > the error at decoding. Nevertheless they are valid msgpack keys that > mean extension format family [2]. User should see a type of > unsupported extension as 15 (\xf0). Great, this part should be polished and added to the commit message. > > This bug is hidden at master because since 2.4.1 (commit 345877) we have > additional error handler (luamp_decode_extension_box) that correctly decode > extension type. So it's not hidden, but handled and the issue doesn't occur since 2.4.1. > > The main idea of the patch is to cherry-pick commits e476ad1 and 8ae606a > to separate msgpack branch for tarantool-1.10 (only them because we > don't want to add a lot of commits to LTS version). Also we should > cherry-pick commit 00d770 to latest tarantool 1.10 and after that add > corresponding decoding of extension type. For versions higher than 1.10 > (IIRC) we can just cherry-pick this patch and a test. As you mentioned above, the issue is gone (or masked, whatever) in all versions above 2.4.1. AFAIR everything except these versions and LTS will be deprecated in a few days. So you need to fix the issue only for 1.10 and nothing to be cherry-picked, right? > > > > > > > Closes #5017 > > > --- > > > src/lua/msgpack.c | 5 +++-- > > > test/unit/CMakeLists.txt | 5 +++++ > > > test/unit/mplua.c | 47 ++++++++++++++++++++++++++++++++++++++++ > > > test/unit/mplua.result | 5 +++++ > > > 4 files changed, 60 insertions(+), 2 deletions(-) > > > create mode 100644 test/unit/mplua.c > > > create mode 100644 test/unit/mplua.result > > > > > > > <snipped> > > > > > diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt > > > index 6dfb10f46..906422e82 100644 > > > --- a/test/unit/CMakeLists.txt > > > +++ b/test/unit/CMakeLists.txt > > > @@ -205,3 +205,8 @@ target_link_libraries(coll.test core unit ${ICU_LIBRARIES} misc) > > > > > > add_executable(tuple_bigref.test tuple_bigref.c) > > > target_link_libraries(tuple_bigref.test tuple unit) > > > + > > > +add_executable(mplua.test mplua.c) > > > +target_link_libraries(mplua.test unit box server core > > > + ${CURL_LIBRARIES} ${LIBYAML_LIBRARIES} ${READLINE_LIBRARIES} > > > + ${ICU_LIBRARIES} ${LUAJIT_LIBRARIES}) > > > > Why should this be done in such complex way? Do your test binary need > > *all* mentioned libs? I see no usage for libcurl, libreadline, libicu, > > libyaml in the test. Furthermore, IIRC LuaJIT symbols are provided by > > libserver archive. > > When I had tried to build my test I found out that lua/msgpack.c requires build > with tarantool_lua_ibuf (from lua/utils.c). > > | /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../x86_64-pc-linux-gnu/bin/ld: > | CMakeFiles/mplua.test.dir/__/__/src/lua/msgpack.c.o: in function > | `lua_msgpack_encode': msgpack.c:(.text+0x1673): undefined reference to > | `tarantool_lua_ibuf' > > Building utils requires build with httpc_lua (from lua/httpc.c). And it > requires curl library. > > | /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../x86_64-pc-linux-gnu/bin/ld: > | CMakeFiles/mplua.test.dir/__/__/src/lua/init.c.o:(.data.rel+0x1b8): undefined > | reference to `httpc_lua' > > And so on. I suppose that building with libserver and its linkage with > necessary libraries would be less bulky than list all necessary sources > inside add_executable. It would be nice if you will offer a prettier > approach. > > Unfortunately libserver doesn't provides necessary symbols, but requires it to > be built. > | nm -g src/libserver.a | grep lua_type | sort | uniq > | U lua_type > | U lua_typename > > > > > Well, I faced the following linkage error when the test is build in > > Release mode: > > | [ 87%] Linking CXX executable mplua.test > > | /usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/../../../../x86_64-pc-linux-gnu/bin/ld: ../../src/libcore.a(evio.cc.o): in function `evio_service_bind(evio_service*, char const*)': > > | evio.cc:(.text+0x9e2): undefined reference to `uri_parse' > > | collect2: error: ld returned 1 exit status > > Hmm, I build successfully with Debug/RelWithDebInfo/Release mode > locally. How did you get this error? Did you build from my branch or > applied diff to master? My bad. I tried to reduce the amount of the required libs myself and get the list quite similar to the one you've mentioned within your next reply. Does it successfully work for all build modes? > > > > diff --git a/test/unit/mplua.c b/test/unit/mplua.c > > > new file mode 100644 > > > index 000000000..3b7f4de76 > > > --- /dev/null > > > +++ b/test/unit/mplua.c <snipped> > > > + const char err_msg[] = "msgpack.decode: unsupported extension: 15"; > > > + ok(memcmp(lua_tostring(L, -1), err_msg, sizeof(err_msg)) == 0, > > > + "mplua_decode: unsupported extension correct type"); > > > +} > > > + > > > + > > > > Well, it looks like you've mixed several approaches and it led to the > > following mess: > > * You set test plan right in the test case, but check it in the <main> > > routine. I guess these calls should be placed within the same > > function, otherwise they are hard maintained. > > * There are lots of tests using header/footer but no one except > > <base64.c> uses it right in <main>. I don't know, what these macros > > are for, but most of the tests use them in the test case. > > > > The test case is really small and I guess you can move everything right > > to the <main> as it's done in rlist.c > > Ok, I will move body of the test to the <main> directly. > > > Besides, the main question is why do you use C test instead of writing a > > small Lua test (similar to the one in the issue)? At least you can avoid > > all linkage issues. > > As I say above this error is hidden from lua at tarantool >= 2.4.1. So > I suppose it would be nice to have one test to 1.10 and 2.4.1. Well, I still don't get the reason for it. You can simply check your patch via a tiny Lua chunk for 1.10, otherwise the patch and the test are not necessary for the reasons I mentioned above. However, if other reviewers insist on the test implemented in C, please adjust it considering the comments I left in my previous reply. > > > > +int > > > +main(void) > > > +{ > > > + header(); > > > + > > > + struct lua_State *L = luaL_newstate(); > > > + test_luamp_encode_extension_default(L); > > > + > > > + footer(); > > > + check_plan(); > > > > The function should return <int> but returns nothing. All other tests > > have the following idiom: > > | return check_plan(); > > > > It is correct if we are talking about master. But at 1.10 AFAIS test > adhere to other way (I use for example test/unit/tuple_bigref.c as one > of the oldest). Should I use approach from master here or I should use > old code style here? As we discussed offline, the most of the tests (23/58) in the unit directory use this idiom (checked on 1.10). So if you finally decide to leave the test implemented in C, please adjust it to the common idiom. > > > > +} > > > > <snipped> > > > > > -- > > > 2.24.1 > > > > > > > [1]: https://gcc.gnu.org/onlinedocs/gcc-10.1.0/cpp/Include-Syntax.html#Include-Syntax > > [2]: https://gcc.gnu.org/onlinedocs/gcc-10.1.0/cpp/Invocation.html#Invocation > > > > -- > > Best regards, > > IM > > [1]: https://github.com/tarantool/tarantool/issues/5017 > [2]: https://github.com/msgpack/msgpack/blob/master/spec.md#ext-format-family > > -- > Best regards, > Sergey Kaplun -- Best regards, IM ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message 2020-07-16 20:53 ` Igor Munkin @ 2020-07-17 8:35 ` Sergey Kaplun 2020-07-17 9:09 ` Alexander Turenko ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Sergey Kaplun @ 2020-07-17 8:35 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches, Alexander Turenko Igor, On 16.07.20, Igor Munkin wrote: > Sergey, > > On 15.07.20, Sergey Kaplun wrote: <snipped> > > > > This bug is hidden at master because since 2.4.1 (commit 345877) we have > > additional error handler (luamp_decode_extension_box) that correctly decode > > extension type. > > So it's not hidden, but handled and the issue doesn't occur since 2.4.1. AFAIK, yes. > > > > > The main idea of the patch is to cherry-pick commits e476ad1 and 8ae606a > > to separate msgpack branch for tarantool-1.10 (only them because we > > don't want to add a lot of commits to LTS version). Also we should > > cherry-pick commit 00d770 to latest tarantool 1.10 and after that add > > corresponding decoding of extension type. For versions higher than 1.10 > > (IIRC) we can just cherry-pick this patch and a test. > > As you mentioned above, the issue is gone (or masked, whatever) in all > versions above 2.4.1. AFAIR everything except these versions and LTS > will be deprecated in a few days. So you need to fix the issue only for > 1.10 and nothing to be cherry-picked, right? Hmm, I suppose that it will be enough to apply this patch only to 1.10. In that way we don't need C-implemented test. > <snipped> > > My bad. I tried to reduce the amount of the required libs myself and get > the list quite similar to the one you've mentioned within your next > reply. Does it successfully work for all build modes? Yes, it does. <snipped> > > Well, I still don't get the reason for it. You can simply check your > patch via a tiny Lua chunk for 1.10, otherwise the patch and the test > are not necessary for the reasons I mentioned above. I rewrote test to simple lua chunk (see it below). > > However, if other reviewers insist on the test implemented in C, please > adjust it considering the comments I left in my previous reply. > <snipped> > > -- > Best regards, > IM msgpack: fix wrong mp_ext type in an error message While decoding msgpack's extension tarantool raises error as far as it doesn't support extension types. Error contains message about wrong extension type with wrong byte (the first instead of the second). This patch adds decoding of mp_ext type and inserts it into error message when error is raised. Closes #5017 --- src/lua/msgpack.c | 5 +++-- test/app-tap/msgpack.test.lua | 5 ++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c index b7a2955fe..e3935d383 100644 --- a/src/lua/msgpack.c +++ b/src/lua/msgpack.c @@ -224,8 +224,9 @@ luamp_set_encode_extension(luamp_encode_extension_f handler) static void luamp_decode_extension_default(struct lua_State *L, const char **data) { - luaL_error(L, "msgpack.decode: unsupported extension: %u", - (unsigned char) **data); + int8_t ext_type; + mp_decode_extl(data, &ext_type); + luaL_error(L, "msgpack.decode: unsupported extension: %u", ext_type); } diff --git a/test/app-tap/msgpack.test.lua b/test/app-tap/msgpack.test.lua index 6f7b46a15..1f3904837 100755 --- a/test/app-tap/msgpack.test.lua +++ b/test/app-tap/msgpack.test.lua @@ -36,7 +36,7 @@ local function test_offsets(test, s) end local function test_misc(test, s) - test:plan(4) + test:plan(5) local ffi = require('ffi') local buffer = require('buffer') local buf = ffi.cast("const char *", "\x91\x01") @@ -47,6 +47,9 @@ local function test_misc(test, s) test:is(buf + 2, bufend, 'ibuf_decode position') test:is_deeply(result, {1}, "ibuf_decode result") test:ok(not st and e:match("null"), "null ibuf") + st, e = pcall(s.decode, "\xd4\x0f\x00") + test:ok(not st and e:match("unsupported extension: 15"), + "unsupported extension decode") end tap.test("msgpack", function(test) -- 2.24.1 -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message 2020-07-17 8:35 ` Sergey Kaplun @ 2020-07-17 9:09 ` Alexander Turenko 2020-07-17 9:54 ` Sergey Kaplun 2020-07-17 9:23 ` sergos 2020-07-17 10:12 ` Igor Munkin 2 siblings, 1 reply; 14+ messages in thread From: Alexander Turenko @ 2020-07-17 9:09 UTC (permalink / raw) To: Sergey Kaplun; +Cc: tarantool-patches > > Well, I still don't get the reason for it. You can simply check your > > patch via a tiny Lua chunk for 1.10, otherwise the patch and the test > > are not necessary for the reasons I mentioned above. > > I rewrote test to simple lua chunk (see it below). +1 here, it is much simpler. The Lua-written test does not fail on 2.4, but it is not a problem: in fact 2.4 is not affected by the problem (from a user point of view). Re how to push: I would anyway push it from master downward, because the fixed code may become alive (I mean, not dead) in a future and we'll have the correct behaviour and the test. It is better than left it incorrect just because it is dead in fact. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message 2020-07-17 9:09 ` Alexander Turenko @ 2020-07-17 9:54 ` Sergey Kaplun 0 siblings, 0 replies; 14+ messages in thread From: Sergey Kaplun @ 2020-07-17 9:54 UTC (permalink / raw) To: Alexander Turenko; +Cc: tarantool-patches On 17.07.20, Alexander Turenko wrote: > > > Well, I still don't get the reason for it. You can simply check your > > > patch via a tiny Lua chunk for 1.10, otherwise the patch and the test > > > are not necessary for the reasons I mentioned above. > > > > I rewrote test to simple lua chunk (see it below). > > +1 here, it is much simpler. > > The Lua-written test does not fail on 2.4, but it is not a problem: in > fact 2.4 is not affected by the problem (from a user point of view). > > Re how to push: I would anyway push it from master downward, because the > fixed code may become alive (I mean, not dead) in a future and we'll > have the correct behaviour and the test. It is better than left it > incorrect just because it is dead in fact. We have a little bit different format of error at master ("Unsuported MsgPack extension type: %u" vs "msgpack.decode: unsupported extension: %u"). So the test will fail at master. Should I make some changes in the test? Should I create branch from master directly (this branch is checked out from 1.10)? -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message 2020-07-17 8:35 ` Sergey Kaplun 2020-07-17 9:09 ` Alexander Turenko @ 2020-07-17 9:23 ` sergos 2020-07-17 10:12 ` Igor Munkin 2 siblings, 0 replies; 14+ messages in thread From: sergos @ 2020-07-17 9:23 UTC (permalink / raw) To: Sergey Kaplun; +Cc: Alexander Turenko, tarantool-patches Hi! Thanks for the new patch, LGTM. I support Alexander Turenko in his contribution path: master + all supported versions. Sergos > On 17 Jul 2020, at 11:35, Sergey Kaplun <skaplun@tarantool.org> wrote: > > Igor, > > On 16.07.20, Igor Munkin wrote: >> Sergey, >> >> On 15.07.20, Sergey Kaplun wrote: > > <snipped> > >>> >>> This bug is hidden at master because since 2.4.1 (commit 345877) we have >>> additional error handler (luamp_decode_extension_box) that correctly decode >>> extension type. >> >> So it's not hidden, but handled and the issue doesn't occur since 2.4.1. > > AFAIK, yes. > >> >>> >>> The main idea of the patch is to cherry-pick commits e476ad1 and 8ae606a >>> to separate msgpack branch for tarantool-1.10 (only them because we >>> don't want to add a lot of commits to LTS version). Also we should >>> cherry-pick commit 00d770 to latest tarantool 1.10 and after that add >>> corresponding decoding of extension type. For versions higher than 1.10 >>> (IIRC) we can just cherry-pick this patch and a test. >> >> As you mentioned above, the issue is gone (or masked, whatever) in all >> versions above 2.4.1. AFAIR everything except these versions and LTS >> will be deprecated in a few days. So you need to fix the issue only for >> 1.10 and nothing to be cherry-picked, right? > > Hmm, I suppose that it will be enough to apply this patch only to 1.10. > In that way we don't need C-implemented test. > >> > <snipped> >> >> My bad. I tried to reduce the amount of the required libs myself and get >> the list quite similar to the one you've mentioned within your next >> reply. Does it successfully work for all build modes? > > Yes, it does. > > <snipped> > >> >> Well, I still don't get the reason for it. You can simply check your >> patch via a tiny Lua chunk for 1.10, otherwise the patch and the test >> are not necessary for the reasons I mentioned above. > > I rewrote test to simple lua chunk (see it below). > >> >> However, if other reviewers insist on the test implemented in C, please >> adjust it considering the comments I left in my previous reply. >> > > <snipped> > >> >> -- >> Best regards, >> IM > > msgpack: fix wrong mp_ext type in an error message > > While decoding msgpack's extension tarantool raises error as far as it > doesn't support extension types. Error contains message about wrong > extension type with wrong byte (the first instead of the second). > > This patch adds decoding of mp_ext type and inserts it into error > message when error is raised. > > Closes #5017 > --- > src/lua/msgpack.c | 5 +++-- > test/app-tap/msgpack.test.lua | 5 ++++- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c > index b7a2955fe..e3935d383 100644 > --- a/src/lua/msgpack.c > +++ b/src/lua/msgpack.c > @@ -224,8 +224,9 @@ luamp_set_encode_extension(luamp_encode_extension_f handler) > static void > luamp_decode_extension_default(struct lua_State *L, const char **data) > { > - luaL_error(L, "msgpack.decode: unsupported extension: %u", > - (unsigned char) **data); > + int8_t ext_type; > + mp_decode_extl(data, &ext_type); > + luaL_error(L, "msgpack.decode: unsupported extension: %u", ext_type); > } > > > diff --git a/test/app-tap/msgpack.test.lua b/test/app-tap/msgpack.test.lua > index 6f7b46a15..1f3904837 100755 > --- a/test/app-tap/msgpack.test.lua > +++ b/test/app-tap/msgpack.test.lua > @@ -36,7 +36,7 @@ local function test_offsets(test, s) > end > > local function test_misc(test, s) > - test:plan(4) > + test:plan(5) > local ffi = require('ffi') > local buffer = require('buffer') > local buf = ffi.cast("const char *", "\x91\x01") > @@ -47,6 +47,9 @@ local function test_misc(test, s) > test:is(buf + 2, bufend, 'ibuf_decode position') > test:is_deeply(result, {1}, "ibuf_decode result") > test:ok(not st and e:match("null"), "null ibuf") > + st, e = pcall(s.decode, "\xd4\x0f\x00") > + test:ok(not st and e:match("unsupported extension: 15"), > + "unsupported extension decode") > end > > tap.test("msgpack", function(test) > -- > 2.24.1 > > -- > Best regards, > Sergey Kaplun ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message 2020-07-17 8:35 ` Sergey Kaplun 2020-07-17 9:09 ` Alexander Turenko 2020-07-17 9:23 ` sergos @ 2020-07-17 10:12 ` Igor Munkin 2020-07-17 11:02 ` Sergey Kaplun 2 siblings, 1 reply; 14+ messages in thread From: Igor Munkin @ 2020-07-17 10:12 UTC (permalink / raw) To: Sergey Kaplun; +Cc: tarantool-patches, Alexander Turenko Sergey, Thanks for the fixes you made! The patch LGTM now, except a single nit regarding the commit message. On 17.07.20, Sergey Kaplun wrote: > Igor, > > On 16.07.20, Igor Munkin wrote: > > Sergey, > > > > On 15.07.20, Sergey Kaplun wrote: <snipped> > > > > > > The main idea of the patch is to cherry-pick commits e476ad1 and 8ae606a > > > to separate msgpack branch for tarantool-1.10 (only them because we > > > don't want to add a lot of commits to LTS version). Also we should > > > cherry-pick commit 00d770 to latest tarantool 1.10 and after that add > > > corresponding decoding of extension type. For versions higher than 1.10 > > > (IIRC) we can just cherry-pick this patch and a test. > > > > As you mentioned above, the issue is gone (or masked, whatever) in all > > versions above 2.4.1. AFAIR everything except these versions and LTS > > will be deprecated in a few days. So you need to fix the issue only for > > 1.10 and nothing to be cherry-picked, right? > > Hmm, I suppose that it will be enough to apply this patch only to 1.10. > In that way we don't need C-implemented test. I see other reviewers have mentioned in their replies that it's worth to apply the patch for all supported versions. Since the output differs, please create two different branches to ease the merge procedure: one to be applied to LTS branch and another one -- for bleeding and active 2.x stable branches. AFAICS, only the error message need to be adjusted. > <snipped> > > msgpack: fix wrong mp_ext type in an error message > > While decoding msgpack's extension tarantool raises error as far as it > doesn't support extension types. Error contains message about wrong > extension type with wrong byte (the first instead of the second). > > This patch adds decoding of mp_ext type and inserts it into error > message when error is raised. I reworded the commit message to make it a bit clearer: | Since Tarantool doesn't support msgpack extension types, the error | occurs while its decoding. The error message is misleading, since | the wrong byte is used for representing the extension type. | | The patch adds mp_ext type decoding and the correct type value is used | in the message when the error is raised. Feel free to change it on your own. > > Closes #5017 > --- > src/lua/msgpack.c | 5 +++-- > test/app-tap/msgpack.test.lua | 5 ++++- > 2 files changed, 7 insertions(+), 3 deletions(-) > <snipped> > -- > 2.24.1 > > -- > Best regards, > Sergey Kaplun -- Best regards, IM ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message 2020-07-17 10:12 ` Igor Munkin @ 2020-07-17 11:02 ` Sergey Kaplun 0 siblings, 0 replies; 14+ messages in thread From: Sergey Kaplun @ 2020-07-17 11:02 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches, Alexander Turenko Igor, On 17.07.20, Igor Munkin wrote: > Sergey, > > Thanks for the fixes you made! The patch LGTM now, except a single nit > regarding the commit message. > > On 17.07.20, Sergey Kaplun wrote: > > Igor, > > > > On 16.07.20, Igor Munkin wrote: > > > Sergey, > > > > > > On 15.07.20, Sergey Kaplun wrote: > > <snipped> > > > > > > > > > The main idea of the patch is to cherry-pick commits e476ad1 and 8ae606a > > > > to separate msgpack branch for tarantool-1.10 (only them because we > > > > don't want to add a lot of commits to LTS version). Also we should > > > > cherry-pick commit 00d770 to latest tarantool 1.10 and after that add > > > > corresponding decoding of extension type. For versions higher than 1.10 > > > > (IIRC) we can just cherry-pick this patch and a test. > > > > > > As you mentioned above, the issue is gone (or masked, whatever) in all > > > versions above 2.4.1. AFAIR everything except these versions and LTS > > > will be deprecated in a few days. So you need to fix the issue only for > > > 1.10 and nothing to be cherry-picked, right? > > > > Hmm, I suppose that it will be enough to apply this patch only to 1.10. > > In that way we don't need C-implemented test. > > I see other reviewers have mentioned in their replies that it's worth to > apply the patch for all supported versions. Since the output differs, > please create two different branches to ease the merge procedure: one to > be applied to LTS branch and another one -- for bleeding and active 2.x > stable branches. AFAICS, only the error message need to be adjusted. > I added additional branch (see corresponding diff with ChangeLog below). > > > > <snipped> > > > > > msgpack: fix wrong mp_ext type in an error message > > > > While decoding msgpack's extension tarantool raises error as far as it > > doesn't support extension types. Error contains message about wrong > > extension type with wrong byte (the first instead of the second). > > > > This patch adds decoding of mp_ext type and inserts it into error > > message when error is raised. > > I reworded the commit message to make it a bit clearer: > | Since Tarantool doesn't support msgpack extension types, the error > | occurs while its decoding. The error message is misleading, since > | the wrong byte is used for representing the extension type. > | > | The patch adds mp_ext type decoding and the correct type value is used > | in the message when the error is raised. > > Feel free to change it on your own. Thanks for your comment! I've changed the commit message for both branches (see it below). > > > > > Closes #5017 > > --- > > src/lua/msgpack.c | 5 +++-- > > test/app-tap/msgpack.test.lua | 5 ++++- > > 2 files changed, 7 insertions(+), 3 deletions(-) > > > > <snipped> > > > -- > > 2.24.1 > > > > -- > > Best regards, > > Sergey Kaplun > > -- > Best regards, > IM msgpack: fix wrong mp_ext type in an error message Since Tarantool doesn't support msgpack extension types, the error occurs while its decoding by default. The error message is misleading, since the wrong byte is used for representing the extension type. The patch adds mp_ext type decoding and the correct type value is used in the message when the error is raised. Closes #5017 --- Branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5017-msgpack-wrong-ext-type-master @ChangeLog: * Fix wrong mpsgpack extension type in an error message at decoding (gh-5017). src/lua/msgpack.c | 5 +++-- test/app-tap/msgpack.test.lua | 5 ++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c index d482c9b9a..128cc5d2f 100644 --- a/src/lua/msgpack.c +++ b/src/lua/msgpack.c @@ -92,8 +92,9 @@ luamp_set_encode_extension(luamp_encode_extension_f handler) static void luamp_decode_extension_default(struct lua_State *L, const char **data) { - luaL_error(L, "msgpack.decode: unsupported extension: %u", - (unsigned char) **data); + int8_t ext_type; + mp_decode_extl(data, &ext_type); + luaL_error(L, "msgpack.decode: unsupported extension: %u", ext_type); } diff --git a/test/app-tap/msgpack.test.lua b/test/app-tap/msgpack.test.lua index 1df9d2372..2d62850fb 100755 --- a/test/app-tap/msgpack.test.lua +++ b/test/app-tap/msgpack.test.lua @@ -36,7 +36,7 @@ local function test_offsets(test, s) end local function test_misc(test, s) - test:plan(4) + test:plan(5) local ffi = require('ffi') local buffer = require('buffer') local buf = ffi.cast("const char *", "\x91\x01") @@ -47,6 +47,9 @@ local function test_misc(test, s) test:is(buf + 2, bufend, 'ibuf_decode position') test:is_deeply(result, {1}, "ibuf_decode result") test:ok(not st and e:match("null"), "null ibuf") + st, e = pcall(s.decode, "\xd4\x0f\x00") + test:ok(not st and e:match("Unsuported MsgPack extension type: 15"), + "unsupported extension decode") end local function test_decode_array_map_header(test, s) -- 2.24.1 -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/2] Msgpack wrong extension type in error message 2020-06-15 15:56 [Tarantool-patches] [PATCH 0/2] Msgpack wrong extension type in error message Sergey Kaplun 2020-06-15 15:56 ` [Tarantool-patches] [PATCH 1/2] lib: update msgpuck library Sergey Kaplun 2020-06-15 15:56 ` [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message Sergey Kaplun @ 2020-07-17 11:27 ` Kirill Yukhin 2 siblings, 0 replies; 14+ messages in thread From: Kirill Yukhin @ 2020-07-17 11:27 UTC (permalink / raw) To: Sergey Kaplun; +Cc: tarantool-patches, Alexander Turenko Hello, On 15 июн 18:56, Sergey Kaplun wrote: > This patch updates msgpucks library to be able to support encoding of > extension type for tarantool version 1.10-2.2 and fixes incorrect > extension type inside error message at msgpack decoding inside default > handler. > > Branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5017-msgpack-wrong-ext-type > Issue: https://github.com/tarantool/tarantool/issues/5017 I've checked your patchset into 1.10, 2.3, 2.4 and master. -- Regards, Kirill Yukhin ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-07-17 11:27 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-15 15:56 [Tarantool-patches] [PATCH 0/2] Msgpack wrong extension type in error message Sergey Kaplun 2020-06-15 15:56 ` [Tarantool-patches] [PATCH 1/2] lib: update msgpuck library Sergey Kaplun 2020-06-15 15:56 ` [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message Sergey Kaplun 2020-07-14 15:46 ` Igor Munkin 2020-07-15 9:32 ` Sergey Kaplun 2020-07-15 15:21 ` Sergey Kaplun 2020-07-16 20:53 ` Igor Munkin 2020-07-17 8:35 ` Sergey Kaplun 2020-07-17 9:09 ` Alexander Turenko 2020-07-17 9:54 ` Sergey Kaplun 2020-07-17 9:23 ` sergos 2020-07-17 10:12 ` Igor Munkin 2020-07-17 11:02 ` Sergey Kaplun 2020-07-17 11:27 ` [Tarantool-patches] [PATCH 0/2] Msgpack wrong extension " Kirill Yukhin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox