[PATCH v3] socket: evaluate buffer size in recv / recvfrom

[PATCH v3] socket: evaluate buffer size in recv / recvfrom

[Alexander Turenko]

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

https://github.com/tarantool/tarantool/tree/Totktonada/gh-3619-socket-recvfrom-detect-a-cropped-packet
https://github.com/tarantool/tarantool/issues/3619

Changes from v2:

- code style fixes according to Vladimir's review;
- fixed zero-length datagrams receiving;
- removed zero-length datagram tests (were incorrect, see the previous
  email in the thread).

 src/lua/socket.c         |   3 +
 src/lua/socket.lua       |  62 +++++-
 test/app/socket.result   | 407 +++++++++++++++++++++++++++++++++++++++
 test/app/socket.test.lua | 140 ++++++++++++++
 4 files changed, 606 insertions(+), 6 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..04e99a668 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,44 @@ local function socket_send(self, octets, flags)
     return tonumber(res)
 end
 
+-- Returns nil and sets EAGAIN when tries to determine the next datagram length
+-- and there are no datagrams in the receive queue.
+local function get_recv_size(self, size)
+    local fd = check_socket(self)
+
+    if size ~= nil then
+        return size
+    end
+
+    if self.itype ~= get_ivalue(internal.SO_TYPE, 'SOCK_DGRAM') then
+        return 512
+    end
+
+    -- Determine the next datagram length.
+    self._errno = nil
+    if jit.os == 'OSX' then
+        size = self:getsockopt('SOL_SOCKET', 'SO_NREAD')
+        -- recv() with zero length buffer is always successful on Mac OS (at
+        -- least for valid UDP sockets). The assignment of size to 1 below
+        -- allows to handle 'no datagram' situation consistently: returning nil
+        -- and setting EAGAIN.
+        if size == 0 then
+            size = 1
+        end
+    else
+        local iflags = get_iflags(internal.SEND_FLAGS, {'MSG_TRUNC', 'MSG_PEEK'})
+        assert(iflags ~= nil)
+        size = tonumber(ffi.C.recv(fd, nil, 0, iflags))
+    end
+
+    if size == -1 then
+        self._errno = boxerrno()
+        return nil
+    end
+
+    return size
+end
+
 local function socket_recv(self, size, flags)
     local fd = check_socket(self)
     local iflags = get_iflags(internal.SEND_FLAGS, flags)
@@ -778,7 +820,11 @@ local function socket_recv(self, size, flags)
         return nil
     end
 
-    size = size or 512
+    size = get_recv_size(self, size)
+    if size == nil then
+        return nil
+    end
+
     self._errno = nil
     local buf = ffi.new("char[?]", size)
 
@@ -799,7 +845,11 @@ local function socket_recvfrom(self, size, flags)
         return nil
     end
 
-    size = size or 512
+    size = get_recv_size(self, size)
+    if size == nil then
+        return nil
+    end
+
     self._errno = nil
     local res, from = internal.recvfrom(fd, size, iflags)
     if res == nil then
@@ -860,7 +910,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
diff --git a/test/app/socket.result b/test/app/socket.result
index b4d4a9a78..f2c73b305 100644
--- a/test/app/socket.result
+++ b/test/app/socket.result
@@ -2230,6 +2230,413 @@ s:close()
 ---
 - 1
 ...
+--------------------------------------------------------------------------------
+-- 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
+...
+-- case: recv, no datagram, explicit size
+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, no datagram, explicit size
+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..840f042e4 100644
--- a/test/app/socket.test.lua
+++ b/test/app/socket.test.lua
@@ -736,4 +736,144 @@ sc:connect(host, port)
 sc:close()
 s: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
+
+-- case: recv, no datagram, explicit size
+received_message = receiving_socket:recv(512)
+e = receiving_socket:errno()
+received_message == nil -- expected true
+received_message
+e == errno.EAGAIN -- expected true
+
+-- case: recvfrom, no datagram, explicit size
+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

Re: [PATCH v3] socket: evaluate buffer size in recv / recvfrom

[Vladimir Davydov]

On Tue, Aug 28, 2018 at 09:21:45PM +0300, Alexander Turenko wrote:
> diff --git a/src/lua/socket.lua b/src/lua/socket.lua
> index 06306eae2..04e99a668 100644
> --- a/src/lua/socket.lua
> +++ b/src/lua/socket.lua
> @@ -770,6 +774,44 @@ local function socket_send(self, octets, flags)
>      return tonumber(res)
>  end
>  
> +-- Returns nil and sets EAGAIN when tries to determine the next datagram length
> +-- and there are no datagrams in the receive queue.

