From: Zhiyi Zhang Subject: Re: [PATCH 1/2] iphlpapi: Use a standalone buffer in IcmpSendEcho(). Message-Id: <6d4cb499-a5d5-ac77-5a42-5252fefb93b6@codeweavers.com> Date: Thu, 21 Jun 2018 17:51:13 +0800 In-Reply-To: <20180621093512.GA8507@merlot.physics.ox.ac.uk> References: <20180621093512.GA8507@merlot.physics.ox.ac.uk> On Thu 6 21 17:35, Huw Davies wrote: > 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? > That was copied from https://github.com/iputils/iputils/blob/master/ping.c#L76 >> + >> /* 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. >