Thread: directory searching causes a windows error

This is basically a continuation of my old thread about file searching, but since I was completely and utterly wrong on the thought behind it I made a new thread. Old thread is here.

I looked over Elysia's example of looking through the files and every time I get a better understanding of it. I basically copied the example in my own terms in to my program, and now it comes up with a windows error (the "Send Error Report", "Don't Send" dialog box). I've tried eliminating things one at a time to see what the problem is but nothing points in a new direction. Where is my code coming up with this error?

Here is Elysia's...

Code:

bool FindFilesInternal(pp<FindFilesInternalStruct> pArgs, pp< vector<CString> > s_vFiles, UINT64& rCurrentCount, UINT64& rTotalCount)
{
	WIN32_FIND_DATA fDataDirs, fDataFiles;
	HANDLE hFileDirs = NULL, hFileFiles = NULL;

	if (pArgs->strFolder.Right(1) == _T("\\")) pArgs->strFolder.Delete(pArgs->strFolder.GetLength());

	hFileDirs = FindFirstFile(pArgs->strFolder + _T("\\*.*"), &fDataDirs);
	if (hFileDirs == (HANDLE)-1) return false; // Apparently we couldn't search this directory...
	hFileFiles = FindFirstFile(pArgs->strFolder + _T("\\") + pArgs->strPattern, &fDataFiles);
	
	if (hFileFiles != (HANDLE)-1)
	{
		while (FindNextFile(hFileFiles, &fDataFiles) && GetLastError() == ERROR_NO_MORE_FILES) // Search for files in local directory
		{
			if (pArgs->pCancel && *pArgs->pCancel) return true;
			if ( !(fDataFiles.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) )
			{
				rCurrentCount++;
				rTotalCount++;
				if (*pArgs->p_bRequest) // Due to thread safety, data has to be requested. Local copies of everything is kept. This avoids expensive locking.
				{
					if (pArgs->p_nCurrentFile) *pArgs->p_nCurrentFile = rCurrentCount;
					if (pArgs->p_strCurrentFile) *pArgs->p_strCurrentFile = pArgs->strFolder + _T("\\") + fDataFiles.cFileName;
					if (pArgs->pTotalFiles) *pArgs->pTotalFiles = rTotalCount;
					*pArgs->p_bRequest = false;
					SetEvent(pArgs->hRequestDataEvent);
				}
				if (!pArgs->bCount)
					s_vFiles->push_back(pArgs->strFolder + _T("\\") + (CString)fDataFiles.cFileName);
			}
		}
	}

	if (pArgs->bSearchSubFolders)
	{
		while (FindNextFile(hFileDirs, &fDataDirs) && GetLastError() == ERROR_NO_MORE_FILES) // Search for directories
		{
			if (pArgs->pCancel && *pArgs->pCancel) return true;
			if ((fDataDirs.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) && _tcscmp(fDataDirs.cFileName, _T(".")) != 0 && _tcscmp(fDataDirs.cFileName, _T("..")) != 0)
			{
				ppnew<FindFilesInternalStruct> pArgs2;
				*pArgs2 = *pArgs;
				pArgs2->strFolder = pArgs->strFolder + _T("\\") + fDataDirs.cFileName;
				FindFilesInternal(pArgs2, s_vFiles, rCurrentCount, rTotalCount);
			}
		}
	}

	return true;
}

Here is mine... It might look choppy cause I am still learning.

Code:

