Append new path to variable makes program crash

Question

I have a function that adds a directory to a defined path, more or less, exactly as the "cd-function" in the command prompt. The first time I add a directory everything works perfectly, the second or third time (depends on the length of the directory) the program crashes.

char path[PATH_MAX];

void dir_forward(char cmd[])
{
    char *temp = malloc(strlen(path) + strlen(cmd) + 2);
    char sep[] = "\\";

    strcpy(temp, path);      // copy path to temp

    strcat(temp, sep);
    strcat(temp, cmd+3);     // +3 for removing "cd " from string

    int i = strlen(temp);    // add null terminator
    temp[i] = '\0';

    strcpy(path, temp);

    free(temp);
    printf("%s\n", path);
}

The program first copies the path to a temporary variable (the size of the temporary variable is determined as the size of the path, the new directory, one backslash, and a null terminator). After that, the backslash and the new directory gets copied to the path in the temporary variable. A null terminator is appended to the end of the string. After that, the new path in the temporary variable gets copied to the path and thus replaces the old one.

Let me demonstrate with an example, I have:

C:\Users\Paul\Desktop\some_folder

I decided to append the folder "images" to the path:

C:\Users\Paul\Desktop\some_folder\images

If I append one more folder (preferably with a long name) the program will crash almost like a buffer overflow and also end the path with some weird characters.

[EDIT]

I cannot reproduce the code completely since its rather large, however, below is the essence of the function. First, the program adds a path to the variable path with the help of getcwd(), after that accepts an input and sends it to the function above.

int main(void)
{
    char cmd[30];

    getcwd(path, PATH_MAX);     // Get current path
    scanf("%s", cmd);
    dir_forward(cmd);
}

This should be enough to reproduce the problem I guess.

That "add null terminator" bit is not needed, it should already be there. If it wasn't then the `strlen` call would not work. — Some programmer dude, Nov 14 '18 at 07:05
is your input string "cmd[]" being terminated properly by your input handler? — arjayosma, Nov 14 '18 at 07:05
Have you printed the value in `path` before and after the append? Have you printed the length? Have you checked that the `malloc()` was successful? If you're going to copy the result into the fixed-size `path`, why not use a fixed size variable of the same length to hold the intermediate value? Adding a null terminator where `strlen()` determines the end of the string is pointless; `strlen()` stops when it reaches a null byte. Where do you check that the concatenated value will fit into `path`? The `cmd + 3` is a bit peculiar. It'll insert a space into the path if you type `cd path`. — Jonathan Leffler, Nov 14 '18 at 07:05
@chaos And that might *not* add the null terminator. And if not, then the `strlen` function can't be used to find the actual end (though then it's just to add the terminator at the last place in the array anyway, it will however make the path will invalid). — Some programmer dude, Nov 14 '18 at 07:14
Answer 1 – That’s correct, I will remove it. Answer 2 – Yes, the string is terminated properly. Answer 3 – When the program gets executed the path of the program is sent to path with the help of getcwd(). I honestly never check if the new path fits into the path-variable, it was totally base don an assumption on my part. May this be the problem? Should the cmd + 3 be changed to cmd + 4? Answer 4 – Will try to change the god per your advice, thanx. — Lavonen, Nov 14 '18 at 07:23
@Lavonen You should add this detail to your question using the edit button. — Fantastic Mr Fox, Nov 14 '18 at 07:27
I added a little bit more code to make it more clear, please update your browser :) — Lavonen, Nov 14 '18 at 07:29
@LavonenWhat is `PATH_MAX`? What is your platform? Please post a [MCVE], not your complete code. Does the problem go away if you declare, say `char path[2000];`? — Jabberwocky, Nov 14 '18 at 07:33
That was a mistake by me (was a little too fast on the keyboard). I have updated the code again. No, char path[2000] doesn't help, already tried it. — Lavonen, Nov 14 '18 at 07:38
@Lavonen and what happens if you naively try `malloc(strlen(path) + strlen(cmd) + 30);`? — Jabberwocky, Nov 14 '18 at 07:41
@Lavonen you write : _"This should be enough to reproduce the problem I guess"_. Are you able to reproduce the problem with this? I'm not. The problem maybe elsewhere in your program, in some code you don't show here. Did you use a debugger? The debugger will show you _where_ your program crashes. — Jabberwocky, Nov 14 '18 at 07:50
The answer below seems to be correct so far, I removed the part that adds a null terminator and also added one extra char (30 like your example also works). Everything works fine with this configuration as long as the name of the path doesn't get too long (why this is a problem I dont know). — Lavonen, Nov 14 '18 at 07:53
The newly added "mcve" main is total nonsense! `scanf` with `%s` **cannot scan** `cd somedirectory` because `%s` scans whitespace-delimited strings! — Antti Haapala -- Слава Україні, Nov 14 '18 at 08:01
The unsafe string functions are so 1990's ... [Which string manipulation functions should I use?](https://stackoverflow.com/q/4185634/608639) — jww, Nov 14 '18 at 17:25

score 1 · Answer 1 · answered Nov 14 '18 at 08:36

It's not obvious why the posted code fails, but the rule #1 here is to always check the size of the data before copying. In addition, the whole function can be simplified by dropping the malloc. For example:

void dir_forward (const char cmd[])
{
  cmd += sizeof("cd ") - 1;  // remove "cd "

  size_t dst_length = strlen(path);
  size_t src_length = strlen(cmd);
  size_t new_length = dstr_length + src_length + 2; // '\\' and '\0' give +2
  if(new_length > PATH_MAX)
  {
    exit(EXIT_FAILURE);
  }

  char* ptr = &path[dst_length]; // point at null terminator

  memcpy(ptr, cmd, src_length);
  ptr += src_length;

  *ptr = '\\';
  ptr++;

  *ptr = '\0';
}

Full example:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#ifndef PATH_MAX
#define PATH_MAX 255
#endif

static char path[PATH_MAX] = "C:\\tmp\\";

void dir_forward (const char cmd[])
{
  cmd += sizeof("cd ") - 1;  // remove "cd "

  size_t dst_length = strlen(path);
  size_t src_length = strlen(cmd);
  size_t new_length = dstr_length + src_length + 2; // '\\' and '\0' give +2
  if(new_length > PATH_MAX)
  {
    exit(EXIT_FAILURE);
  }

  char* ptr = &path[dst_length]; // point at null terminator

  memcpy(ptr, cmd, src_length);
  ptr += src_length;

  *ptr = '\\';
  ptr++;

  *ptr = '\0';
}

int main (void)
{
  dir_forward("cd foo");
  puts(path);
}

I found the problem and it was higher up in the code, the variable for the command (cmd) was too small due to another function which created a buffer overflow. Your answer really helped me to brush up my code though so one upvote for you :) Thanx — Lavonen, Nov 14 '18 at 09:16

score -1 · Answer 2 · answered Nov 14 '18 at 08:20

-1

I found a solution, post it for future reference.

The problem was this part:

free(temp);

I don't know why (though this was necessary) but I had to remove this one.

Now, at least, everything works.

answered Nov 14 '18 at 08:20

Lavonen

606
1
10
21

This is not the solution, it is the consequence of the bug. It means that you have screwed up the use of the `temp` pointer earlier on in the function by assigning it to another address. The solution is not to remove free() but to fix the actual bug. – Lundin Nov 14 '18 at 08:27
Damn... I will continue upward in the code. The problem must be somewhere higher up. – Lavonen Nov 14 '18 at 08:31

Append new path to variable makes program crash

2 Answers2