[PATCH 1/2] iphlpapi: Use a standalone buffer in IcmpSendEcho().
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(a)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 + /* 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) { + 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); +done: + HeapFree(GetProcessHeap(), 0, repbuf); TRACE("received %d replies\n",res); return res; } diff --git a/dlls/iphlpapi/tests/iphlpapi.c b/dlls/iphlpapi/tests/iphlpapi.c index d5613d7b4b..f964878d68 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"); + 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), "Expect data size:%d, got:%d\n", sizeof(senddata), reply->DataSize); + ok(!memcmp(senddata, reply->Data, min(sizeof(senddata), reply->DataSize)), "Data mismatch\n"); } /* -- 2.17.1
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(a)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?
+ /* 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.
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(a)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.
On Thu, Jun 21, 2018 at 05:51:13PM +0800, Zhiyi Zhang wrote:
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(a)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
I guess it's for errors: 8 for the icmp header, 60 for the original ip header, then 8 for the original icmp header. We probably don't need all of that, but it doesn't hurt. Huw.
participants (2)
-
Huw Davies -
Zhiyi Zhang