[tarantool-patches] Re: [PATCH 2/3] socket: don't truncate a datagram in recv/recvfrom

[Alexander Turenko]

On Fri, Aug 24, 2018 at 06:24:39PM +0300, Vladimir Davydov wrote:
> 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.
> 

I agreed with the review, but as Yaroslov wrote, this part of the
patchset is not needed. So I'll skip the patch from the patchset and
will follow up with issue to the documentation team to describe the
behaviour properly (especially, this truncation case).

WBR, Alexander Turenko.