* [PATCH v2] socket: evaluate buffer size in recv / recvfrom
@ 2018-08-27 21:05 Alexander Turenko
2018-08-28 11:32 ` Vladimir Davydov
0 siblings, 1 reply; 3+ messages in thread
From: Alexander Turenko @ 2018-08-27 21:05 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: Alexander Turenko, tarantool-patches, Yaroslav Dynnikov
When size parameter is not passed to socket:recv() or socket:recvfrom()
it will call a) or b) on the socket to evaluate size of the buffer to
store the receiving datagram. Before this commit a datagram will be
truncated to 512 bytes in the case.
a) Linux: recv(fd, NULL, 0 , MSG_TRUNC | MSG_PEEK)
b) Mac OS: getsockopt(fd, SOL_SOCKET, SO_NREAD, &val, &len)
It is recommended to set 'size' parameter (size of the input buffer)
explicitly based on known message format and known network conditions
(say, set it less then MTU to prevent IP fragmentation, which can be
inefficient) or pass it from a configuration option of a library / an
application. The reason is that explicit buffer size provided allows to
avoid extra syscall to evaluate necessary buffer size.
When 'size' parameter is set explicitly for recv / recvfrom on a UDP
socket and the next datagram length is larger then the size, the
returned message will be truncated to the size provided and the rest of
the datagram will be discarded. Of course, the tail will not be
discarded in case of a TCP socket and will be available to read by the
next recv / recvfrom call.
Fixes #3619.
---
branch: Totktonada/gh-3619-socket-recvfrom-detect-a-cropped-packet
issue: https://github.com/tarantool/tarantool/issues/3619
travis-ci: https://travis-ci.org/tarantool/tarantool/builds/421272709
Changes since the previous version of the patch:
- discarded the patch to prevent UDP datagram truncation as Yaroslav
asks;
- moved itype saving into the socket constructor;
- removed is_udp and dgram_length from the public API (moved to
socket.internal reflecting the msgpackffi approach);
- adopted and added test cases for recv / recvfrom from the discarded
patch to hold the current behaviour (truncate with explicit size).
src/lua/socket.c | 3 +
src/lua/socket.lua | 68 +++-
test/app/socket.result | 672 ++++++++++++++++++++++++++++++++++++++-
test/app/socket.test.lua | 249 ++++++++++++++-
4 files changed, 980 insertions(+), 12 deletions(-)
diff --git a/src/lua/socket.c b/src/lua/socket.c
index 1458d3ba4..84711b441 100644
--- a/src/lua/socket.c
+++ b/src/lua/socket.c
@@ -268,6 +268,9 @@ static const struct lbox_sockopt_reg so_opts[] = {
#ifdef SO_PROTOCOL
{"SO_PROTOCOL", SO_PROTOCOL, 1, 0, },
#endif
+#ifdef SO_NREAD /* available on Darwin */
+ {"SO_NREAD", SO_NREAD, 1, 0, },
+#endif
{"SO_TYPE", SO_TYPE, 1, 0, },
diff --git a/src/lua/socket.lua b/src/lua/socket.lua
index 06306eae2..ddf7ebd53 100644
--- a/src/lua/socket.lua
+++ b/src/lua/socket.lua
@@ -86,8 +86,12 @@ local function check_socket(socket)
end
end
-local function make_socket(fd)
- local socket = { _gc_socket = ffi.new(gc_socket_t, { fd = fd }) }
+local function make_socket(fd, itype)
+ assert(itype ~= nil)
+ local socket = {
+ _gc_socket = ffi.new(gc_socket_t, { fd = fd }),
+ itype = itype,
+ }
return setmetatable(socket, socket_mt)
end
@@ -590,7 +594,7 @@ local function socket_accept(self)
self._errno = boxerrno()
return nil
end
- local client = make_socket(client_fd)
+ local client = make_socket(client_fd, self.itype)
if not client:nonblock(true) then
client:close()
return
@@ -770,6 +774,38 @@ local function socket_send(self, octets, flags)
return tonumber(res)
end
+local function socket_is_udp_internal(self)
+ assert(self.itype ~= nil)
+ return self.itype == internal.SO_TYPE['SOCK_DGRAM']
+end
+
+-- returns nil and sets EAGAIN when there are no datagrams in the receive queue
+-- as well as when zero-length datagram is the next one
+local function socket_dgram_length_internal(self)
+ local fd = check_socket(self)
+ if not socket_is_udp_internal(self) then
+ error("socket_dgram_length_internal() supports only a UDP socket")
+ end
+
+ local len
+ if jit.os == 'OSX' then
+ self._errno = nil
+ len = self:getsockopt('SOL_SOCKET', 'SO_NREAD')
+ else
+ local iflags = get_iflags(internal.SEND_FLAGS, {'MSG_TRUNC', 'MSG_PEEK'})
+ assert(iflags ~= nil)
+ self._errno = nil
+ len = tonumber(ffi.C.recv(fd, nil, 0, iflags))
+ end
+
+ if len == -1 or len == 0 then
+ self._errno = boxerrno()
+ return nil
+ end
+
+ return len
+end
+
local function socket_recv(self, size, flags)
local fd = check_socket(self)
local iflags = get_iflags(internal.SEND_FLAGS, flags)
@@ -778,7 +814,15 @@ local function socket_recv(self, size, flags)
return nil
end
- size = size or 512
+ if socket_is_udp_internal(self) then
+ size = size or socket_dgram_length_internal(self)
+ if size == nil then
+ return nil
+ end
+ else
+ size = size or 512
+ end
+
self._errno = nil
local buf = ffi.new("char[?]", size)
@@ -799,7 +843,15 @@ local function socket_recvfrom(self, size, flags)
return nil
end
- size = size or 512
+ if socket_is_udp_internal(self) then
+ size = size or socket_dgram_length_internal(self)
+ if size == nil then
+ return nil
+ end
+ else
+ size = size or 512
+ end
+
self._errno = nil
local res, from = internal.recvfrom(fd, size, iflags)
if res == nil then
@@ -860,7 +912,7 @@ local function socket_new(domain, stype, proto)
local fd = ffi.C.socket(idomain, itype, iproto)
if fd >= 0 then
- local socket = make_socket(fd)
+ local socket = make_socket(fd, itype)
if not socket:nonblock(true) then
socket:close()
else
@@ -1171,6 +1223,10 @@ socket_mt = {
name = socket_name;
peer = socket_peer;
fd = socket_fd;
+ internal = { -- for testing
+ is_udp = socket_is_udp_internal;
+ dgram_length = socket_dgram_length_internal;
+ };
};
__tostring = function(self)
local fd = check_socket(self)
diff --git a/test/app/socket.result b/test/app/socket.result
index b4d4a9a78..20ead8c6f 100644
--- a/test/app/socket.result
+++ b/test/app/socket.result
@@ -59,9 +59,11 @@ test_run:cmd("setopt delimiter ';'")
- true
...
for k in pairs(getmetatable(s).__index) do
- local r, msg = pcall(s[k])
- if not msg:match('Usage:') then
- error("Arguments is not checked for "..k)
+ if k ~= 'internal' then
+ local r, msg = pcall(s[k])
+ if not msg:match('Usage:') then
+ error("Arguments is not checked for "..k)
+ end
end
end;
---
@@ -1721,6 +1723,7 @@ test_run:cmd("setopt delimiter ''");
--------------------------------------------------------------------------------
-- Lua Socket Emulation
--------------------------------------------------------------------------------
+-- {{{
test_run:cmd("push filter 'fd=([0-9]+)' to 'fd=<FD>'")
---
- true
@@ -2230,6 +2233,669 @@ s:close()
---
- 1
...
+-- }}}
+-- s.internal.is_udp() {{{
+-- case: udp socket
+s = socket('AF_INET', 'SOCK_DGRAM', 'udp')
+---
+...
+s.internal.is_udp(s) -- expected true
+---
+- true
+...
+s:close()
+---
+- true
+...
+-- case: tcp socket
+s = socket('AF_INET', 'SOCK_STREAM', 0)
+---
+...
+s.internal.is_udp(s) -- expected false
+---
+- false
+...
+s:close()
+---
+- true
+...
+-- case: tcp socket from :accept()
+listening_socket = socket('AF_INET', 'SOCK_STREAM', 'tcp')
+---
+...
+listening_socket:setsockopt('SOL_SOCKET', 'SO_REUSEADDR', true)
+---
+- true
+...
+listening_socket:bind('127.0.0.1', 0)
+---
+- true
+...
+listening_socket:listen()
+---
+- true
+...
+listening_socket_port = listening_socket:name().port
+---
+...
+sending_socket = socket('AF_INET', 'SOCK_STREAM', 'tcp')
+---
+...
+sending_socket:sysconnect('127.0.0.1', listening_socket_port) or errno() == errno.EINPROGRESS
+---
+- true
+...
+receiving_socket = listening_socket:accept()
+---
+...
+receiving_socket.internal.is_udp(receiving_socket) -- expected false
+---
+- false
+...
+receiving_socket:close()
+---
+- true
+...
+sending_socket:close()
+---
+- true
+...
+listening_socket:close()
+---
+- true
+...
+-- }}}
+-- s.internal.dgram_length() {{{
+-- case: tcp socket is not supported
+s = socket('AF_INET', 'SOCK_STREAM', 0)
+---
+...
+s.internal.dgram_length(s) -- expected an error
+---
+- error: 'builtin/socket.lua: socket_dgram_length_internal() supports only a UDP
+ socket'
+...
+s:close()
+---
+- true
+...
+receiving_socket = socket('AF_INET', 'SOCK_DGRAM', 'udp')
+---
+...
+receiving_socket:bind('127.0.0.1', 0)
+---
+- true
+...
+receiving_socket_port = receiving_socket:name().port
+---
+...
+sending_socket = socket('AF_INET', 'SOCK_DGRAM', 'udp')
+---
+...
+-- case: no datagram
+received_len = receiving_socket.internal.dgram_length(receiving_socket)
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_len == nil -- expected true
+---
+- true
+...
+received_len
+---
+- null
+...
+e == errno.EAGAIN -- expected true
+---
+- true
+...
+message_len = 0
+---
+...
+message = ''
+---
+...
+-- case: zero-length datagram (the same as no datagram)
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+---
+- true
+...
+received_len = receiving_socket.internal.dgram_length(receiving_socket)
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_len == nil -- expected true
+---
+- true
+...
+received_len
+---
+- null
+...
+e == errno.EAGAIN -- expected true
+---
+- true
+...
+message_len = 1025
+---
+...
+message = string.rep('x', message_len)
+---
+...
+-- case: non-zero length datagram
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+---
+- 1025
+...
+received_len = receiving_socket.internal.dgram_length(receiving_socket)
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_len == message_len -- expected true
+---
+- true
+...
+received_len
+---
+- 1025
+...
+e == 0 -- expected true
+---
+- true
+...
+e
+---
+- 0
+...
+receiving_socket:close()
+---
+- true
+...
+sending_socket:close()
+---
+- true
+...
+-- }}}
+-- gh-3619: socket.recvfrom crops UDP packets {{{
+receiving_socket = socket('AF_INET', 'SOCK_DGRAM', 'udp')
+---
+...
+receiving_socket:bind('127.0.0.1', 0)
+---
+- true
+...
+receiving_socket_port = receiving_socket:name().port
+---
+...
+sending_socket = socket('AF_INET', 'SOCK_DGRAM', 'udp')
+---
+...
+-- case: recv, no datagram
+received_message = receiving_socket:recv()
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_message == nil -- expected true
+---
+- true
+...
+received_message
+---
+- null
+...
+e == errno.EAGAIN -- expected true
+---
+- true
+...
+-- case: recvfrom, no datagram
+received_message, from = receiving_socket:recvfrom()
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_message == nil -- expected true
+---
+- true
+...
+received_message
+---
+- null
+...
+from == nil -- expected true
+---
+- true
+...
+from
+---
+- null
+...
+e == errno.EAGAIN -- expected true
+---
+- true
+...
+message_len = 0
+---
+...
+message = ''
+---
+...
+-- case: recv, zero-length datagram (the same as no datagram)
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+---
+- true
+...
+received_message = receiving_socket:recv()
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_message == nil -- expected true
+---
+- true
+...
+received_message
+---
+- null
+...
+e == errno.EAGAIN -- expected true
+---
+- true
+...
+-- case: recvfrom, zero-length datagram (the same as no datagram)
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+---
+- true
+...
+received_message, from = receiving_socket:recvfrom()
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_message == nil -- expected true
+---
+- true
+...
+received_message
+---
+- null
+...
+from == nil -- expected true
+---
+- true
+...
+from
+---
+- null
+...
+e == errno.EAGAIN -- expected true
+---
+- true
+...
+-- case: recv, zero-length datagram (the same as no datagram), explicit size
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+---
+- true
+...
+received_message = receiving_socket:recv(512)
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_message == nil -- expected true
+---
+- true
+...
+received_message
+---
+- null
+...
+e == errno.EAGAIN -- expected true
+---
+- true
+...
+-- case: recvfrom, zero-length datagram (the same as no datagram), explicit size
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+---
+- true
+...
+received_message, from = receiving_socket:recvfrom(512)
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_message == nil -- expected true
+---
+- true
+...
+received_message
+---
+- null
+...
+from == nil -- expected true
+---
+- true
+...
+from
+---
+- null
+...
+e == errno.EAGAIN -- expected true
+---
+- true
+...
+message_len = 1025
+---
+...
+message = string.rep('x', message_len)
+---
+...
+-- case: recv, non-zero length datagram, the buffer size should be evaluated
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+---
+- 1025
+...
+received_message = receiving_socket:recv()
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_message == message -- expected true
+---
+- true
+...
+received_message:len()
+---
+- 1025
+...
+received_message
+---
+- xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
+...
+e == 0 -- expected true
+---
+- true
+...
+e
+---
+- 0
+...
+-- case: recvfrom, non-zero length datagram, the buffer size should be
+-- evaluated
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+---
+- 1025
+...
+received_message, from = receiving_socket:recvfrom()
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_message == message -- expected true
+---
+- true
+...
+received_message:len()
+---
+- 1025
+...
+received_message
+---
+- xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
+...
+from ~= nil -- expected true
+---
+- true
+...
+from.host == '127.0.0.1' -- expected true
+---
+- true
+...
+e == 0 -- expected true
+---
+- true
+...
+e
+---
+- 0
+...
+-- case: recv truncates a datagram larger then the buffer of an explicit size
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+---
+- 1025
+...
+received_message = receiving_socket:recv(512)
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_message == message:sub(1, 512) -- expected true
+---
+- true
+...
+received_message:len() == 512 -- expected true
+---
+- true
+...
+received_message
+---
+- xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
+...
+received_message:len()
+---
+- 512
+...
+e == 0 -- expected true
+---
+- true
+...
+e
+---
+- 0
+...
+-- we set the different message to ensure the tail of the previous datagram was
+-- discarded
+message_len = 1025
+---
+...
+message = string.rep('y', message_len)
+---
+...
+-- case: recvfrom truncates a datagram larger then the buffer of an explicit size
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+---
+- 1025
+...
+received_message, from = receiving_socket:recvfrom(512)
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_message == message:sub(1, 512) -- expected true
+---
+- true
+...
+received_message:len() == 512 -- expected true
+---
+- true
+...
+received_message
+---
+- yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy
+...
+received_message:len()
+---
+- 512
+...
+from ~= nil -- expected true
+---
+- true
+...
+from.host == '127.0.0.1' -- expected true
+---
+- true
+...
+e == 0 -- expected true
+---
+- true
+...
+e
+---
+- 0
+...
+receiving_socket:close()
+---
+- true
+...
+sending_socket:close()
+---
+- true
+...
+message_len = 513
+---
+...
+message = string.rep('x', message_len)
+---
+...
+listening_socket = socket('AF_INET', 'SOCK_STREAM', 'tcp')
+---
+...
+listening_socket:setsockopt('SOL_SOCKET', 'SO_REUSEADDR', true)
+---
+- true
+...
+listening_socket:bind('127.0.0.1', 0)
+---
+- true
+...
+listening_socket:listen()
+---
+- true
+...
+listening_socket_port = listening_socket:name().port
+---
+...
+sending_socket = socket('AF_INET', 'SOCK_STREAM', 'tcp')
+---
+...
+sending_socket:sysconnect('127.0.0.1', listening_socket_port) or errno() == errno.EINPROGRESS
+---
+- true
+...
+receiving_socket = listening_socket:accept()
+---
+...
+sending_socket:write(message)
+---
+- 513
+...
+-- case: recvfrom reads first 512 bytes from the message with tcp
+received_message, from = receiving_socket:recvfrom()
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_message == message:sub(1, 512) -- expected true
+---
+- true
+...
+received_message:len() == 512 -- expected true
+---
+- true
+...
+received_message
+---
+- xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
+...
+received_message:len()
+---
+- 512
+...
+from == nil -- expected true
+---
+- true
+...
+from
+---
+- null
+...
+e == 0 -- expected true
+---
+- true
+...
+e
+---
+- 0
+...
+-- case: recvfrom does not discard the message tail with tcp
+received_message, from = receiving_socket:recvfrom()
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_message == message:sub(513, 513) -- expected true
+---
+- true
+...
+received_message:len() == 1 -- expected true
+---
+- true
+...
+received_message
+---
+- x
+...
+received_message:len()
+---
+- 1
+...
+from == nil -- expected true
+---
+- true
+...
+from
+---
+- null
+...
+e == 0 -- expected true
+---
+- true
+...
+e
+---
+- 0
+...
+receiving_socket:close()
+---
+- true
+...
+sending_socket:close()
+---
+- true
+...
+listening_socket:close()
+---
+- true
+...
+-- }}}
test_run:cmd("clear filter")
---
- true
diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua
index f6539438b..cef71fc95 100644
--- a/test/app/socket.test.lua
+++ b/test/app/socket.test.lua
@@ -20,9 +20,11 @@ type(s)
-- Invalid arguments
test_run:cmd("setopt delimiter ';'")
for k in pairs(getmetatable(s).__index) do
- local r, msg = pcall(s[k])
- if not msg:match('Usage:') then
- error("Arguments is not checked for "..k)
+ if k ~= 'internal' then
+ local r, msg = pcall(s[k])
+ if not msg:match('Usage:') then
+ error("Arguments is not checked for "..k)
+ end
end
end;
s:close();
@@ -591,6 +593,7 @@ test_run:cmd("setopt delimiter ''");
--------------------------------------------------------------------------------
-- Lua Socket Emulation
--------------------------------------------------------------------------------
+-- {{{
test_run:cmd("push filter 'fd=([0-9]+)' to 'fd=<FD>'")
@@ -736,4 +739,244 @@ sc:connect(host, port)
sc:close()
s:close()
+-- }}}
+
+-- s.internal.is_udp() {{{
+
+-- case: udp socket
+s = socket('AF_INET', 'SOCK_DGRAM', 'udp')
+s.internal.is_udp(s) -- expected true
+s:close()
+
+-- case: tcp socket
+s = socket('AF_INET', 'SOCK_STREAM', 0)
+s.internal.is_udp(s) -- expected false
+s:close()
+
+-- case: tcp socket from :accept()
+listening_socket = socket('AF_INET', 'SOCK_STREAM', 'tcp')
+listening_socket:setsockopt('SOL_SOCKET', 'SO_REUSEADDR', true)
+listening_socket:bind('127.0.0.1', 0)
+listening_socket:listen()
+listening_socket_port = listening_socket:name().port
+sending_socket = socket('AF_INET', 'SOCK_STREAM', 'tcp')
+sending_socket:sysconnect('127.0.0.1', listening_socket_port) or errno() == errno.EINPROGRESS
+receiving_socket = listening_socket:accept()
+receiving_socket.internal.is_udp(receiving_socket) -- expected false
+receiving_socket:close()
+sending_socket:close()
+listening_socket:close()
+
+-- }}}
+
+-- s.internal.dgram_length() {{{
+
+-- case: tcp socket is not supported
+s = socket('AF_INET', 'SOCK_STREAM', 0)
+s.internal.dgram_length(s) -- expected an error
+s:close()
+
+receiving_socket = socket('AF_INET', 'SOCK_DGRAM', 'udp')
+receiving_socket:bind('127.0.0.1', 0)
+receiving_socket_port = receiving_socket:name().port
+sending_socket = socket('AF_INET', 'SOCK_DGRAM', 'udp')
+
+-- case: no datagram
+received_len = receiving_socket.internal.dgram_length(receiving_socket)
+e = receiving_socket:errno()
+received_len == nil -- expected true
+received_len
+e == errno.EAGAIN -- expected true
+
+message_len = 0
+message = ''
+
+-- case: zero-length datagram (the same as no datagram)
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+received_len = receiving_socket.internal.dgram_length(receiving_socket)
+e = receiving_socket:errno()
+received_len == nil -- expected true
+received_len
+e == errno.EAGAIN -- expected true
+
+message_len = 1025
+message = string.rep('x', message_len)
+
+-- case: non-zero length datagram
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+received_len = receiving_socket.internal.dgram_length(receiving_socket)
+e = receiving_socket:errno()
+received_len == message_len -- expected true
+received_len
+e == 0 -- expected true
+e
+
+receiving_socket:close()
+sending_socket:close()
+
+-- }}}
+
+-- gh-3619: socket.recvfrom crops UDP packets {{{
+
+receiving_socket = socket('AF_INET', 'SOCK_DGRAM', 'udp')
+receiving_socket:bind('127.0.0.1', 0)
+receiving_socket_port = receiving_socket:name().port
+sending_socket = socket('AF_INET', 'SOCK_DGRAM', 'udp')
+
+-- case: recv, no datagram
+received_message = receiving_socket:recv()
+e = receiving_socket:errno()
+received_message == nil -- expected true
+received_message
+e == errno.EAGAIN -- expected true
+
+-- case: recvfrom, no datagram
+received_message, from = receiving_socket:recvfrom()
+e = receiving_socket:errno()
+received_message == nil -- expected true
+received_message
+from == nil -- expected true
+from
+e == errno.EAGAIN -- expected true
+
+message_len = 0
+message = ''
+
+-- case: recv, zero-length datagram (the same as no datagram)
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+received_message = receiving_socket:recv()
+e = receiving_socket:errno()
+received_message == nil -- expected true
+received_message
+e == errno.EAGAIN -- expected true
+
+-- case: recvfrom, zero-length datagram (the same as no datagram)
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+received_message, from = receiving_socket:recvfrom()
+e = receiving_socket:errno()
+received_message == nil -- expected true
+received_message
+from == nil -- expected true
+from
+e == errno.EAGAIN -- expected true
+
+-- case: recv, zero-length datagram (the same as no datagram), explicit size
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+received_message = receiving_socket:recv(512)
+e = receiving_socket:errno()
+received_message == nil -- expected true
+received_message
+e == errno.EAGAIN -- expected true
+
+-- case: recvfrom, zero-length datagram (the same as no datagram), explicit size
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+received_message, from = receiving_socket:recvfrom(512)
+e = receiving_socket:errno()
+received_message == nil -- expected true
+received_message
+from == nil -- expected true
+from
+e == errno.EAGAIN -- expected true
+
+message_len = 1025
+message = string.rep('x', message_len)
+
+-- case: recv, non-zero length datagram, the buffer size should be evaluated
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+received_message = receiving_socket:recv()
+e = receiving_socket:errno()
+received_message == message -- expected true
+received_message:len()
+received_message
+e == 0 -- expected true
+e
+
+-- case: recvfrom, non-zero length datagram, the buffer size should be
+-- evaluated
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+received_message, from = receiving_socket:recvfrom()
+e = receiving_socket:errno()
+received_message == message -- expected true
+received_message:len()
+received_message
+from ~= nil -- expected true
+from.host == '127.0.0.1' -- expected true
+e == 0 -- expected true
+e
+
+-- case: recv truncates a datagram larger then the buffer of an explicit size
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+received_message = receiving_socket:recv(512)
+e = receiving_socket:errno()
+received_message == message:sub(1, 512) -- expected true
+received_message:len() == 512 -- expected true
+received_message
+received_message:len()
+e == 0 -- expected true
+e
+
+-- we set the different message to ensure the tail of the previous datagram was
+-- discarded
+message_len = 1025
+message = string.rep('y', message_len)
+
+-- case: recvfrom truncates a datagram larger then the buffer of an explicit size
+sending_socket:sendto('127.0.0.1', receiving_socket_port, message)
+received_message, from = receiving_socket:recvfrom(512)
+e = receiving_socket:errno()
+received_message == message:sub(1, 512) -- expected true
+received_message:len() == 512 -- expected true
+received_message
+received_message:len()
+from ~= nil -- expected true
+from.host == '127.0.0.1' -- expected true
+e == 0 -- expected true
+e
+
+receiving_socket:close()
+sending_socket:close()
+
+message_len = 513
+message = string.rep('x', message_len)
+
+listening_socket = socket('AF_INET', 'SOCK_STREAM', 'tcp')
+listening_socket:setsockopt('SOL_SOCKET', 'SO_REUSEADDR', true)
+listening_socket:bind('127.0.0.1', 0)
+listening_socket:listen()
+listening_socket_port = listening_socket:name().port
+sending_socket = socket('AF_INET', 'SOCK_STREAM', 'tcp')
+sending_socket:sysconnect('127.0.0.1', listening_socket_port) or errno() == errno.EINPROGRESS
+receiving_socket = listening_socket:accept()
+sending_socket:write(message)
+
+-- case: recvfrom reads first 512 bytes from the message with tcp
+received_message, from = receiving_socket:recvfrom()
+e = receiving_socket:errno()
+received_message == message:sub(1, 512) -- expected true
+received_message:len() == 512 -- expected true
+received_message
+received_message:len()
+from == nil -- expected true
+from
+e == 0 -- expected true
+e
+
+-- case: recvfrom does not discard the message tail with tcp
+received_message, from = receiving_socket:recvfrom()
+e = receiving_socket:errno()
+received_message == message:sub(513, 513) -- expected true
+received_message:len() == 1 -- expected true
+received_message
+received_message:len()
+from == nil -- expected true
+from
+e == 0 -- expected true
+e
+
+receiving_socket:close()
+sending_socket:close()
+listening_socket:close()
+
+-- }}}
+
test_run:cmd("clear filter")
--
2.17.1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] socket: evaluate buffer size in recv / recvfrom
2018-08-27 21:05 [PATCH v2] socket: evaluate buffer size in recv / recvfrom Alexander Turenko
@ 2018-08-28 11:32 ` Vladimir Davydov
2018-08-28 15:01 ` Alexander Turenko
0 siblings, 1 reply; 3+ messages in thread
From: Vladimir Davydov @ 2018-08-28 11:32 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches, Yaroslav Dynnikov
On Tue, Aug 28, 2018 at 12:05:10AM +0300, Alexander Turenko wrote:
> When size parameter is not passed to socket:recv() or socket:recvfrom()
> it will call a) or b) on the socket to evaluate size of the buffer to
> store the receiving datagram. Before this commit a datagram will be
> truncated to 512 bytes in the case.
>
> a) Linux: recv(fd, NULL, 0 , MSG_TRUNC | MSG_PEEK)
> b) Mac OS: getsockopt(fd, SOL_SOCKET, SO_NREAD, &val, &len)
>
> It is recommended to set 'size' parameter (size of the input buffer)
> explicitly based on known message format and known network conditions
> (say, set it less then MTU to prevent IP fragmentation, which can be
> inefficient) or pass it from a configuration option of a library / an
> application. The reason is that explicit buffer size provided allows to
> avoid extra syscall to evaluate necessary buffer size.
>
> When 'size' parameter is set explicitly for recv / recvfrom on a UDP
> socket and the next datagram length is larger then the size, the
> returned message will be truncated to the size provided and the rest of
> the datagram will be discarded. Of course, the tail will not be
> discarded in case of a TCP socket and will be available to read by the
> next recv / recvfrom call.
>
> Fixes #3619.
> ---
>
> branch: Totktonada/gh-3619-socket-recvfrom-detect-a-cropped-packet
Please put a hyperlink to the branch, not just its name.
Also, prefixes "branch:", "issue:", "travis-ci" aren't necessary as it
is apparent which is which.
> issue: https://github.com/tarantool/tarantool/issues/3619
> travis-ci: https://travis-ci.org/tarantool/tarantool/builds/421272709
>
> Changes since the previous version of the patch:
>
> - discarded the patch to prevent UDP datagram truncation as Yaroslav
> asks;
> - moved itype saving into the socket constructor;
> - removed is_udp and dgram_length from the public API (moved to
> socket.internal reflecting the msgpackffi approach);
> - adopted and added test cases for recv / recvfrom from the discarded
> patch to hold the current behaviour (truncate with explicit size).
>
> src/lua/socket.c | 3 +
> src/lua/socket.lua | 68 +++-
> test/app/socket.result | 672 ++++++++++++++++++++++++++++++++++++++-
> test/app/socket.test.lua | 249 ++++++++++++++-
> 4 files changed, 980 insertions(+), 12 deletions(-)
>
> diff --git a/src/lua/socket.c b/src/lua/socket.c
> index 1458d3ba4..84711b441 100644
> --- a/src/lua/socket.c
> +++ b/src/lua/socket.c
> @@ -268,6 +268,9 @@ static const struct lbox_sockopt_reg so_opts[] = {
> #ifdef SO_PROTOCOL
> {"SO_PROTOCOL", SO_PROTOCOL, 1, 0, },
> #endif
> +#ifdef SO_NREAD /* available on Darwin */
> + {"SO_NREAD", SO_NREAD, 1, 0, },
> +#endif
>
> {"SO_TYPE", SO_TYPE, 1, 0, },
>
> diff --git a/src/lua/socket.lua b/src/lua/socket.lua
> index 06306eae2..ddf7ebd53 100644
> --- a/src/lua/socket.lua
> +++ b/src/lua/socket.lua
> @@ -86,8 +86,12 @@ local function check_socket(socket)
> end
> end
>
> -local function make_socket(fd)
> - local socket = { _gc_socket = ffi.new(gc_socket_t, { fd = fd }) }
> +local function make_socket(fd, itype)
> + assert(itype ~= nil)
> + local socket = {
> + _gc_socket = ffi.new(gc_socket_t, { fd = fd }),
> + itype = itype,
> + }
> return setmetatable(socket, socket_mt)
> end
>
> @@ -590,7 +594,7 @@ local function socket_accept(self)
> self._errno = boxerrno()
> return nil
> end
> - local client = make_socket(client_fd)
> + local client = make_socket(client_fd, self.itype)
> if not client:nonblock(true) then
> client:close()
> return
> @@ -770,6 +774,38 @@ local function socket_send(self, octets, flags)
> return tonumber(res)
> end
>
> +local function socket_is_udp_internal(self)
> + assert(self.itype ~= nil)
> + return self.itype == internal.SO_TYPE['SOCK_DGRAM']
Nit: get_ivalue?
Also, I'd inline this function as it's a one-liner, and remove the
assertion as itype is going to be set anyway.
> +end
> +
> +-- returns nil and sets EAGAIN when there are no datagrams in the receive queue
> +-- as well as when zero-length datagram is the next one
> +local function socket_dgram_length_internal(self)
I don't like the function name. Let's call it simply get_dgram_len?
> + local fd = check_socket(self)
> + if not socket_is_udp_internal(self) then
> + error("socket_dgram_length_internal() supports only a UDP socket")
> + end
I'd remove these checks as this function is only called by recv() and
recvfrom(), where these checks have already been carried out.
> +
> + local len
> + if jit.os == 'OSX' then
> + self._errno = nil
> + len = self:getsockopt('SOL_SOCKET', 'SO_NREAD')
> + else
> + local iflags = get_iflags(internal.SEND_FLAGS, {'MSG_TRUNC', 'MSG_PEEK'})
> + assert(iflags ~= nil)
> + self._errno = nil
Nit: 'self._errno = nil' can be moved before 'if'.
> + len = tonumber(ffi.C.recv(fd, nil, 0, iflags))
Nit: tonumber is not necessary here.
> + end
> +
> + if len == -1 or len == 0 then
AFAIU errno isn't set unless the return value is -1. Please check and
remove 'len == 0' from the expression if so.
> + self._errno = boxerrno()
> + return nil
> + end
> +
> + return len
> +end
> +
> local function socket_recv(self, size, flags)
> local fd = check_socket(self)
> local iflags = get_iflags(internal.SEND_FLAGS, flags)
> @@ -778,7 +814,15 @@ local function socket_recv(self, size, flags)
> return nil
> end
>
> - size = size or 512
> + if socket_is_udp_internal(self) then
> + size = size or socket_dgram_length_internal(self)
> + if size == nil then
> + return nil
> + end
> + else
> + size = size or 512
> + end
> +
> self._errno = nil
> local buf = ffi.new("char[?]", size)
>
> @@ -799,7 +843,15 @@ local function socket_recvfrom(self, size, flags)
> return nil
> end
>
> - size = size or 512
> + if socket_is_udp_internal(self) then
> + size = size or socket_dgram_length_internal(self)
> + if size == nil then
> + return nil
> + end
> + else
> + size = size or 512
> + end
> +
Don't really like the code duplication. What about factoring it out in a
helper function? Then you wouldn't need get_dgram_len() at all as you
could inline it in there. I'm not sure how to call such a function. What
about get_recv_size(size)?
> self._errno = nil
> local res, from = internal.recvfrom(fd, size, iflags)
> if res == nil then
> @@ -860,7 +912,7 @@ local function socket_new(domain, stype, proto)
>
> local fd = ffi.C.socket(idomain, itype, iproto)
> if fd >= 0 then
> - local socket = make_socket(fd)
> + local socket = make_socket(fd, itype)
> if not socket:nonblock(true) then
> socket:close()
> else
> @@ -1171,6 +1223,10 @@ socket_mt = {
> name = socket_name;
> peer = socket_peer;
> fd = socket_fd;
> + internal = { -- for testing
> + is_udp = socket_is_udp_internal;
> + dgram_length = socket_dgram_length_internal;
> + };
I don't think it's worth exporting these functions, even for testing,
because you cover them for both stream and dgram sockets anyway. I'd
remove them and corresponding tests.
> };
> __tostring = function(self)
> local fd = check_socket(self)
> diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua
> index f6539438b..cef71fc95 100644
> --- a/test/app/socket.test.lua
> +++ b/test/app/socket.test.lua
> @@ -591,6 +593,7 @@ test_run:cmd("setopt delimiter ''");
> --------------------------------------------------------------------------------
> -- Lua Socket Emulation
> --------------------------------------------------------------------------------
> +-- {{{
Nit: we never use {{{ }}} for separating test groups in tests so this
looks inconsistent.
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] socket: evaluate buffer size in recv / recvfrom
2018-08-28 11:32 ` Vladimir Davydov
@ 2018-08-28 15:01 ` Alexander Turenko
0 siblings, 0 replies; 3+ messages in thread
From: Alexander Turenko @ 2018-08-28 15:01 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches, Yaroslav Dynnikov
I'll send the patch v3 soon. Answered to the comments below.
I have tested the patch with zero length datagrams using patched sendto
method:
diff --git a/src/lua/socket.lua b/src/lua/socket.lua
index 5d98fd2b2..d3ce6c3f5 100644
--- a/src/lua/socket.lua
+++ b/src/lua/socket.lua
@@ -862,7 +862,7 @@ local function socket_sendto(self, host, port, octets, flags)
end
self._errno = nil
- if octets == nil or octets == '' then
+ if octets == nil then -- or octets == '' then
return true
end
On the following test cases:
diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua
index 840f042e4..af8495aa2 100644
--- a/test/app/socket.test.lua
+++ b/test/app/socket.test.lua
@@ -761,6 +761,24 @@ from == nil -- expected true
from
e == errno.EAGAIN -- expected true
+-- case: recv, zero datagram
+sending_socket:sendto('127.0.0.1', receiving_socket_port, '')
+received_message = receiving_socket:recv()
+e = receiving_socket:errno()
+received_message == '' -- expected true
+received_message
+e == 0 -- expected true
+
+-- case: recvfrom, zero datagram
+sending_socket:sendto('127.0.0.1', receiving_socket_port, '')
+received_message, from = receiving_socket:recvfrom()
+e = receiving_socket:errno()
+received_message == '' -- expected true
+received_message
+from ~= nil -- expected true
+from.host == '127.0.0.1' -- expected true
+e == 0 -- expected true
+
-- case: recv, no datagram, explicit size
received_message = receiving_socket:recv(512)
e = receiving_socket:errno()
@@ -777,6 +795,24 @@ from == nil -- expected true
from
e == errno.EAGAIN -- expected true
+-- case: recv, zero datagram, explicit size
+sending_socket:sendto('127.0.0.1', receiving_socket_port, '')
+received_message = receiving_socket:recv(512)
+e = receiving_socket:errno()
+received_message == '' -- expected true
+received_message
+e == 0 -- expected true
+
+-- case: recvfrom, zero datagram, explicit size
+sending_socket:sendto('127.0.0.1', receiving_socket_port, '')
+received_message, from = receiving_socket:recvfrom(512)
+e = receiving_socket:errno()
+received_message == '' -- expected true
+received_message
+from ~= nil -- expected true
+from.host == '127.0.0.1' -- expected true
+e == 0 -- expected true
+
message_len = 1025
message = string.rep('x', message_len)
But I'll not include this in the patch, because it necessary to copy
patched sendto function into the test to allow send zero-length
datagrams (or change the original sendto with possible compatibility
break); Vladimir seems to be against it.
WBR, Alexander Turenko.
On Tue, Aug 28, 2018 at 02:32:14PM +0300, Vladimir Davydov wrote:
> On Tue, Aug 28, 2018 at 12:05:10AM +0300, Alexander Turenko wrote:
> > When size parameter is not passed to socket:recv() or socket:recvfrom()
> > it will call a) or b) on the socket to evaluate size of the buffer to
> > store the receiving datagram. Before this commit a datagram will be
> > truncated to 512 bytes in the case.
> >
> > a) Linux: recv(fd, NULL, 0 , MSG_TRUNC | MSG_PEEK)
> > b) Mac OS: getsockopt(fd, SOL_SOCKET, SO_NREAD, &val, &len)
> >
> > It is recommended to set 'size' parameter (size of the input buffer)
> > explicitly based on known message format and known network conditions
> > (say, set it less then MTU to prevent IP fragmentation, which can be
> > inefficient) or pass it from a configuration option of a library / an
> > application. The reason is that explicit buffer size provided allows to
> > avoid extra syscall to evaluate necessary buffer size.
> >
> > When 'size' parameter is set explicitly for recv / recvfrom on a UDP
> > socket and the next datagram length is larger then the size, the
> > returned message will be truncated to the size provided and the rest of
> > the datagram will be discarded. Of course, the tail will not be
> > discarded in case of a TCP socket and will be available to read by the
> > next recv / recvfrom call.
> >
> > Fixes #3619.
> > ---
> >
> > branch: Totktonada/gh-3619-socket-recvfrom-detect-a-cropped-packet
>
> Please put a hyperlink to the branch, not just its name.
>
> Also, prefixes "branch:", "issue:", "travis-ci" aren't necessary as it
> is apparent which is which.
>
Ok.
> > issue: https://github.com/tarantool/tarantool/issues/3619
> > travis-ci: https://travis-ci.org/tarantool/tarantool/builds/421272709
> >
> > Changes since the previous version of the patch:
> >
> > - discarded the patch to prevent UDP datagram truncation as Yaroslav
> > asks;
> > - moved itype saving into the socket constructor;
> > - removed is_udp and dgram_length from the public API (moved to
> > socket.internal reflecting the msgpackffi approach);
> > - adopted and added test cases for recv / recvfrom from the discarded
> > patch to hold the current behaviour (truncate with explicit size).
> >
> > src/lua/socket.c | 3 +
> > src/lua/socket.lua | 68 +++-
> > test/app/socket.result | 672 ++++++++++++++++++++++++++++++++++++++-
> > test/app/socket.test.lua | 249 ++++++++++++++-
> > 4 files changed, 980 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/lua/socket.c b/src/lua/socket.c
> > index 1458d3ba4..84711b441 100644
> > --- a/src/lua/socket.c
> > +++ b/src/lua/socket.c
> > @@ -268,6 +268,9 @@ static const struct lbox_sockopt_reg so_opts[] = {
> > #ifdef SO_PROTOCOL
> > {"SO_PROTOCOL", SO_PROTOCOL, 1, 0, },
> > #endif
> > +#ifdef SO_NREAD /* available on Darwin */
> > + {"SO_NREAD", SO_NREAD, 1, 0, },
> > +#endif
> >
> > {"SO_TYPE", SO_TYPE, 1, 0, },
> >
> > diff --git a/src/lua/socket.lua b/src/lua/socket.lua
> > index 06306eae2..ddf7ebd53 100644
> > --- a/src/lua/socket.lua
> > +++ b/src/lua/socket.lua
> > @@ -86,8 +86,12 @@ local function check_socket(socket)
> > end
> > end
> >
> > -local function make_socket(fd)
> > - local socket = { _gc_socket = ffi.new(gc_socket_t, { fd = fd }) }
> > +local function make_socket(fd, itype)
> > + assert(itype ~= nil)
> > + local socket = {
> > + _gc_socket = ffi.new(gc_socket_t, { fd = fd }),
> > + itype = itype,
> > + }
> > return setmetatable(socket, socket_mt)
> > end
> >
> > @@ -590,7 +594,7 @@ local function socket_accept(self)
> > self._errno = boxerrno()
> > return nil
> > end
> > - local client = make_socket(client_fd)
> > + local client = make_socket(client_fd, self.itype)
> > if not client:nonblock(true) then
> > client:close()
> > return
> > @@ -770,6 +774,38 @@ local function socket_send(self, octets, flags)
> > return tonumber(res)
> > end
> >
> > +local function socket_is_udp_internal(self)
> > + assert(self.itype ~= nil)
> > + return self.itype == internal.SO_TYPE['SOCK_DGRAM']
>
> Nit: get_ivalue?
>
Ok.
> Also, I'd inline this function as it's a one-liner, and remove the
> assertion as itype is going to be set anyway.
>
> > +end
> > +
> > +-- returns nil and sets EAGAIN when there are no datagrams in the receive queue
> > +-- as well as when zero-length datagram is the next one
> > +local function socket_dgram_length_internal(self)
>
> I don't like the function name. Let's call it simply get_dgram_len?
>
Ok. Inlined into get_recv_size.
> > + local fd = check_socket(self)
> > + if not socket_is_udp_internal(self) then
> > + error("socket_dgram_length_internal() supports only a UDP socket")
> > + end
>
> I'd remove these checks as this function is only called by recv() and
> recvfrom(), where these checks have already been carried out.
>
Ok.
> > +
> > + local len
> > + if jit.os == 'OSX' then
> > + self._errno = nil
> > + len = self:getsockopt('SOL_SOCKET', 'SO_NREAD')
> > + else
> > + local iflags = get_iflags(internal.SEND_FLAGS, {'MSG_TRUNC', 'MSG_PEEK'})
> > + assert(iflags ~= nil)
> > + self._errno = nil
>
> Nit: 'self._errno = nil' can be moved before 'if'.
>
Ok.
> > + len = tonumber(ffi.C.recv(fd, nil, 0, iflags))
>
It is needed, because lua_tointeger in ffi.C.recvfrom() cannot handle
cdata<int64_t> correctly. It is possible to add a function like
luaL_convertint64 from src/lua/utils.c, but w/o converting a string to
number. I think it is overkill, especially, because tonumber() is
already used here and there this way in socket.lua.
> Nit: tonumber is not necessary here.
>
> > + end
> > +
> > + if len == -1 or len == 0 then
>
> AFAIU errno isn't set unless the return value is -1. Please check and
> remove 'len == 0' from the expression if so.
>
You are right. Removed.
> > + self._errno = boxerrno()
> > + return nil
> > + end
> > +
> > + return len
> > +end
> > +
> > local function socket_recv(self, size, flags)
> > local fd = check_socket(self)
> > local iflags = get_iflags(internal.SEND_FLAGS, flags)
> > @@ -778,7 +814,15 @@ local function socket_recv(self, size, flags)
> > return nil
> > end
> >
> > - size = size or 512
> > + if socket_is_udp_internal(self) then
> > + size = size or socket_dgram_length_internal(self)
> > + if size == nil then
> > + return nil
> > + end
> > + else
> > + size = size or 512
> > + end
> > +
> > self._errno = nil
> > local buf = ffi.new("char[?]", size)
> >
> > @@ -799,7 +843,15 @@ local function socket_recvfrom(self, size, flags)
> > return nil
> > end
> >
> > - size = size or 512
> > + if socket_is_udp_internal(self) then
> > + size = size or socket_dgram_length_internal(self)
> > + if size == nil then
> > + return nil
> > + end
> > + else
> > + size = size or 512
> > + end
> > +
>
> Don't really like the code duplication. What about factoring it out in a
> helper function? Then you wouldn't need get_dgram_len() at all as you
> could inline it in there. I'm not sure how to call such a function. What
> about get_recv_size(size)?
>
Ok.
> > self._errno = nil
> > local res, from = internal.recvfrom(fd, size, iflags)
> > if res == nil then
> > @@ -860,7 +912,7 @@ local function socket_new(domain, stype, proto)
> >
> > local fd = ffi.C.socket(idomain, itype, iproto)
> > if fd >= 0 then
> > - local socket = make_socket(fd)
> > + local socket = make_socket(fd, itype)
> > if not socket:nonblock(true) then
> > socket:close()
> > else
> > @@ -1171,6 +1223,10 @@ socket_mt = {
> > name = socket_name;
> > peer = socket_peer;
> > fd = socket_fd;
> > + internal = { -- for testing
> > + is_udp = socket_is_udp_internal;
> > + dgram_length = socket_dgram_length_internal;
> > + };
>
> I don't think it's worth exporting these functions, even for testing,
> because you cover them for both stream and dgram sockets anyway. I'd
> remove them and corresponding tests.
>
Ok.
> > };
> > __tostring = function(self)
> > local fd = check_socket(self)
>
> > diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua
> > index f6539438b..cef71fc95 100644
> > --- a/test/app/socket.test.lua
> > +++ b/test/app/socket.test.lua
> > @@ -591,6 +593,7 @@ test_run:cmd("setopt delimiter ''");
> > --------------------------------------------------------------------------------
> > -- Lua Socket Emulation
> > --------------------------------------------------------------------------------
> > +-- {{{
>
> Nit: we never use {{{ }}} for separating test groups in tests so this
> looks inconsistent.
Ok.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-08-28 15:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-27 21:05 [PATCH v2] socket: evaluate buffer size in recv / recvfrom Alexander Turenko
2018-08-28 11:32 ` Vladimir Davydov
2018-08-28 15:01 ` Alexander Turenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox