markgrif

Hello,

I'm an old mainframe assembler programmer and I'm trying to teach myself C++. I've bungled through writing a program to copy a file (just to get familiar with I/O. The program works fine when variable BuffSiz is 2048 or smaller. If it's bigger, I can't even start debugging. The error is:

Unhandled exception at 0x004012af in Compression.exe: 0xC0000005: Access violation reading location 0x00371000.

I am using VS2005 on XP SP2 with the latest SDK. This is a Win32 Console App.

This is my first program so please ignore (and advise of) the many shortcomings.

Also - how do I get the code to post here as it is formatted.

Also - I see a couple of light bulbs. They should be replaced with brackets containing the value of "i".

Thanks for the help Brian. The statement "tempout = tempin" should have each variable with brackets and "i" in each set of brackets. Those were "light bulbs" before

Code follows: (Formatting fixed by Brian Kramer)

#include <windows.h>
#include <tchar.h>
#include <iostream.h>
int main()
{
    WORD BuffSiz = 4096;
    DWORD MemAction = MEM_COMMIT;
    DWORD PageReadWrite = PAGE_READWRITE;
    LPOVERLAPPED lpNull = NULL;
    HANDLE OpenFileRead(LPCWSTR lpInFileName);
    HANDLE OpenFileWrite(LPCWSTR lpOutFileName);
    BOOL CloseFile(HANDLE hClose);
    TCHAR InFileName[]=TEXT("C:\\test files\\sol.exe");
    LPCTSTR lpInFileName = (LPCTSTR) &InFileName;
    HANDLE hInFile = NULL;
    LPVOID lpInBuff = NULL;
    DWORD InBuffSiz = BuffSiz;
    DWORD BytesRead = 0;
    LPDWORD lpBytesRead = &BytesRead;
    LPTSTR tempin;
    TCHAR OutFileName[]=TEXT("G:\\sol.exe");
    LPCTSTR lpOutFileName = (LPCTSTR) &OutFileName;
    HANDLE hOutFile = NULL;
    LPVOID lpOutBuff = NULL;
    DWORD OutBuffSiz = BuffSiz;
    DWORD BytesWrite = 0;
    LPDWORD lpBytesWrite = &BytesWrite;
    LPTSTR tempout;

    if ( !(hInFile = OpenFileRead(lpInFileName)) ) goto getout;
    cout << "OK for read open" << endl;

    if ( !(hOutFile = OpenFileWrite(lpOutFileName)) ) goto getout;
    cout << "OK for write open" << endl;

    if ( !(lpInBuff = VirtualAlloc(lpInBuff,InBuffSiz,MemAction,PageReadWrite)) ) goto getout;
    cout << "Allocate for input buffer OK " << endl;

    if ( !(lpOutBuff = VirtualAlloc(lpOutBuff,OutBuffSiz,MemAction,PageReadWrite)) ) goto getout;
    cout << "Allocate for output buffer OK " << endl;

    for (;;)
    {
        if ( !(ReadFile(hInFile,lpInBuff,InBuffSiz,lpBytesRead,lpNull)) )
        {
            cout << "Read failed" << endl;
            goto getout;
        }
        if (BytesRead == 0) break;

        tempin = static_cast <LPTSTR> (lpInBuff);
        tempout = static_cast <LPTSTR> (lpOutBuff);

        for (WORD i=0; i<BytesRead; i++) { tempout = tempin; }

        BytesWrite = BytesRead;

        if ( !(WriteFile(hOutFile,lpOutBuff,OutBuffSiz,lpBytesWrite,lpNull)) )
        {
            cout << "Write failed" << endl;
            goto getout;
        }

    }

getout:
    if ( CloseFile(hInFile) ) cout << "Ok for read close" << endl;
    if ( CloseFile(hOutFile) ) cout << "Ok for write close" << endl;
    MemAction = MEM_RELEASE;
    InBuffSiz = 0;
    OutBuffSiz = 0;
    if ( VirtualFree(lpInBuff,InBuffSiz,MemAction) ) cout << "Read buffer freed" << endl;
    if ( VirtualFree(lpOutBuff,OutBuffSiz,MemAction) ) cout << "Write buffer freed" << endl;
    return 0;
}

HANDLE OpenFileRead(LPCWSTR lpInFileName)
{
    HANDLE hInFile = NULL;
    hInFile = CreateFile(lpInFileName, // file to open
        GENERIC_READ, // open for reading
        FILE_SHARE_READ, // share for reading
        NULL, // default security
        OPEN_EXISTING, // existing file only
        FILE_ATTRIBUTE_NORMAL, // normal file
        NULL); // no attr. template

    return hInFile;
}

