1

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], arglist[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]);
  }
}
1
  • Unrelated to your issue, but you should really lookup the execvp function. Writing a different execlp call for each possible number of arguments, while possible, is incredibly ugly and inefficient. Feb 28, 2011 at 18:20

2 Answers 2

3

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) {
1
  • That's interesting and makes perfect sense as to why I was getting the bug. I did end up using a temporary string and got it working, but now I know why it was doing it. Thanks a lot this is a really great explanation and a perfect answer to the question. Mar 1, 2011 at 4:24
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;
}
2
  • xstrcat() is b0rken, it doesn't strcat() to the new memory, but it does return it.
    – unwind
    Feb 28, 2011 at 18:02
  • @unwind: good point, almost fell into the trap I warn about :)
    – Fred Foo
    Feb 28, 2011 at 18:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.