[PATCH 0/1] Fix MSG_WAITALL support

[PATCH 0/1] Fix MSG_WAITALL support

[Ken Brown]

It looks to me like there's been a bug in the MSG_WAITALL support for
AF_INET and AF_LOCAL sockets ever since that support was first
introduced 13 years ago in commit 023a2fa7.  If I'm right, MSG_WAITALL
has never worked.

This patch fixes it.  I'll push it in a few days if no one sees
anything wrong with it.

In a followup email I'll show how I tested it.

Ken Brown (1):
  Cygwin: AF_INET and AF_LOCAL: recv_internal: fix MSG_WAITALL support

 winsup/cygwin/fhandler_socket_inet.cc  | 2 +-
 winsup/cygwin/fhandler_socket_local.cc | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.28.0

[PATCH 1/1] Cygwin: AF_INET and AF_LOCAL: recv_internal: fix MSG_WAITALL support

[Ken Brown]

If MSG_WAITALL is set, recv_internal calls WSARecv or WSARecvFrom in a
loop, in an effort to fill all the scatter-gather buffers.  The test
for whether all the buffers are full was previously incorrect.
---
 winsup/cygwin/fhandler_socket_inet.cc  | 2 +-
 winsup/cygwin/fhandler_socket_local.cc | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/fhandler_socket_inet.cc b/winsup/cygwin/fhandler_socket_inet.cc
index 71e92e341..bc08d3cf1 100644
--- a/winsup/cygwin/fhandler_socket_inet.cc
+++ b/winsup/cygwin/fhandler_socket_inet.cc
@@ -1208,7 +1208,7 @@ fhandler_socket_inet::recv_internal (LPWSAMSG wsamsg, bool use_recvmsg)
 		  --wsacnt;
 		}
 	    }
-	  if (!wret)
+	  if (!wsacnt)
 	    break;
 	}
       else if (WSAGetLastError () != WSAEWOULDBLOCK)
diff --git a/winsup/cygwin/fhandler_socket_local.cc b/winsup/cygwin/fhandler_socket_local.cc
index 8bfba225a..c94bf828f 100644
--- a/winsup/cygwin/fhandler_socket_local.cc
+++ b/winsup/cygwin/fhandler_socket_local.cc
@@ -1212,7 +1212,7 @@ fhandler_socket_local::recv_internal (LPWSAMSG wsamsg, bool use_recvmsg)
 		  --wsacnt;
 		}
 	    }
-	  if (!wret)
+	  if (!wsacnt)
 	    break;
 	}
       else if (WSAGetLastError () != WSAEWOULDBLOCK)
-- 
2.28.0

Re: [PATCH 0/1] Fix MSG_WAITALL support

[Ken Brown]