Nit: please limit the width of comments to 60-70 symbols - easier to
read them that way.

> +local function get_recv_size(self, size)
> +    local fd = check_socket(self)

Nit: you only need fd in the !OSX branch below so I'd move this there.
No point in calling check_socket when size is set, right?

> +
> +    if size ~= nil then
> +        return size
> +    end
> +
> +    if self.itype ~= get_ivalue(internal.SO_TYPE, 'SOCK_DGRAM') then
> +        return 512
> +    end
> +
> +    -- Determine the next datagram length.
> +    self._errno = nil
> +    if jit.os == 'OSX' then
> +        size = self:getsockopt('SOL_SOCKET', 'SO_NREAD')
> +        -- recv() with zero length buffer is always successful on Mac OS (at
> +        -- least for valid UDP sockets). The assignment of size to 1 below
> +        -- allows to handle 'no datagram' situation consistently: returning nil
> +        -- and setting EAGAIN.
> +        if size == 0 then
> +            size = 1
> +        end

I'm afraid there's an inherent race condition here. What if a new
datagram of non-zero length arrives after you check the size here,
but before you call recv?

> +    else
> +        local iflags = get_iflags(internal.SEND_FLAGS, {'MSG_TRUNC', 'MSG_PEEK'})
> +        assert(iflags ~= nil)
> +        size = tonumber(ffi.C.recv(fd, nil, 0, iflags))
> +    end
> +
> +    if size == -1 then
> +        self._errno = boxerrno()
> +        return nil
> +    end
> +
> +    return size
> +end
> +
>  local function socket_recv(self, size, flags)
>      local fd = check_socket(self)
>      local iflags = get_iflags(internal.SEND_FLAGS, flags)
> @@ -778,7 +820,11 @@ local function socket_recv(self, size, flags)
>          return nil
>      end
>  
> -    size = size or 512
> +    size = get_recv_size(self, size)
> +    if size == nil then
> +        return nil
> +    end
> +
>      self._errno = nil
>      local buf = ffi.new("char[?]", size)
>  

Re: [tarantool-patches] Re: [PATCH v3] socket: evaluate buffer size in recv / recvfrom

[Alexander Turenko]

On Wed, Aug 29, 2018 at 01:16:51PM +0300, Vladimir Davydov wrote:
> On Tue, Aug 28, 2018 at 09:21:45PM +0300, Alexander Turenko wrote:
> > diff --git a/src/lua/socket.lua b/src/lua/socket.lua
> > index 06306eae2..04e99a668 100644
> > --- a/src/lua/socket.lua
> > +++ b/src/lua/socket.lua
> > @@ -770,6 +774,44 @@ local function socket_send(self, octets, flags)
> >      return tonumber(res)
> >  end
> >  
> > +-- Returns nil and sets EAGAIN when tries to determine the next datagram length
> > +-- and there are no datagrams in the receive queue.
> 
> Nit: please limit the width of comments to 60-70 symbols - easier to
> read them that way.
> 

Ok.

> > +local function get_recv_size(self, size)
> > +    local fd = check_socket(self)
> 
> Nit: you only need fd in the !OSX branch below so I'd move this there.
> No point in calling check_socket when size is set, right?
> 

Ok. In the patch v4 I use it in OSX branch too, but I sink it down the
first two ifs.

> > +
> > +    if size ~= nil then
> > +        return size
> > +    end
> > +
> > +    if self.itype ~= get_ivalue(internal.SO_TYPE, 'SOCK_DGRAM') then
> > +        return 512
> > +    end
> > +
> > +    -- Determine the next datagram length.
> > +    self._errno = nil
> > +    if jit.os == 'OSX' then
> > +        size = self:getsockopt('SOL_SOCKET', 'SO_NREAD')
> > +        -- recv() with zero length buffer is always successful on Mac OS (at
> > +        -- least for valid UDP sockets). The assignment of size to 1 below
> > +        -- allows to handle 'no datagram' situation consistently: returning nil
> > +        -- and setting EAGAIN.
> > +        if size == 0 then
> > +            size = 1
> > +        end
> 
> I'm afraid there's an inherent race condition here. What if a new
> datagram of non-zero length arrives after you check the size here,
> but before you call recv?
> 

