[Bug 43252] New: IcmpSendEcho doesn't work with a ReplySize<56

https://bugs.winehq.org/show_bug.cgi?id=43252

Bug ID: 43252
           Summary: IcmpSendEcho doesn't work with a ReplySize<56
           Product: Wine
           Version: unspecified
          Hardware: x86-64
                OS: Linux
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: iphlpapi
          Assignee: [e-mail]
          Reporter: [e-mail]
      Distribution: ---

Created attachment 58564
  --> https://bugs.winehq.org/attachment.cgi?id=58564
IcmpSendEcho trace

IcmpSendEcho of icmp.c doesn't work with a ReplySize<56. If ReplySize<56 the
function returns with IP_REQ_TIMED_OUT. In windows the function returns
properly. This has been tested with ubuntu 16 and CentOs 7 (both x86-64) with
wine 1.8.5(32 and 64bit prefix) and wine staging 2.1(32bit prefix).

[Bug 43252] IcmpSendEcho doesn't work with a ReplySize<56

https://bugs.winehq.org/show_bug.cgi?id=43252

Mark Mankins  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #1 from Mark Mankins  ---
This bug affects IDA Pro (Floating License) v 7.0.

When IDA Pro is run for the first time, it sends an icmp echo request to the
floating license server.  The icmp payload is set to a length of 17 bytes.

IcmpSendEcho() incorrectly calculates the expected size of the icmp echo reply.

Here's the pertinent code:

ip_header=(struct ip *) ((char *) ReplyBuffer+sizeof(ICMP_ECHO_REPLY));   
    endbuf=(char *) ReplyBuffer+ReplySize;                                    
    maxlen=ReplySize-sizeof(ICMP_ECHO_REPLY);

[snip]

res=recvfrom(icp->sid, (char*)ip_header, maxlen, 0, (struct
sockaddr*)&addr,&addrlen);
    TRACE("received %d bytes from %s\n",res, inet_ntoa(addr.sin_addr));

ip_header is set to be a pointer ICMP_ECHO_REPLY characters past the start of
ReplyBuffer.  This appears to be incorrect and a contributor to the issue.

The calculation of endbuf seems to be correct.  This the end of the buffer that
receives the echo reply packet.

In my initial example, maxlen will be set to 25 bytes.  The actual size of the
icmp echo reply packet is 45 bytes.  20 bytes for the ip header, 8 bytes for
the icmp header, and 17 bytes for the icmp data.  maxlen should be 45 bytes to
fully read the entire reply packet.

Since only 25 bytes is read, the entire icmp header will not be read, and wine
will incorrectly drop the icmp echo reply packet.

I believe this diff corrects the issue:

diff --git a/dlls/iphlpapi/icmp.c b/dlls/iphlpapi/icmp.c
index ebc2f2b..8bfdf25 100644
--- a/dlls/iphlpapi/icmp.c
+++ b/dlls/iphlpapi/icmp.c
@@ -367,9 +367,9 @@ DWORD WINAPI IcmpSendEcho(
     fdr.events = POLLIN;
     addrlen=sizeof(addr);
     ier=ReplyBuffer;
-    ip_header=(struct ip *) ((char *) ReplyBuffer+sizeof(ICMP_ECHO_REPLY));
+    ip_header=(struct ip *) ((char *) ReplyBuffer);
     endbuf=(char *) ReplyBuffer+ReplySize;
-    maxlen=ReplySize-sizeof(ICMP_ECHO_REPLY);
+    maxlen=ReplySize;

/* Send the packet */
     TRACE("Sending %d bytes (RequestSize=%d) to %s\n", reqsize, RequestSize,
inet_ntoa(addr.sin_addr));

ip_header should be set to be the start of the ReplyBuffer.  This is the start
of the buffer recvfrom should write to.  It should be large enough to hold the
ip header, the icmp header, and the icmp payload.

With this diff applied, IDA Pro runs as expected.

I will attempt to create a simple console application that replicates this bug.

[Bug 43252] IcmpSendEcho doesn't work with a ReplySize<56

https://bugs.winehq.org/show_bug.cgi?id=43252

--- Comment #2 from Mark Mankins  ---
Upon further review, my analysis is not quite correct...

stand by for a correction.

[Bug 43252] IcmpSendEcho doesn't work with a ReplySize<56

https://bugs.winehq.org/show_bug.cgi?id=43252

--- Comment #3 from Mark Mankins  ---
Ok...I'm happy to report that I think my analysis is basically correct.  My
test program proves that for small values of RequestSize passed to
IcmpSendEcho, wine incorrectly ignores the icmp echo response.  For large
values passed to IcmpSendEcho, wine will handle the icmp echo response, but it
will not pass back the complete payload to the caller.    If the caller
attempts to validate that the echo response matches the echo request (for any
value of RequestSize), the validation will fail.  I will attach my test program
to this issue.

Here is what I am seeing.

Unpatched (unmodified source tree):

osboxes@osboxes:~/wine64-build$ ./wine /mnt/hgfs/Shared/EchoTest/EchoTest.exe
55
Pinging localhost with 55 bytes of data...
Address: 127.0.0.1
DataSize: 35
Echo data mismatch, byte 35
RoundTripTime: 0
Status: 0
osboxes@osboxes:~/wine64-build$ ./wine /mnt/hgfs/Shared/EchoTest/EchoTest.exe
20
Pinging localhost with 20 bytes of data...
Address: 127.0.0.1
DataSize: 0
Echo data mismatch, byte 0
RoundTripTime: 0
Status: 0
osboxes@osboxes:~/wine64-build$ ./wine /mnt/hgfs/Shared/EchoTest/EchoTest.exe
17
Pinging localhost with 17 bytes of data...
Unexpected result: 0

Patched with my patch in an earlier comment:

osboxes@osboxes:~/wine64-build$ ./wine /mnt/hgfs/Shared/EchoTest/EchoTest.exe
55
Pinging localhost with 55 bytes of data...
Address: 127.0.0.1
DataSize: 55
RoundTripTime: 0
Status: 0
osboxes@osboxes:~/wine64-build$ ./wine /mnt/hgfs/Shared/EchoTest/EchoTest.exe
20
Pinging localhost with 20 bytes of data...
Address: 127.0.0.1
DataSize: 20
RoundTripTime: 0
Status: 0
osboxes@osboxes:~/wine64-build$ ./wine /mnt/hgfs/Shared/EchoTest/EchoTest.exe
17
Pinging localhost with 17 bytes of data...
Address: 127.0.0.1
DataSize: 17
RoundTripTime: 0
Status: 0

[Bug 43252] IcmpSendEcho doesn't work with a ReplySize<56

https://bugs.winehq.org/show_bug.cgi?id=43252

--- Comment #4 from Mark Mankins  ---
Created attachment 60273
  --> https://bugs.winehq.org/attachment.cgi?id=60273
Test driver for IcmpSendEcho

This program sends an icmp echo request to localhost and outputs the data
returned to the caller.

This program expects a single argument, the number of bytes to send in the icmp
echo request payload.

For the current version of wine (3.0rc6), for small values of the payload, wine
fails to recognize the icmp echo response.  For larger values of the payload,
wine recognizes the response but doesn't return the complete payload to the
caller.

[Bug 43252] IcmpSendEcho doesn't work with a ReplySize<56

https://bugs.winehq.org/show_bug.cgi?id=43252

Zhiyi Zhang  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #5 from Zhiyi Zhang  ---
Great, if that's the issue, can you write a test for it and submit the patch?
Thanks.

[Bug 43252] IcmpSendEcho doesn't work with a ReplySize<56

https://bugs.winehq.org/show_bug.cgi?id=43252

--- Comment #6 from Mark Mankins  ---
After looking at the requirements for patch submission, it appears I'm not
eligible _at this time_ to submit patches to the project.

However, all my previous comments on this thread were made when I would have
been eligible to submit patches to the project.  Someone else could pick up
what I've documented in this issue and move this to a state where it can be
integrated.

Sorry for the inconvenience.

[Bug 43252] IcmpSendEcho doesn't work with a ReplySize<56

https://bugs.winehq.org/show_bug.cgi?id=43252

--- Comment #7 from Zhiyi Zhang  ---
Mark, thanks for your analysis. 
https://source.winehq.org/patches/data/147451
https://source.winehq.org/patches/data/147452

[Bug 43252] IcmpSendEcho doesn't work with a ReplySize<56

https://bugs.winehq.org/show_bug.cgi?id=43252

--- Comment #8 from Gijs Vermeulen  ---
Was this fixed by: 417e996e97ebc3ff47d986e322dbaf3b3a83b91d ?

Please retest with wine-3.14.

[Bug 43252] IcmpSendEcho doesn't work with a ReplySize<56

https://bugs.winehq.org/show_bug.cgi?id=43252

Gijs Vermeulen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
      Fixed by SHA1|                            |417e996e97ebc3ff47d986e322d
                   |                            |baf3b3a83b91d
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #9 from Gijs Vermeulen  ---
I compiled the test driver from Comment 4 and the results were identical to the
correct results mentioned in Comment 3.

I'm going to do ahead and mark this FIXED, if you are still experiencing
issues, feel free to reopen.

[Bug 43252] IcmpSendEcho doesn't work with a ReplySize<56

https://bugs.winehq.org/show_bug.cgi?id=43252

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #10 from Alexandre Julliard  ---
Closing bugs fixed in 4.1.