c++ winsock2 bad pointers breakpoint triggered

Question

I have a server and a client. I am using winsock2. The client sends 4 bytes:

char *ack = new char[4];
sprintf( ack, "%d", counter );
sendto( clientSocket, ack, 4, 0, ( struct sockaddr* )&remote, sizeof( remote ) );

and the server receives these 4 bytes:

char* acks = new char[4];
if( ( bytes = recvfrom( serverSocket, acks, 4, 0, ( struct sockaddr* )&remote, &remote_size ) ) == SOCKET_ERROR ) {
    cout << "socket error = " << WSAGetLastError() << endl;
    break;
}
if( bytes > 0 ) {
    sscanf( acks, "%d", &i );
}

I am getting this error and I can't figure out how to fix it:

>Critical error detected c0000374
>
>server.exe has triggered a breakpoint.

I know there is a problem with the pointer and the memory allocation. But my c++ skills are basic.

Answer 1

String formatting overflow

The most pressing issue is that you are using sprintf and sscanf. Avoid using sprintf and sscanf - they make it far too easy to accidentally create the type of bug you're seeing here, which is buffer overflow (on both your client and your server).

Consider what happens on your client when your 'counter' value is 1729. Your code will run

sprintf(ack, "%d", 1729);

The C-style-string representation of 1729 is five bytes long - one byte each for the char values '1', '7', '2', '9', and '\0'. But your ack buffer is only 4 bytes long! Now you've written that last zero byte into some chunk of memory you never allocated. In C/C++, this is undefined behavior, which means your program might crash, or it might not, and if it doesn't crash, it might end up subtly wrong later, or it might work perfectly well, or it might work most of the time except it breaks on Tuesdays.

It's not a good place to be.

You might be wondering, "if this is so awful, why didn't the sprintf just return an error or something I called it with a buffer that was too small?" The answer¹ is that sprintf can't make that check because it doesn't give you any way to tell it how big ack actually is. When your code here is calling sprintf, you know that ack is 4 bytes long (since you just created it), but all sprintf sees is a pointer to some memory, somewhere - you haven't told it a length, so it just has to blindly hope the chunk of memory you give it is big enough.

Blindly hoping is a pretty bad way to write software.

There's a few alternatives you could consider here.

1. If you are actually just trying to send an int across the wire, there's not really any need to stringify the int at all - just send it in its native format by passing reinterpret_cast<char*>(&counter) as your buffer to sendto² with sizeof(counter) as the corresponding buffer length. Use a similar construction in recvfrom on the other end. Note that this will break if your sender and your receiver have different underlying representations of ints (for example, if they use different endiannesses), but since you're talking about Winsock here I'm assuming you're assuming both ends are reasonably recent versions of Windows where that won't be a problem.
2. If you really do need to stringify the content first, use size-cognizant string conversion functions, like boost::format (which is implicitly size-cognizant because it deals in std::string instead of raw char* buffers) or _snprintf_s/_snscanf_s (which explicitly take in buffer length parameters, but are Microsoft-specific).

Recvfrom access violation

The overflow in sscanf/sprintf doesn't necessarily explain this, however:

I just want to add that the error occurs in the sscanf line. If I comment that line the error occurrs in the recvfrom line.

One possible explanation for this could be not providing adequate space for the remote address, though so long as your remote_size is a correct reflection of your remote, I'd expect this to cause recvfrom to return an error³, not crash. Another possibility is passing bad memory/handles (for example, if you've set up the new operator to not throw on failure, or if your socket initialization failed and you didn't bail out). It's impossible to say exactly without seeing the code initializing all the variables in play, and ideally the actual error you get in that scenario.

---

¹ Even though sprintf can't catch this, static analysis tools (like those included in Visual Studio 2012/2013) are very capable of catching this particular bug. If you run the posted code through the default Visual Studio 2012 Code Analyzer, it will complain with:

error C4996: 'sprintf': This function or variable may be unsafe

² Some people prefer static_cast<char*>(static_cast<void*>(&counter)) to reinterpret_cast<char*>(&counter). Both work, it's essentially a coding convention choice.

³ For example, if you were initializing remote as a SOCKADDR_IN instead of a SOCKADDR_STORAGE, you might encounter such an error if you happened to receive from an IPv6 address. This answer goes through some of the relevant gory details.