HANDLE OpenFileWrite(LPCWSTR lpOutFileName)
{
    HANDLE hOutFile = NULL;
    hOutFile = CreateFile(lpOutFileName, // file to open
        GENERIC_WRITE, // open for reading
        FILE_SHARE_WRITE, // share for reading
        NULL, // default security
        CREATE_ALWAYS, // existing file only
        FILE_ATTRIBUTE_NORMAL, // normal file
        NULL); // no attr. template
    return hOutFile;
}

BOOL CloseFile(HANDLE hClose)
{
    BOOL RetCode = FALSE;
    RetCode = CloseHandle(hClose);
    return RetCode;
}

 

 

 



Re: Visual C++ Language Read access violation when buffer bigger than 2048

Brian Kramer

The forum editor's handling of pasted code from VS is borked.  After fixing the double-line break and restoring indents in Visual Studio, I pasted into notepad before pasting in the forum editor.  There's supposedly a tag that lets you paste code in but I stopped caring to exercise the functionality (maybe someone else can demonstrate).

I took the liberty of editing your post for readability.

I see tons of issues with your coding approach, but as you requested I'll restrict my comments to what is needed to make your program execute properly. (Sorta: the use of goto getout leaves the program vulnerable to other problems.)

1. Failure of CreateFile to open a file for read or write results in the return value INVALID_HANDLE_VALUE, which is effectively -1 (negative one).  Your check assumes the zero value is the failure result.  I fixed this.

2. You read 4K worth of data into a buffer, write to another buffer (the "output buffer"), and then write the output buffer out.  LPTSTR is not a good datatype to use because it is either 1 or 2 bytes depending on whether your using _UNICODE.  Since you are measuring the sizes in bytes via ReadFile and WriteFile, I changed the type of tempin and tempout to LPBYTE.

3. Your memory copy loop was not written correctly.  You need to dereference your tempin and tempout pointers, and you also need to increment them.

4. In WriteFile, you write the size of the output buffer, not the actual number of bytes left in final pass of the loop.   I changed OutBuffSiz to BytesWrite on the call to WriteFile.

Here's the updated code to main() with changes highlighted.  Note that I'm using a later version of Visual Studio which uses the standard names for headers, so you'll see another change at the top.

#include <windows.h>
#include <tchar.h>
#include <iostream>
using namespace std;

int main()
{
    WORD BuffSiz = 4096;
    DWORD MemAction = MEM_COMMIT;
    DWORD PageReadWrite = PAGE_READWRITE;
    LPOVERLAPPED lpNull = NULL;
    HANDLE OpenFileRead(LPCWSTR lpInFileName);
    HANDLE OpenFileWrite(LPCWSTR lpOutFileName);
    BOOL CloseFile(HANDLE hClose);
    TCHAR InFileName[]=TEXT("C:\\sol.exe");
    LPCTSTR lpInFileName = (LPCTSTR) &InFileName;
    HANDLE hInFile = NULL;
    LPVOID lpInBuff = NULL;
    DWORD InBuffSiz = BuffSiz;
    DWORD BytesRead = 0;
    LPDWORD lpBytesRead = &BytesRead;
    LPBYTE tempin;
    TCHAR OutFileName[]=TEXT("C:\\sol2.exe");
    LPCTSTR lpOutFileName = (LPCTSTR) &OutFileName;
    HANDLE hOutFile = NULL;
    LPVOID lpOutBuff = NULL;
    DWORD OutBuffSiz = BuffSiz;
    DWORD BytesWrite = 0;
    LPDWORD lpBytesWrite = &BytesWrite;
    LPBYTE tempout;

    if ( INVALID_HANDLE_VALUE == (hInFile = OpenFileRead(lpInFileName)) ) goto getout;
    cout << "OK for read open" << endl;

    if ( INVALID_HANDLE_VALUE == (hOutFile = OpenFileWrite(lpOutFileName)) ) goto getout;
    cout << "OK for write open" << endl;

    if ( !(lpInBuff = VirtualAlloc(lpInBuff,InBuffSiz,MemAction,PageReadWrite)) ) goto getout;
    cout << "Allocate for input buffer OK " << endl;

    if ( !(lpOutBuff = VirtualAlloc(lpOutBuff,OutBuffSiz,MemAction,PageReadWrite)) ) goto getout;
    cout << "Allocate for output buffer OK " << endl;

    for (;;)
    {
        if ( !(ReadFile(hInFile,lpInBuff,InBuffSiz,lpBytesRead,lpNull)) )
        {
            cout << "Read failed" << endl;
            goto getout;
        }
        if (BytesRead == 0) break;

        tempin = static_cast <LPBYTE> (lpInBuff);
        tempout = static_cast <LPBYTE> (lpOutBuff);

        for (WORD i=0; i<BytesRead; i++) { *tempout++ = *tempin++; }

        BytesWrite = BytesRead;

        if ( !(WriteFile(hOutFile,lpOutBuff,BytesWrite,lpBytesWrite,lpNull)) )
        {
            cout << "Write failed" << endl;
            goto getout;
        }

    }

getout:
    if ( CloseFile(hInFile) ) cout << "Ok for read close" << endl;
    if ( CloseFile(hOutFile) ) cout << "Ok for write close" << endl;
    MemAction = MEM_RELEASE;
    InBuffSiz = 0;
    OutBuffSiz = 0;
    if ( VirtualFree(lpInBuff,InBuffSiz,MemAction) ) cout << "Read buffer freed" << endl;
    if ( VirtualFree(lpOutBuff,OutBuffSiz,MemAction) ) cout << "Write buffer freed" << endl;
    return 0;
}

 

 





