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

[Alexander Turenko <alexander.turenko@tarantool.org>]

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)