feedback icon indicating copy to clipboard operation
feedback copied to clipboard

IcmpEcho2 documentation has multiple issues.

Open bhelyer opened this issue 3 years ago • 7 comments

The page in question: https://docs.microsoft.com/en-us/windows/win32/api/icmpapi/nf-icmpapi-icmpsendecho2

Apologies for the vague title, but I couldn't condense all my thoughts into something pithier.

I'll start with the reply buffer size documentation:

The allocated size, in bytes, of the reply buffer. The buffer should be large enough to hold at least one ICMP_ECHO_REPLY structure plus RequestSize bytes of data.

This buffer should also be large enough to also hold 8 more bytes of data (the size of an ICMP error message) plus space for an IO_STATUS_BLOCK structure.

Why is the size awkwardly split across multiple paragraphs? I think expressing the total size required in one sentence: "The buffer should be large enough to hold at least one ICMP_ECHO_REPLY, an IO_STATUS_BLOCK, the entire Request buffer, and an additional 8 bytes." would make misreadings less likely. It's complicated, but the fact that that the second half of the size is split across not just sentences but paragraphs tripped me up, and I expect others too (look for async calls to IcmpEcho2 on the internet -- everyone seems to pass different values!)

Further more, IO_STATUS_BLOCK (which appears to be a kernel struct) is not used in the example, which uses sizeof (ICMP_ECHO_REPLY) + sizeof (SendData) + 8 to size the buffer. Is the example or the documentation wrong?

Next up, the return value:

When called asynchronously, the IcmpSendEcho2 function returns ERROR_IO_PENDING to indicate the operation is in progress.

This doesn't seem to be the case here on Windows 10, or any code I've seen using IcmpEcho2 async on the internet. It apparently always returns 0, and GetLastError returns ERROR_IO_PENDING. Maybe this is different when using the ApcRoutine parameter. Maybe it's different in different versions of Windows?

Next, the remarks section.

The application must parse the data pointed to by ReplyBuffer parameter using the IcmpParseReplies function.

If I'm reading this right, when IcmpEcho2 is used async you have to call a different function and then cast the pointer as a different type (PICMP_ECHO_REPLY32). Those two pieces of information seem important enough to move up, rather than being buried in the remarks section and on another function's documentation page respectively -- maybe underneath the ReplyBuffer parameter section?


A small tangent on IcmpParseReplies:

The brief:

The IcmpParseReplies function parses the reply buffer provided and returns the number of ICMP echo request responses found.

The parameter documentation:

The buffer passed to IcmpSendEcho2. This is rewritten to hold an array of ICMP_ECHO_REPLY structures, its type is PICMP_ECHO_REPLY.

On a 64-bit platform, this buffer is rewritten to hold an array of ICMP_ECHO_REPLY32 structures, its type is PICMP_ECHO_REPLY32.

I wouldn't call rewriting a buffer 'parsing'. The brief should probably mention that it modifies the reply buffer provided, as parsing is usually a read only operation.


Back to SendEcho. The next paragraph (emphasis mine):

If the Event parameter is specified, the IcmpSendEcho2 function is called asynchronously. The event specified in the Event parameter is signaled whenever an ICMP response arrives. Use the CreateEvent function to create this event object.

The fact that the async version signals multiple times may be unexpected (it was to me). I think a reasonable assumption may be that the asynchronous version returns the same data as the synchronous version once the event is signalled. It appears this isn't the case, but such a fundamental behaviour change might be better called out explicitly rather than depending on inferring it from 'whenever' in the remarks section.

As the asynchronous version is so drastically different in semantics to the synchronous version, an example for an asynchronous use would be appreciated.


TL;DR

Apologies for the incoherent brain dump. The concrete issues:

  • The return value is documented wrong, or Windows has a bug.
  • The reply size requires sizeof(IO_STATUS_BLOCK) additional bytes, which the example ignores.

bhelyer avatar May 18 '22 05:05 bhelyer

Thank you for creating the issue! One of our team members will get back to you shortly with additional information. If this is a product issue, please close this and contact the particular product's support instead (see https://support.microsoft.com/allproducts for the list of support websites).

welcome[bot] avatar May 18 '22 05:05 welcome[bot]

FYI @drewbatgit

gewarren avatar Jun 22 '22 22:06 gewarren

@stevewhims It looks like this header is assigned to you.

drewbatgit avatar Jun 22 '22 22:06 drewbatgit

Thanks for filing this, @bhelyer. I'm investigating.

-Steve

stevewhims avatar Jun 23 '22 21:06 stevewhims

Still investigating.

stevewhims avatar Jan 03 '23 22:01 stevewhims

UPDATE: Still trying to get help from an SME with this one.

stevewhims avatar Mar 08 '23 20:03 stevewhims

UPDATE: I've corrected and simplified the topic. The changes address most of your feedback. I'll keep asking about the details that I believe we haven't yet addressed.

-Steve

stevewhims avatar Mar 25 '23 00:03 stevewhims