[Qemu-devel] [PATCH] socket: fix blocking udp recvfrom.

[Qemu-devel] [PATCH] socket: fix blocking udp recvfrom.

[llyzs]

Sometimes sorecvfrom() is called from slirp.c because revents == G_IO_IN,
however inside sorecvfrom() function, ioctlsocket() returns 0 bytes available
and recvfrom could be blocking indefinitely. This adds a non-blocking flag to
recvfrom and checks data availability.
---
 slirp/socket.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/slirp/socket.c b/slirp/socket.c
index c01d8696af..ea30478ce6 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -581,7 +581,7 @@ sorecvfrom(struct socket *so)
          }
          /* } */

-         m->m_len = recvfrom(so->s, m->m_data, len, 0,
+         m->m_len = recvfrom(so->s, m->m_data, len, MSG_DONTWAIT,
                              (struct sockaddr *)&addr, &addrlen);
          DEBUG_MISC((dfd, " did recvfrom %d, errno = %d-%s\n",
                      m->m_len, errno,strerror(errno)));
@@ -618,6 +618,8 @@ sorecvfrom(struct socket *so)
              break;
            }
            m_free(m);
+         } else if (m->m_len==0) {
+           m_free(m);
          } else {
          /*
           * Hack: domain name lookup will be used the most for UDP,
-- 
2.20.1

Re: [Qemu-devel] [PATCH] socket: fix blocking udp recvfrom.

[Samuel Thibault]

Hello,

llyzs, le jeu. 28 févr. 2019 19:59:12 +0800, a ecrit:
> Sometimes sorecvfrom() is called from slirp.c because revents == G_IO_IN,
> however inside sorecvfrom() function, ioctlsocket() returns 0 bytes available
> and recvfrom could be blocking indefinitely. This adds a non-blocking flag to
> recvfrom and checks data availability.

When ioctlsocket() returns 0 bytes available, we could as well just
immediately return, without even calling recvfrom. We could just move
that ioctlsocket call above the m_get() call, so we can just return
without having to clean up anything.

This however still looks weird. AFAIK, G_IO_IN means that we can make
one recv call and be sure that it won't block. Do you have an idea why
it wouldn't necessarily be the case?

Samuel

Re: [Qemu-devel] [PATCH] socket: fix blocking udp recvfrom.

[llyzs]

Hi,

I believe this is because UDP packet can have 0 length payload. I
quote some resources here:
https://stackoverflow.com/questions/12505892/under-linux-can-recv-ever-return-0-on-udp
https://stackoverflow.com/questions/5307031/how-to-detect-receipt-of-a-0-length-udp-datagram

I also found out that sometimes ioctlsocket() call can return error
(probably caused by my unstable wifi linux driver), in which case we
can also return immediately.

I will submit a new version of the patch according to your suggestions.

On Fri, 1 Mar 2019 at 04:51, Samuel Thibault <samuel.thibault@gnu.org> wrote:
>
> Hello,
>
> llyzs, le jeu. 28 févr. 2019 19:59:12 +0800, a ecrit:
> > Sometimes sorecvfrom() is called from slirp.c because revents == G_IO_IN,
> > however inside sorecvfrom() function, ioctlsocket() returns 0 bytes available
> > and recvfrom could be blocking indefinitely. This adds a non-blocking flag to
> > recvfrom and checks data availability.
>
> When ioctlsocket() returns 0 bytes available, we could as well just
> immediately return, without even calling recvfrom. We could just move
> that ioctlsocket call above the m_get() call, so we can just return
> without having to clean up anything.
>
> This however still looks weird. AFAIK, G_IO_IN means that we can make
> one recv call and be sure that it won't block. Do you have an idea why
> it wouldn't necessarily be the case?
>
> Samuel

Re: [Qemu-devel] [PATCH] socket: fix blocking udp recvfrom.

[Samuel Thibault]

llyzs, le ven. 01 mars 2019 12:27:31 +0800, a ecrit:
> I believe this is because UDP packet can have 0 length payload.

Ah, right, that one makes sense indeed.

> I also found out that sometimes ioctlsocket() call can return error
> (probably caused by my unstable wifi linux driver), in which case we
> can also return immediately.

We should check for the value returned by ioctlsocket() then, otherwise
the value of n is undefined.

Samuel

[Qemu-devel] [PATCHv2] slirp: check for ioctlsocket error and 0-length udp payload.

[llyzs]

Sometimes sorecvfrom() is called from slirp.c because revents == G_IO_IN,
but there is 0 bytes available and recvfrom could be blocking indefinitely.
This is likely due to 0-length udp payload. This also adds an error
checking for ioctlsocket.

Signed-off-by: Vic Lee <llyzs.vic@gmail.com>
---
 slirp/socket.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/slirp/socket.c b/slirp/socket.c
index c01d8696af..a624d829e3 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -549,6 +549,15 @@ sorecvfrom(struct socket *so)
           int n;
 #endif

+         if (ioctlsocket(so->s, FIONREAD, &n) != 0) {
+             DEBUG_MISC((dfd," ioctlsocket errno = %d-%s\n",
+                         errno,strerror(errno)));
+             return;
+         }
+         if (n == 0) {
+             return;
+         }
+
          m = m_get(so->slirp);
          if (!m) {
              return;
@@ -572,7 +581,6 @@ sorecvfrom(struct socket *so)
           */
          len = M_FREEROOM(m);
          /* if (so->so_fport != htons(53)) { */
-         ioctlsocket(so->s, FIONREAD, &n);

          if (n > len) {
            n = (m->m_data - m->m_dat) + m->m_len + n + 1;
-- 
2.20.1