Zhiyi Zhang : iphlpapi: Don't use the client buffer in IcmpSendEcho().

[Alexandre Julliard]

Module: wine
Branch: master
Commit: 417e996e97ebc3ff47d986e322dbaf3b3a83b91d
URL:    https://source.winehq.org/git/wine.git/?a=commit;h=417e996e97ebc3ff47d986e32...

Author: Zhiyi Zhang <zzhang(a)codeweavers.com>
Date:   Sun Aug  5 11:04:43 2018 +0800

iphlpapi: Don't use the client buffer in IcmpSendEcho().

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=43252
Signed-off-by: Zhiyi Zhang <zzhang(a)codeweavers.com>
Signed-off-by: Alexandre Julliard <julliard(a)winehq.org>

---

 dlls/iphlpapi/icmp.c           | 48 +++++++++++++++++++++++++-----------------
 dlls/iphlpapi/tests/iphlpapi.c | 21 ++++++++++++++----
 2 files changed, 46 insertions(+), 23 deletions(-)

diff --git a/dlls/iphlpapi/icmp.c b/dlls/iphlpapi/icmp.c
index ebc2f2b..0d80248 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
+
 /* The sequence number is unique process wide, so that all threads
  * have a distinct sequence number.
  */
@@ -268,15 +271,14 @@ DWORD WINAPI IcmpSendEcho(
     )
 {
     icmp_t* icp=(icmp_t*)IcmpHandle;
-    unsigned char* reqbuf;
-    int reqsize;
+    unsigned char *buffer;
+    int reqsize, 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;
@@ -306,20 +308,23 @@ DWORD WINAPI IcmpSendEcho(
     seq=InterlockedIncrement(&icmp_sequence) & 0xFFFF;
 
     reqsize=ICMP_MINLEN+RequestSize;
-    reqbuf=HeapAlloc(GetProcessHeap(), 0, reqsize);
-    if (reqbuf==NULL) {
+    /* 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 + min( 65535, ReplySize );
+    buffer = HeapAlloc(GetProcessHeap(), 0, max( repsize, reqsize ));
+    if (buffer == NULL) {
         SetLastError(ERROR_OUTOFMEMORY);
         return 0;
     }
 
-    icmp_header=(struct icmp*)reqbuf;
+    icmp_header=(struct icmp*)buffer;
     icmp_header->icmp_type=ICMP_ECHO;
     icmp_header->icmp_code=0;
     icmp_header->icmp_cksum=0;
     icmp_header->icmp_id=id;
     icmp_header->icmp_seq=seq;
-    memcpy(reqbuf+ICMP_MINLEN, RequestData, RequestSize);
-    icmp_header->icmp_cksum=cksum=in_cksum((u_short*)reqbuf,reqsize);
+    memcpy(buffer+ICMP_MINLEN, RequestData, RequestSize);
+    icmp_header->icmp_cksum=cksum=in_cksum((u_short*)buffer,reqsize);
 
     addr.sin_family=AF_INET;
     addr.sin_addr.s_addr=DestinationAddress;
@@ -367,26 +372,22 @@ 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));
 #if 0
     if (TRACE_ON(icmp)){
-        unsigned char* buf=(unsigned char*)reqbuf;
         int i;
         printf("Output buffer:\n");
         for (i=0;i<reqsize;i++)
-            printf("%2x,", buf[i]);
+            printf("%2x,", buffer[i]);
         printf("\n");
     }
 #endif
 
     send_time = GetTickCount();
-    res=sendto(icp->sid, reqbuf, reqsize, 0, (struct sockaddr*)&addr, sizeof(addr));
-    HeapFree(GetProcessHeap (), 0, reqbuf);
+    res=sendto(icp->sid, buffer, reqsize, 0, (struct sockaddr*)&addr, sizeof(addr));
     if (res<0) {
         if (errno==EMSGSIZE)
             SetLastError(IP_PACKET_TOO_BIG);
@@ -403,14 +404,16 @@ DWORD WINAPI IcmpSendEcho(
                 SetLastError(IP_GENERAL_FAILURE);
             }
         }
+        HeapFree(GetProcessHeap(), 0, buffer);
         return 0;
     }
 
     /* Get the reply */
+    ip_header=(struct ip*)buffer;
     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, buffer, 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 +511,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 +524,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 +532,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 +540,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 +550,8 @@ DWORD WINAPI IcmpSendEcho(
     res=ier-(ICMP_ECHO_REPLY*)ReplyBuffer;
     if (res==0)
         SetLastError(IP_REQ_TIMED_OUT);
+done:
+    HeapFree(GetProcessHeap(), 0, buffer);
     TRACE("received %d replies\n",res);
     return res;
 }
diff --git a/dlls/iphlpapi/tests/iphlpapi.c b/dlls/iphlpapi/tests/iphlpapi.c
index d5613d7..8d71c74 100644
--- a/dlls/iphlpapi/tests/iphlpapi.c
+++ b/dlls/iphlpapi/tests/iphlpapi.c
@@ -951,6 +951,8 @@ static void testIcmpSendEcho(void)
     char senddata[32], replydata[sizeof(senddata) + sizeof(ICMP_ECHO_REPLY)];
     DWORD ret, error, replysz = sizeof(replydata);
     IPAddr address;
+    ICMP_ECHO_REPLY *reply;
+    INT i;
 
     if (!pIcmpSendEcho || !pIcmpCreateFile)
     {
@@ -1038,12 +1040,10 @@ todo_wine
     replysz = sizeof(replydata) - 1;
     ret = pIcmpSendEcho(icmp, address, senddata, sizeof(senddata), NULL, replydata, replysz, 1000);
     error = GetLastError();
-    todo_wine {
     ok (!ret, "IcmpSendEcho succeeded unexpectedly\n");
     ok (error == IP_GENERAL_FAILURE
         || broken(error == IP_BUF_TOO_SMALL) /* <= 2003 */,
         "expected 11050, got %d\n", error);
-    }
 
     SetLastError(0xdeadbeef);
     replysz = sizeof(ICMP_ECHO_REPLY);
@@ -1056,7 +1056,6 @@ todo_wine
     replysz = sizeof(ICMP_ECHO_REPLY) + ICMP_MINLEN;
     ret = pIcmpSendEcho(icmp, address, senddata, ICMP_MINLEN, NULL, replydata, replysz, 1000);
     error = GetLastError();
-todo_wine
     ok (ret, "IcmpSendEcho failed unexpectedly with error %d\n", error);
 
     SetLastError(0xdeadbeef);
@@ -1064,7 +1063,6 @@ todo_wine
     ret = pIcmpSendEcho(icmp, address, senddata, ICMP_MINLEN + 1, NULL, replydata, replysz, 1000);
     error = GetLastError();
     ok (!ret, "IcmpSendEcho succeeded unexpectedly\n");
-todo_wine
     ok (error == IP_GENERAL_FAILURE
         || broken(error == IP_BUF_TOO_SMALL) /* <= 2003 */,
         "expected 11050, got %d\n", error);
@@ -1111,6 +1109,21 @@ todo_wine
     {
         skip ("Failed to ping with error %d, is lo interface down?.\n", error);
     }
+
+    /* check reply data */
+    SetLastError(0xdeadbeef);
+    address = htonl(INADDR_LOOPBACK);
+    for (i = 0; i < ARRAY_SIZE(senddata); i++) senddata[i] = i & 0xff;
+    ret = pIcmpSendEcho(icmp, address, senddata, sizeof(senddata), NULL, replydata, replysz, 1000);
+    error = GetLastError();
+    reply = (ICMP_ECHO_REPLY *)replydata;
+    ok(ret, "IcmpSendEcho failed unexpectedly\n");
+    todo_wine ok(error == NO_ERROR, "Expect last error:0x%08x, got:0x%08x\n", NO_ERROR, error);
+    ok(INADDR_LOOPBACK == ntohl(reply->Address), "Address mismatch, expect:%s, got: %s\n", ntoa(INADDR_LOOPBACK),
+       ntoa(reply->Address));
+    ok(reply->Status == IP_SUCCESS, "Expect status:0x%08x, got:0x%08x\n", IP_SUCCESS, reply->Status);
+    ok(reply->DataSize == sizeof(senddata), "Got size:%d\n", reply->DataSize);
+    ok(!memcmp(senddata, reply->Data, min(sizeof(senddata), reply->DataSize)), "Data mismatch\n");
 }
 
 /*