A fix for gh-206: check for mandatory keys in request body.

A server could crash if some of the request fields were missing.
Check in the request parser for presence of all mandatory fields.
Add a test case from the original fir fox gh-206 by @roman

src/box/lua/box_net.lua

@@ -436,6 +436,10 @@ box.net.box.new = function(host, port, reconnect_timeout)
                 return
             end
             local blen = self.s:recv(5)
+            if string.len(blen) < 5 then
+                self:fatal("The server has closed connection")
+                return
+            end
             blen = msgpack.decode(blen)
 
             local body = ''
@@ -469,6 +473,9 @@ box.net.box.new = function(host, port, reconnect_timeout)
 
                 while not self.closed do
                     local resp = self:read_response()
+                    if resp == nil or string.len(resp) == 0 then
+                        break
+                    end
                     local header, offset = msgpack.decode(resp);
                     local code = header[box.net.box.TYPE]
                     local sync = header[box.net.box.SYNC]

src/box/request.cc

@@ -250,6 +250,7 @@ request_decode(struct request *request, const char *data, uint32_t len)
 {
 	assert(request->execute != NULL);
 	const char *end = data + len;
+	uint64_t key_map = iproto_body_key_map[request->type];
 
 	if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
 error:
@@ -263,6 +264,7 @@ request_decode(struct request *request, const char *data, uint32_t len)
 			continue;
 		}
 		unsigned char key = mp_decode_uint(&data);
+		key_map &= ~iproto_key_bit(key);
 		const char *value = data;
 		if (mp_check(&data, end))
 			goto error;
@@ -301,6 +303,10 @@ request_decode(struct request *request, const char *data, uint32_t len)
 	if (data != end)
 		tnt_raise(ClientError, ER_INVALID_MSGPACK, "packet end");
 #endif
+	if (key_map) {
+		tnt_raise(ClientError, ER_MISSING_REQUEST_FIELD,
+			  iproto_key_strs[__builtin_ffsll((long long) key_map) - 1]);
+	  }
 }
 
 int

src/errcode.h

@@ -118,6 +118,7 @@ enum { TNT_ERRMSG_MAX = 512 };
 	/* 66 */_(ER_NODE_ID_IS_RO,		2, "Can't reset node id") \
 	/* 67 */_(ER_NODE_ID_IS_RESERVED,	2, "Can't initialize node id with a reserved value %u") \
 	/* 68 */_(ER_INVALID_ORDER,		2, "Invalid LSN order for node %u: prev lsn = %llu, new lsn = %llu") \
+	/* 69 */_(ER_MISSING_REQUEST_FIELD,	2, "Missing mandatory field '%s' in request") \
 
 /*
  * !IMPORTANT! Please follow instructions at start of the file

src/iproto_constants.cc

@@ -101,6 +101,61 @@ const char *iproto_request_type_strs[] =
 	"AUTH"
 };
 
+#define bit(c) (1ULL<<IPROTO_##c)
+const uint64_t iproto_body_key_map[IPROTO_DML_REQUEST_MAX + 1] = {
+	0,                                                     /* unused */
+	bit(SPACE_ID) | bit(LIMIT) | bit(KEY),                 /* SELECT */
+	bit(SPACE_ID) | bit(TUPLE),                            /* INSERT */
+	bit(SPACE_ID) | bit(TUPLE),                            /* REPLACE */
+	bit(SPACE_ID) | bit(KEY) | bit(TUPLE),                 /* UPDATE */
+	bit(SPACE_ID) | bit(KEY),                              /* DELETE */
+	bit(FUNCTION_NAME) | bit(TUPLE),                       /* CALL */
+	bit(USER_NAME) | bit(TUPLE)                            /* AUTH */
+};
+#undef bit
+
+const char *iproto_key_strs[IPROTO_KEY_MAX] = {
+	"type",             /* 0x00 */
+	"sync",             /* 0x01 */
+	"node_id",          /* 0x02 */
+	"lsn",              /* 0x03 */
+	"timestamp",        /* 0x04 */
+	"",                 /* 0x05 */
+	"",                 /* 0x06 */
+	"",                 /* 0x07 */
+	"",                 /* 0x08 */
+	"",                 /* 0x09 */
+	"",                 /* 0x0a */
+	"",                 /* 0x0b */
+	"",                 /* 0x0c */
+	"",                 /* 0x0d */
+	"",                 /* 0x0e */
+	"",                 /* 0x0f */
+	"space_id",         /* 0x10 */
+	"index_id",         /* 0x11 */
+	"limit",            /* 0x12 */
+	"offset",           /* 0x13 */
+	"iterator",         /* 0x14 */
+	"",                 /* 0x15 */
+	"",                 /* 0x16 */
+	"",                 /* 0x17 */
+	"",                 /* 0x18 */
+	"",                 /* 0x19 */
+	"",                 /* 0x1a */
+	"",                 /* 0x1b */
+	"",                 /* 0x1c */
+	"",                 /* 0x1d */
+	"",                 /* 0x1e */
+	"",                 /* 0x1f */
+	"key",              /* 0x20 */
+	"tuple",            /* 0x21 */
+	"function name",    /* 0x22 */
+	"user name",        /* 0x23 */
+	"node uuid"         /* 0x24 */
+	"cluster uuid"      /* 0x25 */
+	"lsn map"           /* 0x26 */
+};
+
 void
 iproto_header_decode(struct iproto_header *header, const char **pos,
 		     const char *end)

src/iproto_constants.h

@@ -97,6 +97,12 @@ iproto_body_has_key(const char *pos, const char *end)
 
 #undef bit
 
