Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Alexander Turenko <alexander.turenko@tarantool.org>
Cc: tarantool-patches@freelists.org,
	Yaroslav Dynnikov <yaroslav.dynnikov@tarantool.org>
Subject: Re: [PATCH v3] socket: evaluate buffer size in recv / recvfrom
Date: Wed, 29 Aug 2018 13:16:51 +0300	[thread overview]
Message-ID: <20180829101651.o3s2knsmr53dbgf6@esperanza> (raw)
In-Reply-To: <1c4959e92e1c768bf1bdb63409f46089c5948cbe.1535476618.git.alexander.turenko@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)
>  

  reply	other threads:[~2018-08-29 10:16 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-28 18:21 Alexander Turenko
2018-08-29 10:16 ` Vladimir Davydov [this message]
2018-08-29 16:59   ` [tarantool-patches] " Alexander Turenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180829101651.o3s2knsmr53dbgf6@esperanza \
    --to=vdavydov.dev@gmail.com \
    --cc=alexander.turenko@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=yaroslav.dynnikov@tarantool.org \
    --subject='Re: [PATCH v3] socket: evaluate buffer size in recv / recvfrom' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox