Sign in to follow this  
PPCThug

strcat crashes program

Recommended Posts

PPCThug    130
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
Conner McCloud    1135
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
Aprosenf    372
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
Basiror    241
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
iMalc    2466
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
Zahlman    1682
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
SiCrane    11839
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
iMalc    2466
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
izhbq412    204
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
PPCThug    130
thanks for all the help. CHAR szWebPage[4000] = ""; gave an error, somthing about initialazation of szWebPage skipped by 'case' label(same error I get when I initalize and assigne a value to a variable in the same line in winproc on this program for some odd reason), so after its defined I just used szWebPage[0] = 0; and that fixed the problem. I've got plenty of work left to do on this program, including making the string stuff more versitile. the szWebPage was originally 400,000 in size but messagebox couldn't print a string that long (can't imagine why *sarcasm*), since my program was testing on www.google.com and since google html page source when saved to a text file is 2.5 kb I figured that 4k would be enough. I'll try to keep the advice here in mind in the future. thanks again.

Share this post


Link to post
Share on other sites
raydog    102
Just clear out both strings so you don't have to worry about putting in NULLs before or after:
memset( szBuffer, 0, 1024 );
memset( szWebPage, 0, 4000 );

Share this post


Link to post
Share on other sites
izhbq412    204
Quote:
CHAR szWebPage[4000] = ""; gave an error, somthing about initialazation of szWebPage skipped by 'case' label

The obvious solution is to create a local scope under the case label
switch( whatever )
{
case whatever:
{
// do all your work
}
break;

case somethingElse:
...
}

Share this post


Link to post
Share on other sites
iMalc    2466
Quote:
Original post by izhbq412
Quote:
CHAR szWebPage[4000] = ""; gave an error, somthing about initialazation of szWebPage skipped by 'case' label

The obvious solution is to create a local scope under the case label
switch( whatever )
{
case whatever:
{
// do all your work
}
break;

case somethingElse:
...
}
Yes, that is the typical approach. It's also usually a warning not an error, so perhaps you have warnings-as-errors set to true?

Share this post


Link to post
Share on other sites
Zahlman    1682
Quote:
Original post by iMalc
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.


You learn something every day... that's an odd sort of special case :/ I hate C++ syntax. :(

Quote:
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.


I mean that using a char array has limitations, even if you wouldn't consider them bugs, and it forces stuff (like the strxxx functions) into the rest of the code that are likely to cause more problems. :)

Quote:
the szWebPage was originally 400,000 in size but messagebox couldn't print a string that long (can't imagine why *sarcasm*), since my program was testing on www.google.com and since google html page source when saved to a text file is 2.5 kb I figured that 4k would be enough. I'll try to keep the advice here in mind in the future. thanks again.


Well, you know, one of these days you *will* have to deal with a 400Kb webpage (or bigger; I know of some documents on the web that are several megs in what was a text file that got a minimum of HTML markup added). And you won't want to have to allocate a buffer big enough to make sure of handling those cases, only to have 99% of the space going unused most of the time. Using std::string will take care of this for you; it automatically resizes according to need (it will keep extra space, in general, for efficiency reasons - to avoid resizing too frequently - but again in general you can expect it to take no more than 1.5-2x as much space as is needed or so, depending on your implementation).

It also can make repeated appending faster, because it will automatically keep track of string lengths and therefore "append points", whereas hand-written code to do this stuff in C is often slowed down by unnecessary repeated strlen() calls (either explicit ones or implicitly via other strxxx functions). It's also a heckuva lot easier to get right which is the important thing. Oh, and it reads much more nicely too. :)

Share this post


Link to post
Share on other sites
SiCrane    11839
Quote:
Original post by Zahlman
Quote:
Original post by iMalc
Quote:
Original post by SiCrane
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.


You learn something every day... that's an odd sort of special case :/ I hate C++ syntax. :(


It's not so odd if you keep in mind that "foo" is syntactic sugar for { 'f', 'o', 'o', 0 }

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this