[PATCH 1/2] iphlpapi: Use a standalone buffer in IcmpSendEcho().

[Zhiyi Zhang]

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 <zzhang at codeweavers.com>
>> ---
>>  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.
>