Problem with strcat() overwriting my string

I'm writing my own shell, and seem to have some kind of problem with strcat() overwriting a string unexpectedly.

The problem is when trying to execute a file in the local directory. The second value in the path it should search is '.' and the first is /bin, but when appending the command to /bin for the absolute path to give to execlp() the period is overwritten by the command as well. I know the MYPATH thing is strange and that it's weird to separate by #s, but that is irrelevant to the issue.

I put some useful printf() statements in if you want to run it to see what I'm talking about.

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <readline/readline.h>
#include <readline/history.h>
#include <sys/stat.h>

void execute(char**, int, char**, int);

int main (){
  char *command, *mypath, *buffer, *arglist[1024], *pathlist[1024], **ap;
  buffer = malloc(1024);
  int loop = 1;

  while (loop == 1){
    int argnum = 0, pathnum = 0;
    mypath = malloc(1024);
    if(getenv("MYPATH") == NULL)
      strcpy(mypath, "/bin#.");
    else
      strcpy(mypath, getenv("MYPATH"));
    printf("myshell$ ");
    command = readline("");

    if(strcmp(command, "exit") == 0 || strcmp(command, "quit") == 0)
      return 0;

    if(strcmp(command, "") == 0)
      continue;

    /*Tokenizes Command*/
    for(ap = arglist; (*ap = strsep(&command, " \t")) != NULL;){
      argnum++;
      if(**ap != '\0')
    if(++ap >= &arglist[1024])
      break;
    }

    /*Tokenizes Path*/
      for(ap = pathlist; (*ap = strsep(&mypath, "#")) != NULL;){
    pathnum++;
      if(**ap != '\0')
    if(++ap >= &pathlist[1024])
      break;
      }
      execute(pathlist, pathnum, arglist, argnum);
  }
  return 0;
}

void execute(char *pathlist[], int pathnum, char *arglist[], int argnum){
  pid_t pid;
  int i;
  int found = 0;
  struct stat buf;
  for(i = 0; i < pathnum; i++){
    if (found == 1)
      break;
    printf("pathlist[0]: %s\n", pathlist[0]);
    printf("pathlist[1]: %s\n", pathlist[1]);
    strcat(pathlist[i], "/");
    strcat(pathlist[i], arglist[0]);
    printf("Pathlist[0] after strcat: %s\n", pathlist[0]);
    printf("Pathlist[1] after strcat: %s\n", pathlist[1]);
    if(stat(pathlist[i], &buf) == 0){
      found = 1;
      pid = fork();
      if(pid == -1)
    printf("Error: Fork Failed\n");
      else if(pid == 0){
    if(argnum == 0)
      execlp(pathlist[i], arglist[0], (char *) NULL);
    else if(argnum == 1)
      execlp(pathlist[i], arglist[0], arglist[1], (char *) NULL);
    else if(argnum == 2)
      execlp(pathlist[i], arglis开发者_StackOverflowt[0], arglist[1], arglist[2], (char *) NULL);
    else if(argnum == 3)
      execlp(pathlist[i], arglist[0], arglist[1], arglist[2], (char *) NULL);
      }
      else if(strcmp(arglist[argnum-1], "&") != 0){
    wait(NULL);
      }
    }
    else if(stat(pathlist[i], &buf) == -1 && i == pathnum-1 && found == 0)
      printf("Error: Command '%s' not found.\n", arglist[0]);
  }
}

strsep() does not create an independent copy of the token it returns. It modifies the original string, replacing the delimiter (in this case, the '#' character) with a '\0', and returns a pointer that points to the beginning of the token within the original string.

This means that your pathlist[] pointers all point within the string pointed to by mypath - at the point where you call execute(), it looks like this:

               '/'   'b'   'i'   'n'   '\0'  '.'   '\0'
                ^                             ^
mypath ---------/                             |
                ^                             |
pathlist[0] ----/                             |
                                              |
pathlist[1] ----------------------------------/

pathlist[2] (null)

You can now see why you observe the behaviour you do - when you call strcat() on pathlist[0], it starts overwriting that array, beginning at the first '\0'. pathlist[1] remains pointing at the same location, but the contents of the string at that location have been overwritten by the strcat().

In the execute() function, you shouldn't try to directly concatenate onto pathlist[i]. Instead, construct a new string in a temporary location:

char execpath[4096];

if (snprintf(execpath, sizeof execpath, "%s/%s", pathlist[i], arglist[0]) >= sizeof execpath) {
    /* Path was too long, handle the error */
}

if (stat(execpath, &buf) == 0) {

I must admit I haven't tested your code or even read it very carefully, but I'd recommend dropping strcat entirely. It's unsafe because it may write past the end of a buffer without notice.

You might want to define some safe string functions and use those instead:

char *xstrdup(char const *s)
{
    char *copy = strdup(s);
    if (copy == NULL) {
        fprintf(stderr, "error: cannot copy string: %s\n", strerror(errno));
        exit(1);
    }
    return copy;
}

char *xstrcat(char *s, char const *append)
{
    size_t len = strlen(s) + strlen(append) + 1;
    size_t news = realloc(s, len);
    if (news == NULL) {
        free(s);
        fprintf(stderr, "error: cannot append strings: %s\n", strerror(errno));
        exit(1);
    }
    strcat(news, append);
    return news;
}