Fixed in the patch v4.

> > +    else
> > +        local iflags = get_iflags(internal.SEND_FLAGS, {'MSG_TRUNC', 'MSG_PEEK'})
> > +        assert(iflags ~= nil)
> > +        size = tonumber(ffi.C.recv(fd, nil, 0, iflags))
> > +    end
> > +
> > +    if size == -1 then
> > +        self._errno = boxerrno()
> > +        return nil
> > +    end
> > +
> > +    return size
> > +end
> > +
> >  local function socket_recv(self, size, flags)
> >      local fd = check_socket(self)
> >      local iflags = get_iflags(internal.SEND_FLAGS, flags)
> > @@ -778,7 +820,11 @@ local function socket_recv(self, size, flags)
> >          return nil
> >      end
> >  
> > -    size = size or 512
> > +    size = get_recv_size(self, size)
> > +    if size == nil then
> > +        return nil
> > +    end
> > +
> >      self._errno = nil
> >      local buf = ffi.new("char[?]", size)
> >  
> 

I'll send the patch v4 in the separate email soon.

Changes:

- fixed Vladimir's code style comments;
- fixed race condition on Mac OS;
- added test cases for zero length datagrams.

diff v3..v4:

diff --git a/src/lua/socket.lua b/src/lua/socket.lua
index 04e99a668..8f5e2926b 100644
--- a/src/lua/socket.lua
+++ b/src/lua/socket.lua
@@ -774,11 +774,10 @@ local function socket_send(self, octets, flags)
     return tonumber(res)
 end
 
--- Returns nil and sets EAGAIN when tries to determine the next datagram length
--- and there are no datagrams in the receive queue.
+-- Returns nil and sets EAGAIN when tries to determine the next
+-- datagram length and there are no datagrams in the receive
+-- queue.
 local function get_recv_size(self, size)
-    local fd = check_socket(self)
-
     if size ~= nil then
         return size
     end
@@ -788,15 +787,28 @@ local function get_recv_size(self, size)
     end
 
     -- Determine the next datagram length.
+    local fd = check_socket(self)
     self._errno = nil
     if jit.os == 'OSX' then
         size = self:getsockopt('SOL_SOCKET', 'SO_NREAD')
-        -- recv() with zero length buffer is always successful on Mac OS (at
-        -- least for valid UDP sockets). The assignment of size to 1 below
-        -- allows to handle 'no datagram' situation consistently: returning nil
-        -- and setting EAGAIN.
+        -- recv() with zero length buffer is always successful on
+        -- Mac OS (at least for valid UDP sockets), so we need
+        -- extra calls below to distinguish a zero length datagram
+        -- from no datagram situation to set EAGAIN correctly.
         if size == 0 then
-            size = 1
+            -- No datagram or zero length datagram: distinguish
+            -- them using message peek.
+            local iflags = get_iflags(internal.SEND_FLAGS, {'MSG_PEEK'})
+            assert(iflags ~= nil)
+            local buf = ffi.new('char[?]', 1)
+            size = tonumber(ffi.C.recv(fd, buf, 1, iflags))
+            -- Prevent race condition: proceed with the case when
+            -- a datagram of length > 0 has been arrived after the
+            -- getsockopt call above.
+            if size > 0 then
+                size = self:getsockopt('SOL_SOCKET', 'SO_NREAD')
+                assert(size > 0)
+            end
         end
     else
         local iflags = get_iflags(internal.SEND_FLAGS, {'MSG_TRUNC', 'MSG_PEEK'})
diff --git a/test/app/socket.result b/test/app/socket.result
index f2c73b305..729c1eb3f 100644
--- a/test/app/socket.result
+++ b/test/app/socket.result
@@ -2233,6 +2233,40 @@ s:close()
 --------------------------------------------------------------------------------
 -- gh-3619: socket.recvfrom crops UDP packets
 --------------------------------------------------------------------------------
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+function sendto_zero(self, host, port)
+    local fd = self:fd()
+
+    host = tostring(host)
+    port = tostring(port)
+
+    local addrbuf = ffi.new('char[128]') --[[ enough to fit any address ]]
+    local addr = ffi.cast('struct sockaddr *', addrbuf)
+    local addr_len = ffi.new('socklen_t[1]')
+    addr_len[0] = ffi.sizeof(addrbuf)
+
+    self._errno = nil
+    local res = ffi.C.lbox_socket_local_resolve(host, port, addr, addr_len)
+    if res == 0 then
+        res = ffi.C.sendto(fd, nil, 0, 0, addr, addr_len[0])
+    end
+
+    if res < 0 then
+        self._errno = boxerrno()
+        return nil
+    end
+
+    return tonumber(res)
+end;
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
 receiving_socket = socket('AF_INET', 'SOCK_DGRAM', 'udp')
 ---
 ...
