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

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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.