From: Huw Davies Subject: Re: [PATCH 1/2] iphlpapi: Use a standalone buffer in IcmpSendEcho(). Message-Id: <20180621093512.GA8507@merlot.physics.ox.ac.uk> Date: Thu, 21 Jun 2018 10:35:15 +0100 In-Reply-To: References: On Fri, Jun 15, 2018 at 12:23:23PM +0800, Zhiyi Zhang wrote: > Fix https://bugs.winehq.org/show_bug.cgi?id=43252 > > The old implementation uses user provided buffer to receive > packet data, which is alway not enough, causing data corruptions > or incorrectly timeout. > > Signed-off-by: Zhiyi Zhang > --- > dlls/iphlpapi/icmp.c | 38 +++++++++++++++++++++++++++------- > dlls/iphlpapi/tests/iphlpapi.c | 21 +++++++++++++++---- > 2 files changed, 47 insertions(+), 12 deletions(-) > > diff --git a/dlls/iphlpapi/icmp.c b/dlls/iphlpapi/icmp.c > index ebc2f2b65c..7c91443598 100644 > --- a/dlls/iphlpapi/icmp.c > +++ b/dlls/iphlpapi/icmp.c > @@ -113,6 +113,9 @@ typedef struct { > #define IP_OPTS_DEFAULT 1 > #define IP_OPTS_CUSTOM 2 > > +#define MAXIPLEN 60 > +#define MAXICMPLEN 76 Out of interest, how did you get to 76? > + > /* The sequence number is unique process wide, so that all threads > * have a distinct sequence number. > */ > @@ -270,13 +273,14 @@ DWORD WINAPI IcmpSendEcho( > icmp_t* icp=(icmp_t*)IcmpHandle; > unsigned char* reqbuf; > int reqsize; > + unsigned char* repbuf; > + int repsize; > > struct icmp_echo_reply* ier; > struct ip* ip_header; > struct icmp* icmp_header; > char* endbuf; > int ip_header_len; > - int maxlen; > struct pollfd fdr; > DWORD send_time,recv_time; > struct sockaddr_in addr; > @@ -312,6 +316,16 @@ DWORD WINAPI IcmpSendEcho( > return 0; > } > > + /* max ip header + max icmp header and error data + reply size(max 65535 on Windows) */ > + /* FIXME: request size of 65535 is not supported yet because max buffer size of raw socket on linux is 32767 */ > + repsize=MAXIPLEN+MAXICMPLEN+(ReplySize&0xFFFF); > + repbuf=HeapAlloc(GetProcessHeap(), 0, repsize); > + if (reqbuf==NULL) { This should be repbuf. > + HeapFree(GetProcessHeap(), 0, reqbuf); > + SetLastError(ERROR_OUTOFMEMORY); > + return 0; > + } > + > icmp_header=(struct icmp*)reqbuf; > icmp_header->icmp_type=ICMP_ECHO; > icmp_header->icmp_code=0; > @@ -367,9 +381,7 @@ DWORD WINAPI IcmpSendEcho( > fdr.events = POLLIN; > addrlen=sizeof(addr); > ier=ReplyBuffer; > - ip_header=(struct ip *) ((char *) ReplyBuffer+sizeof(ICMP_ECHO_REPLY)); > endbuf=(char *) ReplyBuffer+ReplySize; > - maxlen=ReplySize-sizeof(ICMP_ECHO_REPLY); > > /* Send the packet */ > TRACE("Sending %d bytes (RequestSize=%d) to %s\n", reqsize, RequestSize, inet_ntoa(addr.sin_addr)); > @@ -407,10 +419,11 @@ DWORD WINAPI IcmpSendEcho( > } > > /* Get the reply */ > + ip_header=(struct ip*)repbuf; > ip_header_len=0; /* because gcc was complaining */ > while (poll(&fdr,1,Timeout)>0) { > recv_time = GetTickCount(); > - res=recvfrom(icp->sid, (char*)ip_header, maxlen, 0, (struct sockaddr*)&addr,&addrlen); > + res=recvfrom(icp->sid, (char*)repbuf, repsize, 0, (struct sockaddr*)&addr, &addrlen); > TRACE("received %d bytes from %s\n",res, inet_ntoa(addr.sin_addr)); > ier->Status=IP_REQ_TIMED_OUT; > > @@ -508,6 +521,12 @@ DWORD WINAPI IcmpSendEcho( > else Timeout = 0; > continue; > } else { > + /* Check free space, should be large enough for an ICMP_ECHO_REPLY and remainning icmp data */ > + if (endbuf-(char *)ier < sizeof(struct icmp_echo_reply)+(res-ip_header_len-ICMP_MINLEN)) { > + res=ier-(ICMP_ECHO_REPLY *)ReplyBuffer; > + SetLastError(IP_GENERAL_FAILURE); > + goto done; > + } > /* This is a reply to our packet */ > memcpy(&ier->Address,&ip_header->ip_src,sizeof(IPAddr)); > /* Status is already set */ > @@ -515,7 +534,7 @@ DWORD WINAPI IcmpSendEcho( > ier->DataSize=res-ip_header_len-ICMP_MINLEN; > ier->Reserved=0; > ier->Data=endbuf-ier->DataSize; > - memmove(ier->Data,((char*)ip_header)+ip_header_len+ICMP_MINLEN,ier->DataSize); > + memcpy(ier->Data, ((char *)ip_header)+ip_header_len+ICMP_MINLEN, ier->DataSize); > ier->Options.Ttl=ip_header->ip_ttl; > ier->Options.Tos=ip_header->ip_tos; > ier->Options.Flags=ip_header->ip_off >> 13; > @@ -523,7 +542,7 @@ DWORD WINAPI IcmpSendEcho( > if (ier->Options.OptionsSize!=0) { > ier->Options.OptionsData=(unsigned char *) ier->Data-ier->Options.OptionsSize; > /* FIXME: We are supposed to rearrange the option's 'source route' data */ > - memmove(ier->Options.OptionsData,((char*)ip_header)+ip_header_len,ier->Options.OptionsSize); > + memcpy(ier->Options.OptionsData, ((char *)ip_header)+ip_header_len, ier->Options.OptionsSize); > endbuf=(char*)ier->Options.OptionsData; > } else { > ier->Options.OptionsData=NULL; > @@ -531,9 +550,8 @@ DWORD WINAPI IcmpSendEcho( > } > > /* Prepare for the next packet */ > + endbuf-=ier->DataSize; > ier++; > - ip_header=(struct ip*)(((char*)ip_header)+sizeof(ICMP_ECHO_REPLY)); > - maxlen=endbuf-(char*)ip_header; > > /* Check out whether there is more but don't wait this time */ > Timeout=0; > @@ -542,6 +560,10 @@ DWORD WINAPI IcmpSendEcho( > res=ier-(ICMP_ECHO_REPLY*)ReplyBuffer; > if (res==0) > SetLastError(IP_REQ_TIMED_OUT); > + else > + SetLastError(NO_ERROR); This hunk looks like a separate change so should be a separate patch. Note, in general setting last error to NO_ERROR looks wrong, though it may be needed in this case. Huw.