Jump to content
  • Advertisement
Sign in to follow this  
PPCThug

strcat crashes program

This topic is 4848 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I am working on a program that downloads a web page source, I want to put all the source in one long string. unfortunatly it seems I have to call internetreadfile several times to get a whole web page, so I was trying to figure out how to get this to work and ended up finding strcat in the microsoft VC++ docs and thought I'd found the answer to my problems. I tried puting strcat into my program and compiled it and ran it. and nothing. the mouse changes to indicate that the program is starting up and then nothing, the program is not in memory (ctrl + alt + del, under task manager, no trace in either the applications or processes tabs) remarking out strcat fixes the problem. I've got a sneaking hunch that this is a stupid problem that I should know the answer to, but I did try a few things to fix it, substituting strcat for strncat, looking in the string.h file for the strcat function (couldn't find it) searching google for the same problem (found the problem but no solution) heres a code snippet

		  CHAR     szBuffer[1024];
		  CHAR     szWebPage[4000];
		  int      iTest;

			  while ( iTest = InternetReadFile( hFile, szBuffer, 1000, &dwRead )  )
				  {

					if ( dwRead == 0 )
					break;
					szBuffer[dwRead] = 0;
					strcat ( szWebPage, szBuffer}
				  }

heres the whole thing
#include <windows.h>
#include <stdio.h>
#include <tchar.h>
#include <wchar.h>
#include <string.h>
#include "stdafx.h"

LRESULT CALLBACK WndProc (HWND, UINT, WPARAM, LPARAM) ;

int WINAPI WinMain (HINSTANCE hInstance, HINSTANCE hPrevInstance,
                    PSTR szCmdLine, int iCmdShow)
{
     static TCHAR szAppName[] = TEXT ("TestProgram") ;
     HWND         hwnd ;
     MSG          msg ;
     WNDCLASS     wndclass ;
     
     wndclass.style         = CS_HREDRAW | CS_VREDRAW ;
     wndclass.lpfnWndProc   = WndProc ;
     wndclass.cbClsExtra    = 0 ;
     wndclass.cbWndExtra    = 0 ;
     wndclass.hInstance     = hInstance ;
     wndclass.hIcon         = LoadIcon (NULL, IDI_APPLICATION) ;
     wndclass.hCursor       = LoadCursor (NULL, IDC_ARROW) ;
     wndclass.hbrBackground = (HBRUSH) GetStockObject (WHITE_BRUSH) ;
     wndclass.lpszMenuName  = NULL ;
     wndclass.lpszClassName = szAppName ;
     
     if (!RegisterClass (&wndclass))
     {
          MessageBox (NULL, TEXT ("Program requires Windows NT!"), 
                      szAppName, MB_ICONERROR) ;
          return 0 ;
     }
     
     hwnd = CreateWindow (szAppName, TEXT ("Test Program"),
                          WS_OVERLAPPEDWINDOW,
                          CW_USEDEFAULT, 
                          CW_USEDEFAULT,
                          700, 
                          150,
                          NULL, NULL, hInstance, NULL) ;
     
     ShowWindow (hwnd, iCmdShow) ;
     UpdateWindow (hwnd) ;
     
     while (GetMessage (&msg, NULL, 0, 0))
     {
          TranslateMessage (&msg) ;
          DispatchMessage (&msg) ;
     }
     return msg.wParam ;
}

LRESULT CALLBACK WndProc (HWND hwnd, UINT message, WPARAM wParam, LPARAM lParam)
{
     static int  cxChar, cxCaps, cyChar ;
     static int  cxClient, cyClient ;
     HDC         hdc ;
     PAINTSTRUCT ps ;
     TEXTMETRIC  tm ;
     
     // these are for placment calculations
     static unsigned short int WSymbol, WShares, WWorth, WChange, WTotal;
     
     
     
     switch (message)
     {
     
     case WM_CREATE:
          hdc = GetDC (hwnd) ;

          GetTextMetrics (hdc, &tm) ;
          cxChar = tm.tmAveCharWidth ;
          cxCaps = (tm.tmPitchAndFamily & 1 ? 3 : 2) * cxChar / 2 ;
          cyChar = tm.tmHeight + tm.tmExternalLeading ;
          
          WSymbol = 3 * cxCaps;
          WShares = 11 * cxCaps;
          WWorth  = 19 * cxCaps;
          WChange = 29 * cxCaps;
          WTotal  = 45 * cxCaps;




	      HINTERNET hINet, hFile;

		  hINet = InternetOpen("testap", INTERNET_OPEN_TYPE_PRECONFIG, NULL, NULL, 0 );

		  CHAR     szBuffer[1024];
		  CHAR     szWebPage[4000];
		  DWORD    dwRead;
		  CHAR     szTemp[64];
		  int      iTest;



		  if ( !hINet )
			{
			  MessageBox ( hwnd, "InternetOpen Failed", "Failure", 0);
			  return 0;
			}

		  hFile = InternetOpenUrl( hINet, "http://www.google.com", NULL, 0, 0, 0 ) ;

		  if ( hFile )
			 {
			  
			  
			  while ( iTest = InternetReadFile( hFile, szBuffer, 1000, &dwRead )  )
				  {

					if ( dwRead == 0 )
					break;
					szBuffer[dwRead] = 0;
					strcat ( szWebPage, szBuffer);
			     
	
				  }
			  InternetCloseHandle( hFile );
			 }
		  InternetCloseHandle( hINet );
          
          ReleaseDC (hwnd, hdc) ;

		  wsprintf ( szTemp, " %i characters where read", dwRead);


		  MessageBox ( hwnd, szTemp, "char's read", 0);
		  if (iTest)
			  MessageBox ( hwnd, "success!", "hurra", 0);

          return 0 ;

     case WM_SIZE:
          cxClient = LOWORD (lParam) ;
          cyClient = HIWORD (lParam) ;
          return 0 ;
          
     case WM_PAINT:
          hdc = BeginPaint (hwnd, &ps) ;
          
          EndPaint (hwnd, &ps) ;
          return 0 ;
          
     case WM_DESTROY:
          PostQuitMessage (0) ;
          return 0 ;
     }
     return DefWindowProc (hwnd, message, wParam, lParam) ;
}

Share this post


Link to post
Share on other sites
Advertisement
While I can't comment on your specific problem, I can say that your function is horribly broken. You can perform at most four successful reads before your application will bitch out. Or maybe it won't, it is hard to say. Maybe your application just won't run at all, which is the behavior you're seeing.

You need to keep track of how many characters you've put into szWebPage to ensure it isn't more than 4000. That isn't much [ignoring whitespace, ~800 words in a standard document, less on a web page]. Once you clear 4000, you get a buffer overflow, and everything falls apart quite nicely.

Or look into std::string, which is considerably safer as it not only does that for you, it'll stretch itself out as long as it needs to be.

CM

Share this post


Link to post
Share on other sites
You're not initializing szWebPage. strcat assumes that your string is already null-terminated, and an uninitialized char array is anything but that. What's probably happening is that strcat is searching for the end of the string, it overshoots the bounds of the array, stomps on memory that's not yours, and then Windows kills the application because of that. Just add a simple

szWebPage[0] = '\0';

after the declaration of szWebPage.

Share this post


Link to post
Share on other sites
std:string webpage;

webpage.reserve(8192);//most pages are atleast 8 kbs large
here some pseudo code which works for text files if your want to read binary files you had do it a little bit different so you don t need the terminating \0s which might destroy the binary format

while(..internetreadfile(szbuffer, 1023...))
{
szbuffer[1023] = '\0';
webpage+=szbuffer;
}


Share this post


Link to post
Share on other sites
Here is your fix (basically what Aprosenf is saying). Replace this
		  CHAR     szWebPage[4000];
with this

CHAR szWebPage[4000] = "";

Share this post


Link to post
Share on other sites
Quote:
Original post by iMalc
Here is your fix (basically what Aprosenf is saying). Replace this CHAR szWebPage[4000];with this
CHAR szWebPage[4000] = "";


No, you can't assign a string literal to a char array like that, because the array is a char * const - you can't repoint it. You can certainly initialize the array (or at least make it appear to contain zero-length data) in the way that Aprosenf indicated, or initialize as "CHAR szWebPage[4000] = {0}", but either of these still has lots of problems.

I'll bet dollars to donuts that InternetReadFile does not null-terminate the read-in data, since it's reading into a "buffer", not a "string". This is indicated by the manual assignment to szBuffer in the original code (and reading only 1000 chars into a 1024 char buffer). Basiror has it correct, although I'd do it like:


const int KILOBYTE = 1024;
const int CHUNKSIZE = KILOBYTE;
const int MIN_EXPECTED_SIZE = 8 * KILOBYTE;

CHAR buffer[CHUNKSIZE + 1];
buffer[CHUNKSIZE] = '\0'; // make sure that any read-in data is null terminated
// We only need to do that once since by design we don't want to overwrite here
std::string pageSource; pageSource.reserve(MIN_EXPECTED_SIZE);
DWORD readErrorCode, amountRead;
while ((readErrorCode = InternetReadFile(file, buffer, CHUNKSIZE, &amountRead))
&& amountRead) {
pageSource += buffer;
}


Share this post


Link to post
Share on other sites
Quote:
Original post by Zahlman
No, you can't assign a string literal to a char array like that, because the array is a char * const - you can't repoint it. You can certainly initialize the array (or at least make it appear to contain zero-length data) in the way that Aprosenf indicated, or initialize as "CHAR szWebPage[4000] = {0}", but either of these still has lots of problems.


CHAR szWebPage[4000] = ""; is a valid char array initialization statement, equivalent to CHAR szWebPage[4000] = {0};

Share this post


Link to post
Share on other sites
Quote:
Original post by SiCrane
Quote:
Original post by Zahlman
No, you can't assign a string literal to a char array like that, because the array is a char * const - you can't repoint it. You can certainly initialize the array (or at least make it appear to contain zero-length data) in the way that Aprosenf indicated, or initialize as "CHAR szWebPage[4000] = {0}", but either of these still has lots of problems.


CHAR szWebPage[4000] = ""; is a valid char array initialization statement, equivalent to CHAR szWebPage[4000] = {0};
Exactly, and one I use regularly enough too. It's one of the rare cases where you don't have to use a strxxx function. When you say "either of these still has lots of problems" do you mean with the rest of the code? I must admit, I wasn't looking for other bugs.

You've been using C++ strings too long[smile]. Oh well, that's all good anyway.

Share this post


Link to post
Share on other sites
Does
CHAR szWebPage[4000] = "";
really null all the 4000 bytes as
CHAR szWebPage[4000] = {0};
does?
EDIT: I tried it and it seems CHAR szWebPage[4000] = ""; really nulls the whole array. I expected only the length of the string literal to be copied over...

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!