[-- Attachment #1: Type: text/plain, Size: 1695 bytes --]

On 10/12/2020 2:02 PM, Ken Brown via Cygwin-patches wrote:
> It looks to me like there's been a bug in the MSG_WAITALL support for
> AF_INET and AF_LOCAL sockets ever since that support was first
> introduced 13 years ago in commit 023a2fa7.  If I'm right, MSG_WAITALL
> has never worked.
> 
> This patch fixes it.  I'll push it in a few days if no one sees
> anything wrong with it.
> 
> In a followup email I'll show how I tested it.

Attached are slight variants of the server/client programs from Section 57.2 of 
Kerrisk's book, "The Linux Programming Interface".  The only essential 
difference is that I've changed the server program to (a) use a small buffer 
(size 10 instead of 100) and (b) use 'recv' with the MSG_WAITALL flag instead of 
'read'.  The 'recv' call shouldn't return until it reads 10 bytes.

To test, run waitall_sv in one terminal and waitall_cl in a second.  Type 
something in the second terminal (followed by RET), and it should be echoed in 
the first.  But because of the MSG_WAITALL flag, the echoing shouldn't occur 
until 10 bytes have been written.  For example, if I type "abcd<RET>" in the 
second terminal and then do it again, I should see the following:

# Terminal 2:
$ ./waitall_cl
abcd
abcd

# Terminal 1:
$ ./waitall_sv
abcd
abcd

Here the echoing in Terminal 1 shouldn't occur until I've typed both "abcd" 
lines in Terminal 2.

[Note that there is a newline character after each "abcd", so "abcd<RET>" is 5 
bytes long, and the two lines together are 10 bytes long.]

Before I apply my patch, each line typed in Terminal 2 is immediately echoed in 
Terminal 1.  After I apply the patch, the echoing doesn't occur until I've typed 
both lines.

Ken

[-- Attachment #2: waitall_sv.c --]
[-- Type: text/plain, Size: 1461 bytes --]

#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <errno.h>
#include <sys/un.h>
#include <sys/socket.h>

#define SV_SOCK_PATH "/tmp/waitall"

#define BUF_SIZE 10

#define BACKLOG 5

int
main ()
{
    struct sockaddr_un addr;
    int sfd, cfd;
    ssize_t nread;
    char buf[BUF_SIZE];

    if ((sfd = socket (AF_UNIX, SOCK_STREAM, 0)) < 0)
      {
	perror ("socket");
	exit (1);
      }
    if (remove (SV_SOCK_PATH) < 0 && errno != ENOENT)
      {
	perror ("remove");
        exit (1);
      }

    memset (&addr, 0, sizeof (struct sockaddr_un));
    addr.sun_family = AF_UNIX;
    strncpy (addr.sun_path, SV_SOCK_PATH, sizeof (addr.sun_path) - 1);

    if (bind (sfd, (struct sockaddr *) &addr, sizeof (struct sockaddr_un)) < 0)
      {
	perror ("bind");
	exit (1);
      }

    if (listen (sfd, BACKLOG) < 0)
      {
        perror ("listen");
	exit (1);
      }

    while (1)
      {
        cfd = accept (sfd, NULL, NULL);
        if (cfd < 0)
	  {
            perror ("accept");
	    exit (1);
	  }

        /* Transfer data from connected socket to stdout until EOF. */
        while ((nread = recv (cfd, buf, BUF_SIZE, MSG_WAITALL)) > 0)
	  if (write (STDOUT_FILENO, buf, nread) != nread)
	    {
	      perror ("partial/failed write");
	      exit (1);
	    }

        if (nread < 0)
	  {
            perror ("read");
	    exit (1);
	  }

        if (close (cfd) < 0)
	  {
            perror ("close");
	    exit (1);
	  }
      }
}

[-- Attachment #3: waitall_cl.c --]
[-- Type: text/plain, Size: 944 bytes --]

#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <errno.h>
#include <sys/un.h>
#include <sys/socket.h>

#define SV_SOCK_PATH "/tmp/waitall"

#define BUF_SIZE 100

int
main ()
{
    struct sockaddr_un addr;
    int sfd;
    ssize_t nread;
    char buf[BUF_SIZE];

    if ((sfd = socket (AF_UNIX, SOCK_STREAM, 0)) < 0)
      {
	perror ("socket");
	exit (1);
      }

    memset (&addr, 0, sizeof(struct sockaddr_un));
    addr.sun_family = AF_UNIX;
    strncpy (addr.sun_path, SV_SOCK_PATH, sizeof(addr.sun_path) - 1);

    if (connect (sfd, (struct sockaddr *) &addr,
		sizeof (struct sockaddr_un)) < 0)
      {
        perror ("connect");
	exit (1);
      }

    /* Copy stdin to socket. */
    while ((nread = read (STDIN_FILENO, buf, BUF_SIZE)) > 0)
      if (write (sfd, buf, nread) != nread)
	{
	  perror ("partial/failed write");
	  exit (1);
	}

    if (nread < 0)
      {
        perror ("read");
	exit (1);
      }
}

Re: [PATCH 0/1] Fix MSG_WAITALL support

[Ken Brown]

On 10/12/2020 2:02 PM, Ken Brown via Cygwin-patches wrote:
> It looks to me like there's been a bug in the MSG_WAITALL support for
> AF_INET and AF_LOCAL sockets ever since that support was first
> introduced 13 years ago in commit 023a2fa7.  If I'm right, MSG_WAITALL
> has never worked.
> 
> This patch fixes it.  I'll push it in a few days if no one sees
> anything wrong with it.
> 
> In a followup email I'll show how I tested it.

Hi Corinna,

I know I said I'd push this in a few days, but that was when I thought you'd be 
gone for a while longer.

Does the patch look OK?

Ken

Re: [PATCH 0/1] Fix MSG_WAITALL support

[Corinna Vinschen]

On Oct 22 15:26, Ken Brown via Cygwin-patches wrote:
> On 10/12/2020 2:02 PM, Ken Brown via Cygwin-patches wrote:
> > It looks to me like there's been a bug in the MSG_WAITALL support for
> > AF_INET and AF_LOCAL sockets ever since that support was first
> > introduced 13 years ago in commit 023a2fa7.  If I'm right, MSG_WAITALL
> > has never worked.
> > 
> > This patch fixes it.  I'll push it in a few days if no one sees
> > anything wrong with it.
> > 
> > In a followup email I'll show how I tested it.
> 
> Hi Corinna,
> 
> I know I said I'd push this in a few days, but that was when I thought you'd
> be gone for a while longer.
> 
> Does the patch look OK?

Sure!  I mean, you tested it and it fixing a problem, right?


Thanks,
Corinna