[tarantool-patches] Re: [PATCH v3] socket: evaluate buffer size in recv / recvfrom
Alexander Turenko
alexander.turenko at tarantool.org
Wed Aug 29 19:59:23 MSK 2018
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)
More information about the Tarantool-patches
mailing list