newb readdir() trouble

PatrickNew · 02-08-2007, 08:31 PM

I'm having some trouble in a program I'm writing. It's in C, and uses an ncurses interface. My trouble is apparently with the readdir() function. Through commenting out code, I've narrowed the problem down to a single while loop, which seems to be looping infinitely, but I can't see why.

The objective of this code is to get a list of all the files in the directory whose location is stored in char* location. They are stored in char** files.

sizeOfFiles and sizeOfArray are integers, sizeOfFiles contains the number of actual entries in "files" and sizeOfArray is an ugly hack to only expand file by 5 bytes at a time (expanding it for each entry did bad things). char* tmp1 is just a temporary placeholder. dirp is a pointer of type DIR and dp is pointer to a struct dirent.

Code:

while((dp = readdir(dirp)) != NULL)
{
if (sizeOfFiles == sizeOfArray)
    {files = realloc(files, sizeOfArray + 5);
     sizeOfArray += 5;};
if (dp == NULL)
    {
    tmp1 = malloc(1);
    tmp1 = '\0';
    files[sizeOfFiles - 1] = tmp1;
    sizeOfFiles++;
    }
else
    {
    tmp1 = malloc(strlen(dp->d_name));
    strcpy(tmp1, dp->d_name);
    files[sizeOfFiles - 1] = tmp1;
    sizeOfFiles++;
    }

};

I think I'm doing this the same way as the code examples. hrm.

wjevans_7d1@yahoo.co · 02-08-2007, 09:27 PM

Two comments.

First, there's an error here; it's a quite common one.

Code:

tmp1 = malloc(strlen(dp->d_name));

strlen() returns the length of a string, not including the delimiting NUL character. strcpy() copies everything up to and including the delimiting NUL character.

You want to say:

Code:

tmp1 = malloc(strlen(dp->d_name)+1);

Another common error (please avoid this one too) is to say instead:

Code:

tmp1 = malloc(strlen(dp->d_name+1));

That gives you a new area that's two bytes less than what you need.

Second comment:

Quote:

an ugly hack to only expand file by 5 bytes at a time (expanding it for each entry did bad things)

is a hack that's not just ugly. It's to be avoided with extreme horror, at all times. Never ever ever just tweak the code because it seems to make the problem go away. Never apply a fix unless you know what it fixes and why. If you do apply such a fix, it only makes the bug harder to find. Murphy's law says that more often than not, sweeping such a bug under the rug will make the bug want to crawl out as soon as the code is shipped to a crucial customer, or handed in to a teacher.

Now, it could be that your original problem will be fixed by my observation. But there could also be another bug.

Hope this helps.

PatrickNew · 02-08-2007, 10:23 PM

Okay, updated to fix the malloc() error, and removed the hack returning to the original implementation involving repeated calls to realloc(). Now I'm back to the problem I had, which the hack tried to solve. There is some value, specifically which is getting passed to realloc();. The error message is "./nAudio" realloc(): invalid next size 0x8055648".

This value is far larger than any my app should be sending, which leads me to believe that I'm in an infinite loop with sizeOfFiles being incremented. But, I still don't know how this loop gets going. Upon encountering the last entry in the directory being read, readdir() should return a null pointer. This should be caught by the while. This puzzles me.

Here's the updated code for this segment:

Code:

while((dp = readdir(dirp)) != NULL)
{
if (dp == NULL)
    {
    tmp1 = malloc(1);
    tmp1 = '\0';
    files[sizeOfFiles - 1] = tmp1;
    sizeOfFiles++;
    }
else
    {
    files = realloc(files, sizeOfFiles + 1);
    tmp1 = malloc(strlen(dp->d_name) + 1);
    strcpy(tmp1, dp->d_name);
    files[sizeOfFiles - 1] = tmp1;
    sizeOfFiles++;
    }

};

Guttorm · 02-09-2007, 01:44 AM

Hi

I added some comments to your code:

Code:

while((dp = readdir(dirp)) != NULL)
{
if (dp == NULL)
    {
    /* this code is unreachable, the while just compared dp with NULL */
    tmp1 = malloc(1);
    tmp1 = '\0';
    files[sizeOfFiles - 1] = tmp1;
    sizeOfFiles++;
    }
else
    {
    files = realloc(files, sizeOfFiles + 1);
    tmp1 = malloc(strlen(dp->d_name) + 1);
    strcpy(tmp1, dp->d_name);
    /* tmp1 = strdup(dp->d_name) is a bit simpler than the two lines above */
    files[sizeOfFiles - 1] = tmp1;
    /* why -1? The first file will be placed below the array? */
    sizeOfFiles++;
    }

};

My guess is the "files[sizeOfFiles - 1] = tmp1;" Typically when you allocate some memory, the size is a bit below the pointer you get, and this size gets overwritten on the first file found.

varun_shrivastava · 02-09-2007, 03:26 AM

