[Tarantool-patches] [PATCH 0/2] Msgpack wrong extension type in error message

[Tarantool-patches] [PATCH 0/2] Msgpack wrong extension type in error message

[Sergey Kaplun]

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

[Tarantool-patches] [PATCH 1/2] lib: update msgpuck library

[Sergey Kaplun]

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

[Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message

[Sergey Kaplun]

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

Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message

[Igor Munkin]

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

Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message

[Sergey Kaplun]

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

Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message

[Sergey Kaplun]

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

Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message

[Igor Munkin]

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

Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message

[Sergey Kaplun]

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

Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message

[Alexander Turenko]

> > 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.

Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message

[sergos]

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

Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message

[Sergey Kaplun]

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

Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message

[Igor Munkin]

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

Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message

[Sergey Kaplun]

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

Re: [Tarantool-patches] [PATCH 0/2] Msgpack wrong extension type in error message

[Kirill Yukhin]

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