From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 29 Aug 2018 19:59:23 +0300 From: Alexander Turenko Subject: Re: [tarantool-patches] Re: [PATCH v3] socket: evaluate buffer size in recv / recvfrom Message-ID: <20180829165923.em56qofo4ioxd2ev@tkn_work_nb> References: <1c4959e92e1c768bf1bdb63409f46089c5948cbe.1535476618.git.alexander.turenko@tarantool.org> <20180829101651.o3s2knsmr53dbgf6@esperanza> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20180829101651.o3s2knsmr53dbgf6@esperanza> To: Vladimir Davydov Cc: tarantool-patches@freelists.org, Yaroslav Dynnikov List-ID: 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)