@@ -2292,6 +2326,60 @@ e == errno.EAGAIN -- expected true
 ---
 - true
 ...
+-- case: recv, zero datagram
+sendto_zero(sending_socket, '127.0.0.1', receiving_socket_port)
+---
+- 0
+...
+received_message = receiving_socket:recv()
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_message == '' -- expected true
+---
+- true
+...
+received_message
+---
+- 
+...
+e == 0 -- expected true
+---
+- true
+...
+-- case: recvfrom, zero datagram
+sendto_zero(sending_socket, '127.0.0.1', receiving_socket_port)
+---
+- 0
+...
+received_message, from = receiving_socket:recvfrom()
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_message == '' -- expected true
+---
+- true
+...
+received_message
+---
+- 
+...
+from ~= nil -- expected true
+---
+- true
+...
+from.host == '127.0.0.1' -- expected true
+---
+- true
+...
+e == 0 -- expected true
+---
+- true
+...
 -- case: recv, no datagram, explicit size
 received_message = receiving_socket:recv(512)
 ---
@@ -2338,6 +2426,60 @@ e == errno.EAGAIN -- expected true
 ---
 - true
 ...
+-- case: recv, zero datagram, explicit size
+sendto_zero(sending_socket, '127.0.0.1', receiving_socket_port)
+---
+- 0
+...
+received_message = receiving_socket:recv(512)
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_message == '' -- expected true
+---
+- true
+...
+received_message
+---
+- 
+...
+e == 0 -- expected true
+---
+- true
+...
+-- case: recvfrom, zero datagram, explicit size
+sendto_zero(sending_socket, '127.0.0.1', receiving_socket_port)
+---
+- 0
+...
+received_message, from = receiving_socket:recvfrom(512)
+---
+...
+e = receiving_socket:errno()
+---
+...
+received_message == '' -- expected true
+---
+- true
+...
+received_message
+---
+- 
+...
+from ~= nil -- expected true
+---
+- true
+...
+from.host == '127.0.0.1' -- expected true
+---
+- true
+...
+e == 0 -- expected true
+---
+- true
+...
 message_len = 1025
 ---
 ...
diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua
index 840f042e4..9b896522b 100644
--- a/test/app/socket.test.lua
+++ b/test/app/socket.test.lua
@@ -740,6 +740,33 @@ s:close()
 -- gh-3619: socket.recvfrom crops UDP packets
 --------------------------------------------------------------------------------
 
+test_run:cmd("setopt delimiter ';'")
+function sendto_zero(self, host, port)
+    local fd = self:fd()
+
+    host = tostring(host)
+    port = tostring(port)
+
+    local addrbuf = ffi.new('char[128]') --[[ enough to fit any address ]]
+    local addr = ffi.cast('struct sockaddr *', addrbuf)
+    local addr_len = ffi.new('socklen_t[1]')
+    addr_len[0] = ffi.sizeof(addrbuf)
+
+    self._errno = nil
+    local res = ffi.C.lbox_socket_local_resolve(host, port, addr, addr_len)
+    if res == 0 then
+        res = ffi.C.sendto(fd, nil, 0, 0, addr, addr_len[0])
+    end
+
+    if res < 0 then
+        self._errno = boxerrno()
+        return nil
+    end
+
+    return tonumber(res)
+end;
+test_run:cmd("setopt delimiter ''");
+
 receiving_socket = socket('AF_INET', 'SOCK_DGRAM', 'udp')
 receiving_socket:bind('127.0.0.1', 0)
 receiving_socket_port = receiving_socket:name().port
@@ -761,6 +788,24 @@ from == nil -- expected true
 from
 e == errno.EAGAIN -- expected true
 
+-- case: recv, zero datagram
+sendto_zero(sending_socket, '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
+sendto_zero(sending_socket, '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 +822,24 @@ from == nil -- expected true
 from
 e == errno.EAGAIN -- expected true
 
+-- case: recv, zero datagram, explicit size
+sendto_zero(sending_socket, '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
+sendto_zero(sending_socket, '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)