From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Fri, 24 Aug 2018 18:24:39 +0300 From: Vladimir Davydov Subject: Re: [PATCH 2/3] socket: don't truncate a datagram in recv/recvfrom Message-ID: <20180824152439.wjs7nl3el2i72bcp@esperanza> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Alexander Turenko Cc: tarantool-patches@freelists.org, Yaroslav Dynnikov List-ID: On Fri, Aug 24, 2018 at 05:47:38AM +0300, Alexander Turenko wrote: > When the datagram length is larger then the buffer size we discard the > datagram, return nil as the message, set errno to EMSGSIZE, but still > return `from` table (in case of recvfrom). The commit changes behaviour > of socket:recv and socket:recvfrom methods in case of a UDP socket. They > still truncate the input in case of a TCP socket (because of stream > nature of TCP). > > On Linux we set MSG_TRUNC for recv / recvfrom calls to receive the real > length of the datagram. On Mac OS we call getsockopt(fd, SOL_SOCKET, > SO_NREAD, &val, &len) before the recv / recvfrom call. These extra steps > are avoided for TCP sockets, because MSG_TRUNC leads to discarding the > input (Linux) and to avoid extra getsockopt syscall (Mac OS). > > Fixes #3619. > --- > src/lua/socket.c | 17 +++++ > src/lua/socket.lua | 55 +++++++++++----- > test/app/socket.result | 139 ++++++++++++++++++++++++++++++++++++++- > test/app/socket.test.lua | 44 +++++++++++++ > 4 files changed, 238 insertions(+), 17 deletions(-) > > diff --git a/src/lua/socket.c b/src/lua/socket.c > index bf23eae1f..c716979f4 100644 > --- a/src/lua/socket.c > +++ b/src/lua/socket.c > @@ -953,6 +953,13 @@ lbox_socket_recvfrom(struct lua_State *L) > int fh = lua_tointeger(L, 1); > int size = lua_tointeger(L, 2); > int flags = lua_tointeger(L, 3); > + int is_udp = lua_toboolean(L, 4); > +#ifdef TARGET_OS_DARWIN > + int dgram_length = lua_tointeger(L, 5); > +#else > + if (is_udp) > + flags |= MSG_TRUNC; > +#endif > > struct sockaddr_storage fa; > socklen_t len = sizeof(fa); > @@ -971,6 +978,16 @@ lbox_socket_recvfrom(struct lua_State *L) > free(buf); > lua_pushnil(L); > return 1; > +#ifdef TARGET_OS_DARWIN > + } else if ((is_udp && dgram_length > size) || (!is_udp && res > size)) { > +#else > + } else if (res > size) { > +#endif > + free(buf); > + lua_pushnil(L); > + lbox_socket_push_addr(L, (struct sockaddr *)&fa, len); > + errno = EMSGSIZE; > + return 2; > } > > lua_pushcfunction(L, lbox_socket_recvfrom_wrapper); > diff --git a/src/lua/socket.lua b/src/lua/socket.lua > index 946e11e9e..eed764f96 100644 > --- a/src/lua/socket.lua > +++ b/src/lua/socket.lua > @@ -805,6 +805,26 @@ local function socket_dgram_length(self) > return len > end > > +-- @retval size (size or 512) for tcp, (size or dgram_length) for udp > +-- @retval dgram_length or nil > +local function socket_recv_buffer_size_internal(self, size, is_udp) > + if not is_udp then > + return size or 512 > + end > + > + -- we obligated to ask for dgram_length on OS X, because we cannot use > + -- MSG_TRUNC with the recv / recvfrom later > + local dgram_length > + if size == nil or jit.os == 'OSX' then > + dgram_length = self:dgram_length() > + if dgram_length == nil then > + return nil > + end > + end > + > + return size or dgram_length, dgram_length > +end > + I don't like how the logic is spread between Lua and C. Let's try to put everything in C. I suggest to do the following. First, let's add a wrapper around recvfrom to lua/socket.c. Let's call it do_recvfrom, but the name is not of an importance here. The function will take a buffer, its size, flags, and a boolean flag whether a datagram is expected or not. In case a datagram is expected, do_recvfrom will auto-detect the size in case size == 0. If size != 0, it should check that the message fits in the buffer with MSG_TRUNC or getsockopt depending on the OS. I think it's possible to implement this function with just one ifdef TARGET_OS_DARWIN. Next, we implement both socket.recv and socket.recvfrom in C with the aid of this function. They will differ only by the Lua return value. I'm not 100% sure it's the right way, but as I said spreading logic doesn't look good to me. Please try to gather all related bits one place.