fclose(f_ptr2) - "Access Violation or memery can not be read"

[mrby123]

Please check the code for me. Thanks a lots.

void CBlast_vib_procDlg::Next()
{
FILE *f_ptr1;
FILE *f_ptr2;
const int MAX=15;

CBlast\_vib\_procDlg rr;

//
float north[2000];
float east[2000];
float elv[2000];
CString fname[2000];
//
char buffer[MAX];

// TODO: Add your control notification handler code here

UpdateData();

if(m\_outputFileName == "" ) 
{

MessageBox("All file names have to be typed in !");
rr.m\_outputFileName = m\_outputFileName;
rr.DoModal();
UpdateData(false);

}

// TODO: Add extra validation here
#define BUFSIZE MAX\_PATH
WIN32\_FIND\_DATA FindFileData;
LPTSTR DirSpec;

//
HANDLE hFind = INVALID_HANDLE_VALUE;
CString fileName[2000];
int i=0;
DirSpec = (LPTSTR) malloc (BUFSIZE);
DirSpec=TEXT("*.txt");
//
hFind = FindFirstFile(DirSpec, &FindFileData);
fileName[0]=FindFileData.cFileName;
//
while (FindNextFile(hFind, &FindFileData) != 0)
{
i+=1;
fileName[i]=FindFileData.cFileName;
}
int nfile=i;
FindClose(hFind);
///*
//
for (int ii=0;ii

[Luc Pattyn]

Hi, [Added] ignore this reply, it is wrong! [/added] there is a problem in fileName[i]=FindFileData.cFileName; this line does NOT copy the filename, it copies the pointer to the cFileName field in your unique FindFileData struct, hence in all iterations it will point to the buffer containing the last data written into it. If you want to hold all the different filenames, you must copy them, which you could do with strcpy() or strncpy(). BTW: your NULL test shows a MessageBox but then continues the program execution, which will result in failure of fscanf and/or fclose. The right way to handle this is to have an if-then-else with all file actions (fscanf/fclose) in one part, and the error handling (I do not really like MessageBox !) in the other part. :) -- modified at 13:39 Monday 26th November, 2007

Luc Pattyn [Forum Guidelines] [My Articles]

---

this months tips: - before you ask a question here, search CodeProject, then Google - the quality and detail of your question reflects on the effectiveness of the help you are likely to get - use PRE tags to preserve formatting when showing multi-line code snippets

---

[David Crow]

While it (probably) has nothing to do with your problem, I'd offer: 1) Don't post commented-out code. It just makes that much more for us to have to read/ignore. 2) Since you are using MFC, why not take advantage of CStdioFile, AfxMessagBox(), and CFileFind? That said, do the first 2-4 "columns" in your input file contain more than 14 characters? If so, buffer will not hold them all. If there are more than 2000 files in the folder pointed to by DirSpec, you'll have obvious trouble. Your very last for() loop is using ii and II. Is that intentional?

mrby123 wrote:

DirSpec = (LPTSTR) malloc (BUFSIZE); DirSpec=TEXT("*.txt");

The address assigned to DirSpec (from malloc()) has been changed, and a subsequent call to free() would fail. Consider:

CStringArray fileNames;
CFileFind fileFind;

BOOL bFound = fileFind.FindFile("*.txt");
while (bFound)
{
bFound = fileFind.FindNextFile();
fileNames.Add(fileFind.GetFilePath());
}

fileFind.Close();

for (int ii = 0; ii < fileNames.GetSize(); ii++)
{
CString fileName = fileNames.GetAt(ii);

CStdioFile fileIn;        
if (fileIn.Open(fileName, CFile::modeRead))
{
    CString line;
    fileIn.ReadString(line);

    // parse line here
     
    fileIn.Close();
}

}

CStdioFile fileOut;
if (fileOut.Open(m_outputFileName, CFile::modeWrite))
{
CString str;

str.Format("%d\\n", fileNames.GetSize());
fileOut.WriteString(str);
fileOut.WriteString(str);
 
fileOut.WriteString("fineName  Easting(m)  Northing(m)  elv(m)\\n");

for (int ii = 0; ii < fileNames.GetSize(); ii++)
{
    CString fileName = fileNames.GetAt(ii);

    str.Format("%s  %f  %f  %f\\n", fileName, east\[ii\], north\[ii\], elv\[ii\]);
    fileOut.WriteString(str);
}

fileOut.Close();

}

---

"Normal is getting dressed in clothes that you buy for work and driving through traffic in a car that you are still paying for, in order to get to the job you need to pay for the clothes and the car and the house you leave vacant all day so you can afford to live in it." - Ellen Goodman

"To have a respect for ourselves guides our morals; to have deference for others governs our manners." - Lau

[David Crow]

Luc Pattyn wrote:

there is a problem in fileName[i]=FindFileData.cFileName; this line does NOT copy the filename, it copies the pointer to the cFileName field in your unique FindFileData struct, hence in all iterations it will point to the buffer containing the last data written into it. If you want to hold all the different filenames, you must copy them, which you could do with strcpy() or strncpy().

