From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Wed, 29 Aug 2018 13:16:51 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [PATCH v3] socket: evaluate buffer size in recv / recvfrom
Message-ID: <20180829101651.o3s2knsmr53dbgf6@esperanza>
References: <1c4959e92e1c768bf1bdb63409f46089c5948cbe.1535476618.git.alexander.turenko@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1c4959e92e1c768bf1bdb63409f46089c5948cbe.1535476618.git.alexander.turenko@tarantool.org>
To: Alexander Turenko <alexander.turenko@tarantool.org>
Cc: tarantool-patches@freelists.org, Yaroslav Dynnikov <yaroslav.dynnikov@tarantool.org>
List-ID: <tarantool-patches.dev.tarantool.org>

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.

> +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?

> +
> +    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?

> +    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)
>