bool Search(SEARCHFILES pArgs, int &fCount, int &dCount, int &total){
    
    LPSTR lBuffer; //storage for _getdcwd
    LPSTR buffer; //displayed storage for _getdcwd
    LPSTR pbuffer; //storage for parent
    LPSTR fBuffer; //storage for file name to be checked
    
    WIN32_FIND_DATA Files,Dirs; //structure for handling all the files
    HANDLE hFiles,hDirs; //handles for FindNextFile()
    
    strcat(fBuffer,pArgs.StartFolder); //set up new folder to search in
    strcat(fBuffer,"\\*.*"); //find all files in new folder
    
    hFiles = FindFirstFile(fBuffer,&Files); //find the first, associated with files
    hDirs = FindFirstFile(fBuffer,&Dirs); //find the first, associated with directories
    
    if(hFiles == INVALID_HANDLE_VALUE) return false; //if the file handle is invalid, return false
    else if(hFiles != INVALID_HANDLE_VALUE)
    {
        while(FindNextFile(hFiles,&Files) && GetLastError() != ERROR_NO_MORE_FILES)
        {
            if(Files.dwFileAttributes != FILE_ATTRIBUTE_DIRECTORY)
            {
                fCount++; //increase file count
                //SendMessage(hStatus,SB_SETTEXT,1,(LPARAM)"files searched: %d fCount"); //this might be wrong
                //but it's not the cause of the problem (editted out)
                
                fBuffer = _getcwd(lBuffer,MAX_PATH); //get current directory
                strcat(fBuffer,Files.cFileName); //get full path for file (NOT USED YET)
                SetDlgItemText(Hwnd,TEST,Files.cFileName); //show the current file
            }
        }
    }
    else{
        SetDlgItemText(Hwnd,TEST,"!= INVALID_HANDLE_VALUE");
        return false;
    }
    
    if(hDirs == INVALID_HANDLE_VALUE) return false;
    else if(hDirs != INVALID_HANDLE_VALUE)
    {
        while(FindNextFile(hDirs,&Dirs) && GetLastError() != ERROR_NO_MORE_FILES)
        {
            if((Dirs.dwFileAttributes == FILE_ATTRIBUTE_DIRECTORY)){
                if(!(strcmp(Dirs.cFileName,"..") == 0 || (strcmp(Dirs.cFileName,".") == 0)))
                {
                    SEARCHFILES pArgs2; //create new structure
                    pArgs2 = pArgs; //assign new structure the old structure
                    pbuffer = _getcwd(lBuffer,MAX_PATH); //get the current directory
                    strcat(pbuffer,Dirs.cFileName); //add the new directory to it
                    pArgs2.StartFolder = pbuffer; //assign new directory that is to be search
                    
                    SetDlgItemText(Hwnd,CUR_PATH,pbuffer); //show the new directory
                    
                    Search(pArgs2,fCount,dCount,Total); //recursively call the function again with a new directory
                }
            }
        }
    }
    else{
        SetDlgItemText(Hwnd,TEST,"!= INVALID_HANDLE_VALUE");
        return false;
    }
    
    return true;
}

Originally Posted by Yarin

The problem is that your program is making an invalid memory read/write request.

On a glance I did notice that your strcat()ing a string the doesn't even look like it has been allocated yet.

I fixed the strcat() issue, but where am I making an invalid memory read, and how can I change that? I did not have any read problems before when I was not using recursion.

Code:

    LPSTR lBuffer; //storage for _getdcwd
    LPSTR buffer; //displayed storage for _getdcwd
    LPSTR pbuffer; //storage for parent
    LPSTR fBuffer; //storage for file name to be checked

Windows is evil in using these stupid identifiers. What they are, are pointers. LPSTR is a pointer to char, or in other words, char*.
So you just have a pointer and not an actual buffer, thus you get an access violation when writing to them.

You also seem to be mistaken that you need two loops: one for files and one for directories. This is not necessarily true.
If you specify "*.*" as the mask, then the function will return all files including directories. However, if you specify anything other than "*.*", it will only return the matching files (no directories). This was the reason I used two loops, one to gather files and one to enumerate directories.

Code:

//SendMessage(hStatus,SB_SETTEXT,1,(LPARAM)"files searched: %d fCount"); //this might be wrong

This would obviously be wrong since you don't want to see "files searched: %d fCount" in your dialog. You need to use sprintf or similar functions to create your string buffer first and then send it via SendMessage.

And as a last note,

Code:

SEARCHFILES pArgs

You don't really need to it that way. You could just pass in the directory to search or such. You don't need to copy my code 100%. The initial reason I used a struct is because I allowed the search to be done in a different thread if desired. And a thread can only take one argument, so a struct it was.

> LPSTR lBuffer; //storage for _getdcwd

