Why does opendir() change the file path string passed to it?

Question

I am trying to write a function given a current working directory will print all the contents of that directory as well as the contents in the sub directories in the cwd.

void printDir(char *cwd)
{
  printf("file path after printdir call: %s\n",cwd);
  DIR *dirPtr = opendir(cwd);
  struct dirent *dirEnt;

  char *slash = "/";
  char *dot = ".";
  char *dotdot = "..";
  char *pathPrefix = malloc(sizeof(cwd) + sizeof(slash)+1);
  pathPrefix = strcat(cwd,slash);

  if (dirPtr != NULL)
  {
      while((dirEnt = readdir(dirPtr)) != NULL)
      {
          char *temp = dirEnt->d_name;
          if (strcmp(temp,dot) != 0 && strcmp(temp,dotdot) != 0)
          {

             char *tempFullPath = malloc(sizeof(pathPrefix) + sizeof(temp) + 1);
             tempFullPath = strcpy(tempFullPath, cwd);
             strcat(tempFullPath, temp);

             printf("file path before we try to openthedir: %s\n",tempFullPath);
             DIR *tempSubDirPtr = opendir(tempFullPath);

             printf("filePath after we try to open this shit: %s\n",tempFullPath2);
             if (tempSubDirPtr != NULL)
             {
                  printf("file path right before a recursive call: %s\n",tempFullPath);
                  closedir(tempSubDirPtr);
                  printDir(tempFullPath);
           }
           printf("%s\n",tempFullPath);
        }
    }
}
else
{

With the debugging printf()'s the console output is:

current working directory string after getcwd: /home/TTU/canorman/testUtil
file path after printdir call: /home/TTU/canorman/testUtil
file path before we try to openthedir: /home/TTU/canorman/testUtil/find.c
filePath after we try to open this shit: /home/TTU/canorman/testUtil/find.c
/home/TTU/canorman/testUtil/find.c
file path before we try to openthedir: /home/TTU/canorman/testUtil/find2.c
filePath after we try to open this shit: /home/TTU/canorman/testUtil/find2.c
/home/TTU/canorman/testUtil/find2.c
file path before we try to openthedir: /home/TTU/canorman/testUtil/find
filePath after we try to open this shit: /home/TTU/canorman/testUtil/find
/home/TTU/canorman/testUtil/find
file path before we try to openthedir: /home/TTU/canorman/testUtil/testDir
filePath after we try to open this shit: /home/TTU/canorman/testUA
file path right before a recursive call: /home/TTU/canorman/testUA
file path after printdir call: /home/TTU/canorman/testUA
Could not open specified working directory
/home/TTU/canorman/testUA/

So as you can see in the last few lines of the output that the file string goes from

/home/TTU/canorman/testUtil/testDir

to

/home/TTU/canorman/testUA

I can't find anything in the opendir(3) man pages about this happening. Any ideas as to why this going on.

What did you want to happen instead? And what is the actual directory structure? In other words, what is wrong? — Lightness Races in Orbit, Dec 03 '19 at 23:29
You're overwriting memory, because you're not calling calling `malloc` with the right sizes (you're writing more than you've allocated). For example, `sizeof(cwd)` is giving you 4 (or maybe 8), not the length you want, which you can compute with `strlen(cwd)`. (I suspect there are other similar issues; that's just the first one I confirmed.) — Steve Summit, Dec 03 '19 at 23:31
`char *pathPrefix = malloc(sizeof(cwd) + sizeof(slash)+1);` does not allocate the amount of memory that you seem to think it does. `sizeof(cwd)` is the size of a pointer, and is unrelated to the length of the string. — William Pursell, Dec 03 '19 at 23:33
`pathPrefix = strcat(cwd,slash)` is actually **modifying** the buffer `cwd` is pointing to, and it's not at all clear this is allowed. Is there room in the buffer for that extra slash and trailing NUL ? This would be allocated in the caller where we can't see it. — Steve Friedl, Dec 03 '19 at 23:54

Steve Friedl · Answer 1 · 2019-12-04T00:08:36.440

This code is for sure wrong:

char *pathPrefix = malloc(sizeof(cwd) + sizeof(slash)+1);
pathPrefix = strcat(cwd,slash);

As noted by @Steve and @William, the sizeof are applying to the pointer instead of the character string, so it's allocating either 4 or 8 bytes each (plus one), and this is unlikely to be enough. strlen(s) counts the number of active characters in the string; you have to add +1 for the NUL byte yourself.

But a bigger problem is the use of strcat(), which appends the second string to the tail end of the first string, modifying that first string. This is NOT creating a new joined string for you, filling it into the newly-allocated (too small) memory.

This should behave better:

char *pathPrefix = malloc(strlen(cwd) + strlen(slash) + 1);
strcpy(pathPrefix, cwd);   // cwd goes to the start of the new buffer
strcat(pathPrefix, slash); // append the slash to the end of cwd

... then make similar changes in the rest of the code.

EDIT: When doing this kind of work, it's a good practice to const qualify any char * variables that you don't intend to modify (where the strings are readonly from your perspective). Doing this adds helpful documentation to your code, and the compiler will warn you of common mistakes.

Example:

void printDir(const char *cwd)
{
  ...
  const char *slash = "/";
  const char *dot   = ".";
  const char *dotdot = "..";

  ...

It's not legal to modify a string literal anyway, but had the cwd parameter been const-qualified, the compiler would have objected to strcat(cwd, slash) because it knows that the first parameter gets written to.

This doesn't really change the behavior of the code, but it's a good habit to get into. const is your friend.

Thank you! This helped a ton! – juggy_crisp Dec 04 '19 at 20:47 — juggy_crisp, Dec 04 '19 at 20:47

score 0 · Answer 2 · answered Dec 04 '19 at 00:13

There are some bugs related to string operations. Remember that str*() functions (strcat(), strcopy() etc.) put the result into buffer, which pointer is passed as the first argument. So:

1) About the code on beginning:

char *pathPrefix = malloc(sizeof(cwd) + sizeof(slash)+1);
pathPrefix = strcat(cwd, slash);

you should use mallocated buffer pointer as the first argument, of course you need to have the cwd copied here before strcat(). Below is proper code:

char *pathPrefix = malloc(sizeof(cwd) + sizeof(slash)+1);
strcpy(pathPrefix, cwd);
strcat(pathPrefix, slash);

2) Inside the first 'if' block in 'while' loop: you did it properly, but there is no need to assign 'tempFullPath' by value returned from 'strcpy()' - you already passed this pointer as the first argument, so the copying will be performed into this buffer. So, replace the:

char *tempFullPath = malloc(sizeof(pathPrefix) + sizeof(temp) + 1);
tempFullPath = strcpy(tempFullPath, cwd);
strcat(tempFullPath, temp);

with:

char *tempFullPath = malloc(sizeof(pathPrefix) + sizeof(temp) + 1);
strcpy(tempFullPath, cwd);
strcat(tempFullPath, temp);

3) Do NOT forget about freeing memory allocated by "malloc()' !!!! (every 'malloc()' must have corresponding 'free()').

Why does opendir() change the file path string passed to it?

2 Answers2