Re: Visual C++ Language Read access violation when buffer bigger than 2048

Brian Kramer

Okay, so you did have the memory copy done correctly. The emoticons which was supposed to be [ i ] (san spaces) got dropped when I pasted in code.  You can either do for loop the way you had it before, or my way--they mean the same thing.

So ignore my #3.

Also, lets make new posts as we continue discussion.  The "thanks Brian" in the OP (original post) threw me for a loop, but of course I'm also guilty of the thread discontinuity when I accidently dropped your emoticon/[ i ] thing.





Re: Visual C++ Language Read access violation when buffer bigger than 2048

markgrif

Thanks again Brian - I'll certainly take your recommendation of follow up posts instead of editting.

On your number 4 -

The function requires that parameter 3 be the size of the output buffer. Parameter 4 is the number of bytes to write. Parameter 4 is "lpBytesWrite", which is a pointer to "BytesWrite", which is set to the number of bytes read in the statement just before the call to "WriteFile".

As to the us of LPTSTR, the program would'nt compile without its' use.

Also - please comment on anything you like. I'm really good at mainframe assembler, but I know little of C++.





Re: Visual C++ Language Read access violation when buffer bigger than 2048

Brian Kramer

The design of the WriteFile() API is intended to be symmetric to ReadFile().  With ReadFile(), you're specifying the number of bytes you want to read (in your case, 4K), and the API reports back the number of bytes actually read (it should be fewer only only on your last pass through your loop).  With WriteFile(), you specify the number of bytes you want to write (in your case, whatever number of bytes you previously read), and the API reports back the number of bytes actually written (and if this number is any fewer, then that's actually an error that should be reported by GetLastError()).  With both APIs, lpBytesRead and lpBytesWritten are strictly "out" parameters: you don't need to set it to anything yourself.

I would recommend against just changing datatypes just to get something to compile.  Try to obtain understanding, or you might be coding up a house of cards.  When you see "T" in LPTSTR, etc, you're seeing a reference to the TCHAR type which is either 1 or 2 bytes depending if _UNICODE is defined in your compilation.  If a Win32 API uses a TCHAR-related type, then use TCHARs.  If it API says "number of bytes" then use a BYTE-related type (or char* if you're not coding against the Win32 API).  Also pay close attention to treatment of strings with null terminators: the "length" of a C string, which consists of 1 or 2 byte characters and usually does not include the null terminator character, is not the same as the "number of bytes" of an array containing that string.

The first change I'd suggest is to get religious about not using goto.  Use a "triangle" style of coding where you do something that might be erroneous, check for the result with if, and enter a new block which takes the next step along with the necessary cleanup.

e.g. for the file opening logic, do

HANDLE hInFile = OpenFile(...);
if( hInFile != INVALID_HANDLE_VALUE )

   HANDLE hOutFile = OpenFile(...);
   if( hOutFile != INVALID_HANDLE_VALUE )
   {
      // enter your read loop here, continuing to use the triangle style coding as needed
      CloseHandle( hOutFile ); // corresponding cleanup
   }
   CloseHandle( hInFile ); // corresponding cleanup
}

Also use the same coding style on your memory allocations.  Also, you do not need to call VirtualAlloc... that's low level.  In C++, do this instead:

LPBYTE buffer = new BYTE[ BufSize ];
if( buffer != 0 )
{
   // do your stuff
   delete[] buffer;  // corresponding cleanup
}   

The second change i'd suggest in your code is to not copy to a separate buffer.  Your read buffer can instead be passed into WriteFile.  e.g. just the buffer described below.

Note in C++ you do not need to declare all your variables at the top of your function.  You're doing this way too much, and it interferes with the cleaner triangle style of coding.

But in the real world, I would just use the CopyFile API, or if I weren't using the Win32 API set, I'd use system("copy file1 file2").

 





Re: Visual C++ Language Read access violation when buffer bigger than 2048