Code:

# include <stdlib.h>
# include <dirent.h>
main()
{

DIR *dirp;
struct dirent *dp;
int sizeOfFiles =0, sizeOfArray = 100, i;//declare sizeOfArray as                   
                                         //100 

char *tmp1 , **files, **files1;

dirp = opendir("./.");


files = (char **)malloc(sizeOfArray);//allocate here only the  
                                     //required size
                                //rather than doing realloc again 
                                 //and again

while((dp = readdir(dirp)) > 0 )//readdir returns an int not 
                                //pointer check man page
{

/*
        THIS CONDITION IS NEVER TOUCHED SO I HAVE COMENTED IT
        if(dp == NULL)
        {
                tmp1 = malloc(1);
                tmp1 = '\0';
                files[sizeOfFiles] = tmp1;
                sizeOfFiles++;
        }       */

                tmp1 = (char *)malloc(strlen(dp->d_name)+1);
                strcpy(tmp1,dp->d_name);
                files[sizeOfFiles] = tmp1;
        printf("%d   %s\n",sizeOfFiles,*(files+sizeOfFiles));
                sizeOfFiles++;

}
}

PatrickNew · 02-09-2007, 02:10 PM

Quote:

Originally Posted by varun_shrivastava

readdir returns an int not pointer, check man page

From the manpage:
The readdir() function returns a pointer to a dirent structure, or NULL if an error occurs or end-of-file is reached. On error, errno is set appropriately.

dp is defined as a pointer to a dirent structure. Thus, shouldn't my while() conditional find that dp == NULL after the EOF is reached? At the end of the directory, readdir() will return the null pointer, that will be stored in dp.

Revised code is below:

Code:

while((dp = readdir(dirp)) != NULL)
{
files = realloc(files, sizeOfFiles + 1);
tmp1 = strdup(dp->d_name);
files[sizeOfFiles] = tmp1;
sizeOfFiles++;
};

tmp1 = malloc(1);
tmp1 = '\0';
files[sizeOfFiles - 1] = tmp1;
sizeOfFiles++;
closedir(dirp);

however the same problem remains.

varun_shrivastava · 02-10-2007, 12:31 AM

yes u r correct
i thought of readdir(2)

is it necessary to to do realloc again and again

wjevans_7d1@yahoo.co · 02-10-2007, 06:44 AM

Um, Patrick, I'm getting confused.

1. Could you please post the whole program, not just a fragment?

2. Could you please reiterate just what happens with the program as revised so far? (Sorry to be so dense.)

PatrickNew · 02-11-2007, 10:34 PM

Well, the whole program would be too much code and irrelevant, but I'll post the whole function. This program crashes immediately upon invocation (the function in question is called during the initialization of the program). The error message is "./nAudio" realloc(): invalid next size 0x8055648". The application (an ncurses app) does not return to terminal to regular text mode, forcing reset. The whole function is below:

Code:

void refreshListOfFiles(char* location)
{
DIR *dirp;
struct dirent *dp;
unsigned int counter = 0;
char* tmp1;


/*Free the space holding the filenames*/
while(counter < sizeOfFiles)
{free(files[counter]);
 counter++;};

/*Free files*/
free(files);
sizeOfFiles = 1;
files = malloc(1);

/*Open the directory*/
dirp = opendir(location);

/*Put the entries in files*/
while((dp = readdir(dirp)) != NULL)
{
   files = realloc(files, sizeOfFiles + 1);
   tmp1 = strdup(dp->d_name);
   files[sizeOfFiles] = tmp1;
   sizeOfFiles++;
};

tmp1 = malloc(1);
tmp1 = '\0';
files[sizeOfFiles - 1] = tmp1;
sizeOfFiles++;
closedir(dirp);

}

unsigned int sizeOfFiles and char** files are global variables.

wjevans_7d1@yahoo.co · 02-12-2007, 07:28 AM

My guess is that you have heap corruption.

Your program's memory is divided into several parts:

non-modifiable segments, such as the code itself
your stack, including local variables in your functions
the heap, which is organized by malloc(), realloc(), and free(), as well as other functions which call these three (such as strdup())

When you allocate a chunk of memory with malloc() or realloc(), there is nothing to prevent you from writing beyond the end of it, or before the beginning of it (if, for example, you [usually accidentally] use a negative subscript). Once that is done, it is not guaranteed that your program will crash immediately; it might crash later when you do another malloc(), realloc(), or free(). I'm thinking that's what's happened in your situation.

The bad news is that there's no guarantee that the actual bug is in the function which you posted.

The good news is that you can force your program to crash at the point at which you actually do the illegal writing to memory. Electric Fence is what you need.

I've used it to find instantly something that would have taken days of pulling my hair out. (Although my partner says that I don't have that much hair to pull, but that's a subject for another day.)

http://directory.fsf.org/ElectricFence.html

Hope this helps.