Consider the difference between
char *temp = NULL; strcpy( temp, "hello world" );
and
char temp[100]; strcpy( temp, "hello world" );

As far as strcpy is concerned, the first parameter has the same TYPE in both cases, but the result is very different. It isn't enough to simply declare variables of the type expected by the various functions, you have to think about what the function is going to do with that variable when it gets it. For pointers especially, this means you have to think about where the memory is which is going to get written to, or read from.

To expand:
Virtual memory, the memory a process uses to read and write to, is divided into blocks the size of a byte. To be able to use all the memory, each block needs an address. A virtual address. Now, when you pass something to a function, usually it's by value - a new variable is created and the original value is copied to the new variable.
But if you want to change the original variable you pass into a function, then you must tell that function the address of the original variable, otherwise it can't find it. And to get the address of a variable, you must use the address of operator (&). If you do not use this, then you are lying to the function and supplying an invalid address, thus causing the whole thing to fall apart.
Note that arrays are an exception to the address rule. Instead of copying the entire array by value, the compiler instead takes the address where the array begins by default, thus creating a pointer without the need for &.

Alright I'm confused. When I had it the first way (the one in the old thread) I had the LPSTR buffers declared as globals, and it worked fine. I never got any errors, and did not run in to the issue you guys are saying. I think I understand what you guys are saying though. I am declaring a variable, then trying to give it an assignment when really it corresponds to nothing in the first place, right? How should I go about changing this then so it is essentially non-problematic. I don't want to change it, then run in to another different problem that I might think is the same problem.

Elysia-
I know I don't need two loops. I could have merged them under the same loop to shorten it up, but if it works I don't see a need to change it. And the thing about the struct as an argument in the function, I realized what you did there and noticed I could maybe use that to give the user extra options of searching later on without having to change the arguments of the function, and just add them to the struct. It was a clever idea to make it short

Originally Posted by scwizzo

Alright I'm confused. When I had it the first way (the one in the old thread) I had the LPSTR buffers declared as globals, and it worked fine. I never got any errors, and did not run in to the issue you guys are saying. I think I understand what you guys are saying though. I am declaring a variable, then trying to give it an assignment when really it corresponds to nothing in the first place, right? How should I go about changing this then so it is essentially non-problematic. I don't want to change it, then run in to another different problem that I might think is the same problem.

Well, if you re-read my above reply, you would see that you have a pointer pointing to somewhere in memory you don't have write access and therefore you get a crash.

Elysia-
I know I don't need two loops. I could have merged them under the same loop to shorten it up, but if it works I don't see a need to change it. And the thing about the struct as an argument in the function, I realized what you did there and noticed I could maybe use that to give the user extra options of searching later on without having to change the arguments of the function, and just add them to the struct. It was a clever idea to make it short

You do realize that most people prefer to just pass arguments instead of creating a struct and passing it, right?
The example is an "internal" function. It's not exported publically. There's a public interface that doesn't take a struct that calls this internal function.

Now I'm really lost. If it worked before, and doesn't now, how did I change the write access in going from global to local? Better yet, how can I get write access. I've been googling this and cannot get an answer.

Use malloc for C or new for C++, or just make an array on the stack. Don't use LP* types. Windows uses them to indicate a pointer is expected. LPSTR means "LP" - pointer and "STR" - string, or in other words char.
So something like char[100] or something will work.

I used the new operator, and also tried making char variables with pointers to them separately. Both ways got rid of the error. I just need to work out some other kinks now. Thanks guys!

EDIT: New problem... the hFiles is returning INVALID_HANDLE_VALUE every time. Does the directory need to be changed in order for FindFirstFile to work?

