failing with certain string lengths

[Bleech]

well, I've just now written the below little program. it is a Win32 app, however my question doesn't regard anything Win32 specific.

this program will produce some funky results if the string passed is too small to hold the entire IP address. it works fine if the string passed is large enough, and most of the time even if it is too small, however not all the time. my goal with the string length here is to prevent a buffer overflow, however it's not exactly doing that for every case.

when I declare the string szStr of different sizes, specifically 1, 4, 8, 11, and 12 bytes it crashes. I should also note that I get weird results with values less than 12, I seem to somehow get more characters outputted than what are supposed to be.

#include <string.h>
#include <ctype.h>
#include <windows.h>
#include <winsock.h>

char *GetIP(char *pszDest, char *pszDesc, char *pszBuf, int iLen, unsigned short usPort)
{
	SOCKET sock;
	WSADATA wsaD;
	SOCKADDR_IN sin;
	PHOSTENT pHostEnt;
	char szGET[40], szBuf[1024], *p, *ip;
	int i, j;

	WSAStartup(0x101, &wsaD);

	sock = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);

	if(!(pHostEnt = gethostbyname(pszDest)))
		goto Failure;

	sin.sin_family = AF_INET;
	sin.sin_addr = *((PIN_ADDR)pHostEnt->h_addr);
	sin.sin_port = htons(usPort);

	if(connect(sock, (PSOCKADDR)&sin, sizeof(SOCKADDR_IN)) == SOCKET_ERROR)
		goto Failure;

	wsprintf(szGET, "GET / HTTP/1.1\r\nHost: &#37;s\r\nConnection: close\r\n\r\n", pszDest);

	if(send(sock, szGET, strlen(szGET) + 1, 0) == SOCKET_ERROR)
		goto Failure;

	do {
		i = recv(sock, szBuf, sizeof szBuf - 1, 0);
		
		if(p = strstr(szBuf, pszDesc)) {
			ip = p + strlen(pszDesc);

			for(j = 0; isdigit(*ip) || *ip == '.' && j < iLen; ip++, j++)
				*(pszBuf + j) = *ip;

			*(pszBuf + j) = '\0';
		}
	} while(i > 0 && i != SOCKET_ERROR);

	closesocket(sock);
	WSACleanup();

	return pszBuf;
Failure:
	closesocket(sock);
	WSACleanup();

	return NULL;
}

int WinMainCRTStartup()
{
	char szStr[4];

	MessageBox(NULL, GetIP("whatismyip.com", "Your IP Is ", szStr, sizeof szStr - 1, 80), "hi", MB_OK);

	return 0;
}

could anyone here please tell me what is happening here? thank you in advance.

[itsme86]

sizeof szBuf

Since szBuf is a pointer, using sizeof on it will give you the size of the pointer (typically 4 bytes). If you want to find out how many bytes are allocated for szBuf, you'll need to pass that size as another argument to your function.

[Bleech]

hmm. I don't think that's the problem though, at least, I can't see how it would relate to the results that I'm recieving. I'm not having trouble sending and recieving data, I am having trouble parsing the data once it has been recieved.

for example, the above code will print:

11.222