Not necessary at all. The statement is correct, since CString has an assignment operator that internally does the copying. fileName[0], fileName[1], fileName[2], etc will each contain different data.

---

"Normal is getting dressed in clothes that you buy for work and driving through traffic in a car that you are still paying for, in order to get to the job you need to pay for the clothes and the car and the house you leave vacant all day so you can afford to live in it." - Ellen Goodman

"To have a respect for ourselves guides our morals; to have deference for others governs our manners." - Laurence Sterne

[Luc Pattyn]

My mistake, was looking at it as a C function. :rolleyes:

Luc Pattyn [Forum Guidelines] [My Articles]

---

this months tips: - before you ask a question here, search CodeProject, then Google - the quality and detail of your question reflects on the effectiveness of the help you are likely to get - use PRE tags to preserve formatting when showing multi-line code snippets

---

[Luc Pattyn]

DavidCrow wrote:

If so, buffer will not hold them all.

And worse, the data may spill and overwrite whatever is adjacent to buffer, causing unpredictable errors. Overwriting a pointer is likely to result in "Access violation". The present code is unsafe. :)

Luc Pattyn [Forum Guidelines] [My Articles]

---

this months tips: - before you ask a question here, search CodeProject, then Google - the quality and detail of your question reflects on the effectiveness of the help you are likely to get - use PRE tags to preserve formatting when showing multi-line code snippets

---

[mrby123]

David, You solve my problem. You are right that I have a test which is longer than 15 characters in the data file. Thanks alots. Ruilin

[David Crow]

You could have possibly found the problem sooner by using the debugger to step over each of the calls to fscanf() and watch the value of f_ptr2. I suspect it was changed by the time fclose() was called.

---

"Normal is getting dressed in clothes that you buy for work and driving through traffic in a car that you are still paying for, in order to get to the job you need to pay for the clothes and the car and the house you leave vacant all day so you can afford to live in it." - Ellen Goodman

"To have a respect for ourselves guides our morals; to have deference for others governs our manners." - Laurence Sterne

[Mark Salsbery]

DavidCrow wrote:

You could have possibly found the problem sooner by using the debugger

Surely YOU jest ;P

Mark Salsbery Microsoft MVP - Visual C++ :java:

[David Crow]

No really, I'm dead serious. Science has proven that the debugger really does add years to your life, make you look taller, help you find problems quicker.

---

"Normal is getting dressed in clothes that you buy for work and driving through traffic in a car that you are still paying for, in order to get to the job you need to pay for the clothes and the car and the house you leave vacant all day so you can afford to live in it." - Ellen Goodman

"To have a respect for ourselves guides our morals; to have deference for others governs our manners." - Laurence Sterne

[Sunil Shindekar]

I think this statement might have the problem. fscanf( f_ptr2,"%s %s %s\n",buffer,buffer,fname[ii].GetBuffer(MAX_PATH)); Here one of the pointer is taken using GetBuffer of CString. I think it is not the correct way of geting the pointer of CString memory and copying value in it like character array. Taking the value in character array and then assigning it to CString might be better option. Something like this. char sTemp[500]; fscanf( f_ptr2,"%s %s %s\n",buffer,buffer, sTemp); fname[ii] = sTemp;

[David Crow]

Sunil Shindekar wrote:

I think this statement might have the problem. fscanf( f_ptr2,"%s %s %s\n",buffer,buffer,fname[ii].GetBuffer(MAX_PATH));

While it's awkward looking, there is nothing wrong with it.

Sunil Shindekar wrote:

Taking the value in character array and then assigning it to CString might be better option.

Different? Yes. Better? No.

---

"Normal is getting dressed in clothes that you buy for work and driving through traffic in a car that you are still paying for, in order to get to the job you need to pay for the clothes and the car and the house you leave vacant all day so you can afford to live in it." - Ellen Goodman

"To have a respect for ourselves guides our morals; to have deference for others governs our manners." - Laurence Sterne

[Sunil Shindekar]

GetBuffer returns the pointer to the memory which is enough to store the current string assigned to the CString object. You are using the same pointer to read the data from the file. If the data is too large to store in the currently allocated memory for the pointer by the CString, then there will be memory overrun. It can cause overwriting the values of other memory locations which may or may not include file pointer also or FILE structure also.

[David Crow]

Sunil Shindekar wrote:

If the data is too large to store in the currently allocated memory for the pointer by the CString, then there will be memory overrun. It can cause overwriting the values of other memory locations which may or may not include file pointer also or FILE structure also.

Aand how is your suggestion of using char sTemp[500] any better?

---

"Normal is getting dressed in clothes that you buy for work and driving through traffic in a car that you are still paying for, in order to get to the job you need to pay for the clothes and the car and the house you leave vacant all day so you can afford to live in it." - Ellen Goodman

"To have a respect for ourselves guides our morals; to have deference for others governs our manners." - Laurence Sterne