Code:

    LPSTR lBuffer = new char[256]; //storage for _getcwd
    LPSTR pbuffer = new char[256]; //storage for parent
    LPSTR fBuffer = new char[256]; //storage for file name to be checked
    
    WIN32_FIND_DATA Files,Dirs; //structure for handling all the files
    HANDLE hFiles = NULL, hDirs = NULL; //handles for FindNextFile()
    
    strcat(fBuffer,pArgs.StartFolder); //set up new folder to search in
    strcat(fBuffer,"\\*.*"); //find all files in new folder
    
    hFiles = FindFirstFile(fBuffer,&Files); //find the first, associated with files
    hDirs = FindFirstFile(fBuffer,&Dirs); //find the first, associated with directories

    if(hFiles == INVALID_HANDLE_VALUE){
        MessageBox(Hwnd,"INVALID_HANDLE_VALUE/hFiles","Error",MB_OK);
        return false; //if the file handle is invalid, return false
    }

EDIT 2: Never mind... it was the strcat() in blue that was giving me the problem. I changed it to strcpy() and it works now.

I think new is overcomplicated in this code. Use a regular char array instead.
And I would like to say, avoid LPSTR and the like. Use the real type, such as char* instead.

I changed the LPSTR to char*, and the delete []'s are in when the buffers are no longer needed. However, the program gets the same Send Error Report dialog now after going through about 3 or 4 folders. There's also the problem of where the messagebox() in blue is, if that message box is not there it will not go through the directories loop. What's the deal with that?

Here's the revised code.

Code:

bool Search(LPSTR sFolder, int &fC, int &dC, int &total){
    
    char* lBuffer = new char[256]; //storage for _getcwd
    char* pbuffer = new char[256]; //storage for parent
    char* fBuffer = new char[256]; //storage for file name to be checked
    char* buffer = new char[256];
    
    WIN32_FIND_DATA Files,Dirs; //structure for handling all the files
    HANDLE hFiles = NULL, hDirs = NULL; //handles for FindNextFile()
    
    strcpy(fBuffer,sFolder); //set up new folder to search in
    strcat(fBuffer,"\\*.*"); //find all files in new folder
    
    hFiles = FindFirstFile(fBuffer,&Files); //find the first, associated with files
    hDirs = FindFirstFile(fBuffer,&Dirs); //find the first, associated with directories
    
    if(hFiles == INVALID_HANDLE_VALUE || hDirs == INVALID_HANDLE_VALUE){
        SetDlgItemText(Hwnd,TEST,"hFiles/hDirs = INVALID_HANDLE_VALUE");
        return false;
    }
    else if(hFiles != INVALID_HANDLE_VALUE || hDirs != INVALID_HANDLE_VALUE)
    {
        while((FindNextFile(hFiles,&Files) && GetLastError() != ERROR_NO_MORE_FILES) || (FindNextFile(hDirs,&Dirs) && GetLastError() != ERROR_NO_MORE_FILES))
        {
            if(Files.dwFileAttributes != FILE_ATTRIBUTE_DIRECTORY)
            {
                fCount++;
               //this is not returning as true
                if(strcmp("my_test_file.txt",Files.cFileName) == 0){
                    SetDlgItemText(Hwnd,TEST,"File found!");
                    
                    strcpy(FoundLocation,sFolder);
                    strcat(FoundLocation,Files.cFileName);
                    SetDlgItemText(Hwnd,CUR_PATH,FoundLocation);
                    
                }

            }
            
            if((Dirs.dwFileAttributes == FILE_ATTRIBUTE_DIRECTORY) && !(strcmp(Dirs.cFileName,"..") == 0 || (strcmp(Dirs.cFileName,".") == 0)))
            {
                char* StartFolder = new char[256]; //create new structure
                
                strcpy(StartFolder,sFolder);
                strcat(StartFolder,"\\");
                strcat(StartFolder,Dirs.cFileName); //add the new directory to it
                //strcpy(StartFolder,pbuffer); //assign new directory that is to be search
                
                dCount++;
                    
                Search(StartFolder,fCount,dCount,Total); //recursively call the function again with a new directory
                delete [] StartFolder;
            }
        }
    }
    
    delete [] pbuffer;
    delete [] fBuffer;
    delete [] lBuffer;
    delete [] buffer;

    return true;
}

EDIT: I changed the code so the file and directory searches are in the same loop, and it fixed the problem. Right now it seems to work correctly, but it doesn't. It does not display that it found my test file, but it's in one of the first directories that it would search. Did I miss something?