[PATCH 3/4] ntdll: Support SOCK_RAW / IPPROTO_ICMP fallback over SOCK_DGRAM.

[Zebediah Figura]

On 7/5/22 14:53, Paul Gofman wrote:

@@ -602,9 +623,68 @@ static NTSTATUS try_recv( int fd, struct 
async_recv_ioctl *async, ULONG_PTR *siz
      }

      status = (hdr.msg_flags & MSG_TRUNC) ? STATUS_BUFFER_OVERFLOW : 
STATUS_SUCCESS;
+    if (async->fixup_type == SOCK_PROT_FIXUP_ICMP_OVER_DGRAM)
+    {
+        unsigned int tot_len = sizeof(struct ip_hdr) + ret;
+        size_t len = hdr.msg_iov[0].iov_len;
+        char *buf = hdr.msg_iov[0].iov_base;
+        struct cmsghdr *cmsg;
+        struct ip_hdr ip_h;
+
+        if (ret + sizeof(ip_h) > len)
+            status = STATUS_BUFFER_OVERFLOW;
+
+        if (len < sizeof(ip_h))
+        {
+            ret = len;
+        }
+        else
+        {
+            ret = min( ret, len - sizeof(ip_h) );
+            memmove( buf + sizeof(ip_h), buf, ret );
+            ret += sizeof(ip_h);
+        }
+        memset( &ip_h, 0, sizeof(ip_h) );
+        ip_h.v_hl = (4 << 4) | (sizeof(ip_h) >> 2);
+        ip_h.tot_len = htons( tot_len );
+        ip_h.protocol = 1;
+        ip_h.saddr = unix_addr.in.sin_addr.s_addr;
+
+        for (cmsg = CMSG_FIRSTHDR( &hdr ); cmsg; cmsg = CMSG_NXTHDR( 
&hdr, cmsg ))
+        {
+            if (cmsg->cmsg_level != IPPROTO_IP) continue;
+            switch (cmsg->cmsg_type)
+            {
+#if defined(IP_TTL)
+                case IP_TTL:
+                    ip_h.ttl = *(BYTE *)CMSG_DATA( cmsg );
+                    break;
+#endif
+#if defined(IP_TOS)
+                case IP_TOS:
+                    ip_h.tos = *(BYTE *)CMSG_DATA( cmsg );
+                    break;
+#endif
+#if defined(IP_PKTINFO)
+                case IP_PKTINFO:
+                {
+                    struct in_pktinfo *info;
+
+                    info = (struct in_pktinfo *)CMSG_DATA( cmsg );
+                    ip_h.daddr = info->ipi_addr.s_addr;
+                    break;
+                }
+#endif
+            }
+        }
+        memcpy( buf, &ip_h, min( sizeof(ip_h), len ));
+    }

Would you mind making this a helper function? try_recv() is already 
large and this seems to be pretty self-contained logic.

>       if (async->control)
>       {
> +        if (async->fixup_type == SOCK_PROT_FIXUP_ICMP_OVER_DGRAM)
> +            FIXME( "May return extra control headers.\n" );
> +

What is this for?

> @@ -737,17 +818,26 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi
>           }
>       }
>   
> -    SERVER_START_REQ( recv_socket )
> +    do
>       {
> -        req->force_async = force_async;
> -        req->async  = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) );
> -        req->oob    = !!(unix_flags & MSG_OOB);
> -        status = wine_server_call( req );
> -        wait_handle = wine_server_ptr_handle( reply->wait );
> -        options     = reply->options;
> -        nonblocking = reply->nonblocking;
> -    }
> -    SERVER_END_REQ;
> +        SERVER_START_REQ( recv_socket )
> +        {
> +            req->force_async = force_async;
> +            req->async  = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) );
> +            req->oob    = !!(unix_flags & MSG_OOB);
> +            req->fixup_type = async->fixup_type;
> +            status = wine_server_call( req );
> +            if (status == STATUS_MORE_PROCESSING_REQUIRED)
> +            {
> +                async->fixup_type = reply->fixup_type;
> +                continue;
> +            }
> +            wait_handle = wine_server_ptr_handle( reply->wait );
> +            options     = reply->options;
> +            nonblocking = reply->nonblocking;
> +        }
> +        SERVER_END_REQ;
> +    } while (status == STATUS_MORE_PROCESSING_REQUIRED);
>   
>       alerted = status == STATUS_ALERTED;
>       if (alerted)

I'm missing something; why do we need to call this in a loop? Why does 
the client needs to communicate the fixup type to the server in the 
first place?

> diff --git a/server/protocol.def b/server/protocol.def
> index a8beccaf468..e1e5f3d9faa 100644
> --- a/server/protocol.def
> +++ b/server/protocol.def
> @@ -1446,17 +1446,23 @@ enum server_fd_type
>       file_pos_t   count;         /* count of bytes to unlock */
>   @END
>   
> -
>   /* Perform a recv on a socket */
>   @REQ(recv_socket)
>       int          oob;           /* are we receiving OOB data? */
>       async_data_t async;         /* async I/O parameters */
>       int          force_async;   /* Force asynchronous mode? */
> +    int          fixup_type;    /* protocol fixup type returned previously */
>   @REPLY
>       obj_handle_t wait;          /* handle to wait on for blocking recv */
>       unsigned int options;       /* device open options */
>       int          nonblocking;   /* is socket non-blocking? */
> +    int          fixup_type;    /* protocol fixup type (see below) */
>   @END
> +enum sock_prot_fixup_type
> +{
> +    SOCK_PROT_FIXUP_NONE,
> +    SOCK_PROT_FIXUP_ICMP_OVER_DGRAM,
> +};

Making this an enum strikes me as odd in general. A bitmask might make 
more sense, but frankly I'd just leave it as a dedicated field until we 
have something else that needs fixups.

Do you have any plans for introducing other types of fixups?