(fake IP for privacy, 7 bytes (with NULL?), don't ask how that is being contained in szStr)

I should also note that if I try compiling this in Debug build config and run it, it will trigger an error message asking to start it in the debugger. when I step into the debugger, here are some messages I recieve:

test.exe has triggered a breakpoint

After I hit "Continue":

Run-Time Check Failure #2 - Stack around the variable 'szGET' was corrupted.

A string looking like the above (11.222) is outputted in a message box, and then:

Run-Time Check Failure #2 - Stack around the variable 'szStr' was corrupted.

I think it's probably an error in my string manipulation, specifically this block of code:

if(p = strstr(szBuf, pszDesc)) {
			ip = p + strlen(pszDesc);

			for(j = 0; isdigit(*ip) || *ip == '.' && j < iLen; ip++, j++)
				*(pszBuf + j) = *ip;

			*(pszBuf + j) = '\0';
		}

although I just can't find the problem. any help is appreciated. thanks again in advance.

[iMalc]

Hmm, spot the VB programmer!

(when else does one use gotos like that!)

[Bleech]

actually, I don't code VB, and I never have. I normally don't use goto statements, but here they saved me alot of repetition. although the goto statements are also repetitive, they're smaller/shorter than repeating the code in the Failure label.

back on topic, this problem is driving me crazy.

[Salem]

> wsprintf(szGET, "GET / HTTP/1.1\r\nHost: %s\r\nConnection: close\r\n\r\n", pszDest);
This is already 45 characters long, before you add anything with the %s substitution.
Instant buffer overflow.

> i = recv(sock, szBuf, sizeof szBuf - 1, 0);
1. Check that i is > 0 before doing anything with the buffer.
2. recv() doesn't append a \0 (unless you send one).
3. Since both send() and recv() can fragment messages, you can't rely on point 2 working for you.

Something like

i = recv(sock, szBuf, sizeof szBuf - 1, 0);
if ( i > 0 ) {
    szBuf[i] = '\0';
    // Now you can use str... functions.
}

[prog-bman]

Are you sure your szGET buffer is enough for the string you are trying to put into it?

#include <string.h>
#include <ctype.h>
#include <windows.h>
#include <winsock.h>

char *GetIP(char *pszDest, char *pszDesc, char *pszBuf, int iLen, unsigned short usPort)
{
	SOCKET sock;
	WSADATA wsaD;
	SOCKADDR_IN sin;
	PHOSTENT pHostEnt;
	char *szGET, szBuf[1024], *p, *ip;
	int i, j;
	size_t getBufSize = 0;

	WSAStartup(0x101, &wsaD);

	sock = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);

	if(!(pHostEnt = gethostbyname(pszDest)))
		goto Failure;

	sin.sin_family = AF_INET;
	sin.sin_addr = *((PIN_ADDR)pHostEnt->h_addr);
	sin.sin_port = htons(usPort);

	if(connect(sock, (PSOCKADDR)&sin, sizeof(SOCKADDR_IN)) == SOCKET_ERROR){
		goto Failure;
	}/*if*/

	
	/*Grab the right sized buffer needed*/
	getBufSize = strlen("GET / HTTP/1.1\r\nHost: &#37;s\r\nConnection: close\r\n\r\n") + strlen(pszDest) + 2;

	/*Allocate The Memory For The Buffer, NOTE: This seems to be where you corruption was, was the buffer big enough?*/
	szGET = malloc(getBufSize * sizeof(szGET));
	
	/*Make sure we allocated properly*/
	if(NULL == szGET){
		goto Failure;
	}/*if*/

	/*Zero out the buffer*/
	memset(szGET, '\0', getBufSize);

	/*Format The Message To Send To The Host*/
	wsprintf(szGET, "GET / HTTP/1.1\r\nHost: %s\r\nConnection: close\r\n\r\n", pszDest);

	/*Send the message to the host*/
	if(send(sock, szGET, strlen(szGET) + 1, 0) == SOCKET_ERROR){
		goto Failure;
	}/*if*/

	do {
		i = recv(sock, szBuf, 1023, 0);
		
		if(i < 1){
			break;
		}/*if*/

		/*Make sure to null the buffer out*/
		szBuf[i] = '\0';

		p = strstr(szBuf, pszDesc);

		if(NULL != p){
			ip = p + strlen(pszDesc);

			for(j = 0; j < iLen; ip++, j++){
				if(isdigit(*ip) || *ip == '.'){
					pszBuf[j] = *ip;
				}/*if*/
				else{
					break;
				}/*else*/
			}/*for*/

			pszBuf[j] = '\0';

			/*Get out of the loop since we did what we need to do*/
			break;
		}/*if*/

	} while(i > 0 && i != SOCKET_ERROR);

	/*Clean up the goodies*/
	closesocket(sock);
	free(szGET);
	WSACleanup();

	return pszBuf;
Failure:
	closesocket(sock);
	free(szGET);
	WSACleanup();

	return NULL;
}

int main()
{
	int szSize = 0;
	char szStr[10];

	/*Calc the size of the array. Note: This is only valid on arrays not pointer types!*/
	szSize = sizeof(szStr) / sizeof(szStr[0]) - 1;

	/*Message Box The Outputed Value*/
	MessageBox(NULL, GetIP("whatismyip.com", "Your IP Is ", szStr, szSize, 80), "hi", MB_OK);

	return 0;
}

Now I am a C++ guy, so you C guys yell at me if I blew anything, I don't think I did though.

[Bleech]

lol. I thought this message from the debugger seemed weird:

Run-Time Check Failure #2 - Stack around the variable 'szGET' was corrupted.

turns out that I was actually causing what I am trying to prevent here.

Thanks all, fixed it with the advice given here and noticed that there was still a problem that was being caused by operator precedence.

this:

for(j = 0; isdigit(*ip) || *ip == '.' && j < iLen; ip++, j++)

should be:

for(j = 0; (isdigit(*ip) || *ip == '.') && j < iLen; ip++, j++)

in order to be evaluated how I want it to be.

all fixed now, printing proper results. thanks again.