GitLab
gitlab-to-mail https://gitlab.winehq.org/project_2_bot started a new discussion https://gitlab.winehq.org/wine/wine/-/merge_requests/384#note_4039:
Zebediah Figura replied on the mailing list:
|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.|
|Sure.|
|> if (async->control) > { > + if (async->fixup_type == SOCK_PROT_FIXUP_ICMP_OVER_DGRAM) > + FIXME( "May return extra control headers.\n" ); > + What is this for?|
When initializing the fixup on server I am setting IP_RECVTTL, IP_RECVTOS and IP_PKTINFO socket options. If the app is calling recvmsg() it may get the headers it didn't request. My reasoning that such a situation doesn't deserve further code complication to avoid but deserves a fixme.
|> @@ -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?|
The async may be initiated on the server and delivered to the process (through SIGUSR1)�� before we set async->fixup_type. It might be not likely in practice, but I double verified that it is possible by injecting Sleep(...) before setting fixup type to async structure from server. So I need to make sure that fixup_type in async structure is correctly set before try_recv() can be reached. So idea is, fixup_type is "no fixup" by default, no extra server call is involved in normal case. But if fixup is required and client does not yet know about it server doesn't�� do anything and just returns fixup type with STATUS_MORE_PROCESSING_REQUIRED.
|> 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?|
Well, originally I did something between the lines of this (had icmp_over_dgram field in request / reply). But then I looked at that and it felt odd that general recv_socket() has some special icmp related fields, it felt like naming that "fixup_type" tells what it is without going into very specific ICMP fixup details looking weird at this level. If you feel like it is better to name it, e. g., "icmp_over_dgram" I can change it back. I don't currently have any other types of fixups in mind.