2

I am working on a ping tool and I am consistently getting an access violation around my sending buffer while calculating the ICMP checksum when using a packet size of 45041 or greater (including ICMP header). Any packet with size 45040 or below throws no error and properly transmits with correct checksum. Abbreviated code is below; the access violation occurs when dereferencing the buffer within the while loop in the checksum function on the first iteration.

typedef struct ICMPHeader 
{
  BYTE type;          // ICMP packet type
  BYTE code;          // Type sub code
  USHORT checksum;
  USHORT id;
  USHORT seq;
} ICMPHeader;

typedef struct echoRequest
{
  ICMPHeader icmpHead;
  char *data;
} EchoRequest;

// ...
EchoRequest *sendBuffer = new EchoRequest();
sendBuffer->data = new char[packetSize];

memset((void *)sendBuffer->data, 0xfa, packetSize);

sendBuffer->icmpHead.checksum = ipChecksum((USHORT *)sendBuffer, 
                                     packetSize + sizeof(sendBuffer->icmpHead));
// ...

// checksum function
USHORT ipChecksum(USHORT *buffer, unsigned long size)
{
  unsigned long cksum = 0;

  while (size > 1) 
  {
    cksum += *buffer++;
    size -= sizeof(USHORT);
  }

  if (size)
    cksum += *(UCHAR *)buffer;

  cksum = (cksum >> 16) + (cksum & 0xffff);
  cksum += (cksum >> 16);

  return (USHORT)(~cksum);
}

Any ideas as to why this is happening?

Exact error wording: Unhandled exception at 0x009C2582 in PingProject.exe: 0xC0000005: Access violation reading location 0x004D5000.

Using Visual Studio Professional 2012 with platform toolset v100 for .NET 4.0

mibogo
  • 173
  • 1
  • 8

2 Answers2

1

Your ipChecksum function expects a pointer to the data it's supposed to checksum, not a pointer to a structure that contains a pointer to the data to checksum. So first it checksums icmpHead, which is good. But then it checksums the pointer to data, which makes no sense. And then it checksums off the end of the EchoRequest structure.

David Schwartz
  • 179,497
  • 17
  • 214
  • 278
  • I changed `EchoRequest` to just be the `icmpHeader` and allocate enough space beyond it for the data portion and it now works for any size packet. Thanks! (though I still don't understand why it still worked for smaller packets than that threshold). – mibogo Aug 15 '13 at 17:12
  • @MikeBogochow It worked because memory protection applies to whole pages. As long you didn't run off the end of the allocated page, it won't crash, though it won't generate the right checksum either. – David Schwartz Aug 15 '13 at 18:12
  • But the checksums were correct according to wireshark and I was getting replies from the host I was pinging which I wasn't when the checksums were not correct – mibogo Aug 15 '13 at 19:31
  • @MikeBogochow Then you have a real mystery on your hands. – David Schwartz Aug 15 '13 at 19:45
0

If you want this code to be interpreted as c++ by the readers you need to fix some things.

  • memset really?

  • use reinterpret_cast to convert one pointer type to another.

  • it's generally considered a much better practice to use size_t instead of unsigned long

  • use smart pointers instead.

  • use static_cast to convert ulong to ushort.

  • USHORT is not guaranteed to be 16 bits. Use a different type instead.

Edit: You're waaaay above MTU. Keep your packets under 1k bytes. IEEE 802.3 expects 1492 though this value might vary.

gifnoc-gkp
  • 1,506
  • 1
  • 8
  • 17
  • The project started off using pure C but I changed C++ when I ran into some issues and I guess this section was still mostly C so I apologize for that. Thanks for the tips! – mibogo Aug 15 '13 at 17:15
  • For the MTU, my application doesn't care if the packet gets fragmented and it also may be sent over other things besides ethernet so I just make sure it is within the defined bounds for ICMP – mibogo Aug 15 '13 at 17:39