+static inline uint64_t
+iproto_key_bit(unsigned char key)
+{
+	return 1ULL << key;
+}
+
 extern const unsigned char iproto_key_type[IPROTO_KEY_MAX];
 
 enum iproto_request_type {
@@ -115,6 +121,10 @@ enum iproto_request_type {
 };
 
 extern const char *iproto_request_type_strs[];
+/** Key names. */
+extern const char *iproto_key_strs[];
+/** A map of mandatory members of an iproto DML request. */
+extern const uint64_t iproto_body_key_map[];
 
 static inline const char *
 iproto_request_name(uint32_t type)

test/box/iproto.result

@@ -13,3 +13,56 @@ ping
 ---
 - ok
 ...
+
+#  Test gh-206 "Segfault if sending IPROTO package without `KEY` field"
+
+IPROTO_SELECT
+query {'IPROTO_CODE': 1} {'IPROTO_SPACE_ID': 280}
+ping
+---
+- ok
+...
+
+
+IPROTO_DELETE
+query {'IPROTO_CODE': 5} {'IPROTO_SPACE_ID': 280}
+ping
+---
+- ok
+...
+
+
+IPROTO_UPDATE
+query {'IPROTO_CODE': 4} {'IPROTO_SPACE_ID': 280}
+ping
+---
+- ok
+...
+query {'IPROTO_CODE': 4} {'IPROTO_SPACE_ID': 280, 'IPROTO_KEY': (1,)}
+ping
+---
+- ok
+...
+
+
+IPROTO_REPLACE
+query {'IPROTO_CODE': 3} {'IPROTO_SPACE_ID': 280}
+ping
+---
+- ok
+...
+
+
+IPROTO_CALL
+query {'IPROTO_CODE': 6} {}
+ping
+---
+- ok
+...
+query {'IPROTO_CODE': 6} {'IPROTO_KEY': ('procname',)}
+ping
+---
+- ok
+...
+
+

test/box/iproto.test.py

@@ -2,6 +2,9 @@ import os
 import sys
 import struct
 import socket
+import msgpack
+from tarantool.const import *
+from tarantool import Connection
 
 print """
 #
@@ -24,3 +27,59 @@ sql("ping")
 
 # closing connection
 s.close()
+
+key_names = {}
+for (k,v) in globals().items():
+    if type(k) == str and k.startswith('IPROTO_') and type(v) == int:
+        key_names[v] = k
+
+def repr_dict(todump):
+    d = {}
+    for (k, v) in todump.items():
+        k_name = key_names.get(k, k)
+        d[k_name] = v
+    return repr(d)
+
+def test(header, body):
+    # Connect and authenticate
+    c = Connection('localhost', server.sql.port)
+    c.connect()
+    print 'query', repr_dict(header), repr_dict(body)
+    header = msgpack.dumps(header)
+    body = msgpack.dumps(body)
+    query = msgpack.dumps(len(header) + len(body)) + header + body
+    # Send raw request using connectred socket
+    s = c._socket
+    try:
+        s.send(query)
+    except OSError as e:
+        print '   => ', 'Failed to send request'
+    c.close()
+    sql("ping")
+
+print """
+#  Test gh-206 "Segfault if sending IPROTO package without `KEY` field"
+"""
+
+print "IPROTO_SELECT"
+test({ IPROTO_CODE : REQUEST_TYPE_SELECT }, { IPROTO_SPACE_ID: 280 })
+print "\n"
+
+print "IPROTO_DELETE"
+test({ IPROTO_CODE : REQUEST_TYPE_DELETE }, { IPROTO_SPACE_ID: 280 })
+print "\n"
+
+print "IPROTO_UPDATE"
+test({ IPROTO_CODE : REQUEST_TYPE_UPDATE }, { IPROTO_SPACE_ID: 280 })
+test({ IPROTO_CODE : REQUEST_TYPE_UPDATE },
+     { IPROTO_SPACE_ID: 280, IPROTO_KEY: (1, )})
+print "\n"
+
+print "IPROTO_REPLACE"
+test({ IPROTO_CODE : REQUEST_TYPE_REPLACE }, { IPROTO_SPACE_ID: 280 })
+print "\n"
+
+print "IPROTO_CALL"
+test({ IPROTO_CODE : REQUEST_TYPE_CALL }, {})
+test({ IPROTO_CODE : REQUEST_TYPE_CALL }, { IPROTO_KEY: ('procname', )})
+print "\n"

test/box/misc.result

@@ -239,6 +239,7 @@ t;
   - 'box.error.ER_SPACE_EXISTS : 10'
   - 'box.error.ER_UNKNOWN_NODE : 62'
   - 'box.error.ER_MODIFY_INDEX : 14'
+  - 'box.error.ER_MISSING_REQUEST_FIELD : 69'
   - 'box.error.ER_SECONDARY : 7'
   - 'box.error.ER_NODE_ID_IS_RO : 66'
   - 'box.error.ER_INVALID_UUID : 64'

test/box/net.box.result

@@ -430,11 +430,11 @@ remote:close()
 ...
 remote:close()
 ---
-- error: '[string "-- box_net.lua (internal file)..."]:530: box.net.box: already closed'
+- error: '[string "-- box_net.lua (internal file)..."]:537: box.net.box: already closed'
 ...
 remote:ping()
 ---
-- error: '[string "-- box_net.lua (internal file)..."]:535: box.net.box: connection
+- error: '[string "-- box_net.lua (internal file)..."]:542: box.net.box: connection
     was closed'
 ...
 space:drop()