markgrif

Thanks again -

I'll do my best to eliminate "goto" forever. It's harder for me because in assembler the "branch" instruction is the most used.

 

All of your help and comments have been extremely useful. I would like to say that this was just an educational exercise. I did know that I could use the input buffer but I just wanted to copy it to see if it worked. This thing with data types in C++ is difficult for a beginner to get a handle on (no pun intended). I tried every which way to change the types but no luck.

 

Even with your changes I still can't specify "BuffSiz" larger than 2048 without getting the exception. Would it be possible for you to run with 4096 to see if it works

I forgot to say that I used VirtualAlloc because I erroneously read in "WriteFile" or "ReadFile" that the buffers had to start on a sector boundary. That turns out not to be the case with this simple I/O type.

 

 





Re: Visual C++ Language Read access violation when buffer bigger than 2048

Brian Kramer

I don't know why the problem is occuring. My best guess is that you're mixing up your character widths (TCHAR=two byte wchar_t versus char = one byte) I would use the debugger to spot exactly where the memory exception occurs.

PS I added the variable declarations in my example. e.g. HANDLE hINFile.





Re: Visual C++ Language Read access violation when buffer bigger than 2048

markgrif

Brian - you have solved it!!!

The project was set to UNICODE. I changed all my variables to "T" instead of a couple of them that were "W" and changed the project setting to non-unicode.

All works fine.

Thank you very much!

PS: Post something so I can close this thread





Re: Visual C++ Language Read access violation when buffer bigger than 2048

Simple Samples

As for improvements, including removing gotos, one trick is to move most of the code out of the main function and into one or more other functions. That way, it is relatively easy to use the return statement instead of goto. Use of return that way is acceptable eventhough it might be used in a manner similar to goto. I have never, ever, ever used goto in a C or C++ program, except maybe when it was in sample code.

The following is a version that uses all C++. This version will not report read/write errors.

#include <iostream>
#include <fstream>
#include <string>
int main(int argc, char* argv[]) {
	(void)argc, argv;
	static const int BufferSize = 96;
	std::string InFilename("Stuff\\ShowWhere.cpp");
	std::string OutFilename("Console.txt");
	std::ifstream InFile;
	std::ofstream OutFile;
	char *InBuffer, *OutBuffer;
std::cout << "Opening output file: " << OutFilename << '\n';
OutFile.open(OutFilename.c_str(), std::ios_base::out | std::ios_base::binary);
if (!OutFile.is_open()) {
	std::cerr << "Error opening file\n";
	return 16;
	}
std::cout << "Opening input file: " << InFilename << '\n';
InFile.open(InFilename.c_str(), std::ios_base::in | std::ios_base::binary);
if (!InFile.is_open()) {
	OutFile.close();
	std::cerr << "Error opening file\n";
	return 16;
	}
InBuffer = new char[BufferSize];
OutBuffer = new char[BufferSize];
while (!InFile.eof()) {
	InFile.read(InBuffer, BufferSize);
	memcpy(OutBuffer, InBuffer, InFile.gcount());	// could use BufferSize
	OutFile.write(OutBuffer, InFile.gcount());
	}
delete [] InBuffer;
delete [] OutBuffer;
InFile.close();
OutFile.close();
return 0;
}





Re: Visual C++ Language Read access violation when buffer bigger than 2048

markgrif

Thank you Sam -

Your code looks a lot better than mine. It is an acceptable alternative and I thank you for your time and effort. Lots of people on the forum, I'm sure, will copy that code and use it!

I am happy that, due to some effort by you and Brian, I have learned so much in one day! If you guys ever want to write IBM assembler - you have a friend.





Re: Visual C++ Language Read access violation when buffer bigger than 2048

Simple Samples

markgrif wrote:
If you guys ever want to write IBM assembler - you have a friend.
I have done a little. I know what a TCB and a BAL are, although my memory is fuzzy. I know that in that environment, there is not a stack supported by the machine. I am getting off-topic, though.

That code I posted is something that many others in these forums can do better than I. I assume there are many books also with samples such as that. So I think you are getting a good tour.

The Windows functions such as CreateFile are great for working with devices and many such things, but for simple application programming the C++ standard classes are easy and effective. There are some limitations I don't like.






Re: Visual C++ Language Read access violation when buffer bigger than 2048

Brian Kramer

Here's a previous post of mine that might help clarify the various character types.



Re: Visual C++ Language Read access violation when buffer bigger than 2048

Simple Samples

Brian Kramer wrote:
Here's a previous post of mine that might help clarify the various character types.
You might have intended to post that to a different thread, but it is useful here (too).






Re: Visual C++ Language Read access violation when buffer bigger than 2048

Brian Kramer

It was intended for this thread. :)