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

[Huw Davies]

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.