C - A simple shell on linux - Some trouble with commands

Question

I wrote this simple shell so far. But I got some trouble with my shell. For example, when I try to open a pdf file via the command "evince pdffile.pdf", the actual pdfile does not get opened. The pdf viewer runs but the actual file with the whole content never appears. Or another example is the command "ls -l". I don't get the files and folders listed as it should be, but "ls" is working. Another example is gedit, and so on. Also, I should mention. I am not using "system()", because system() would do everything and I would not have something to do. Instead, I am using "execvp()". Here is the code. I hope, you may find the problem, because I have no clue what the problem is causing.

#include <stdio.h>
#include <sys/types.h>
#include <unistd.h>
#include <stdlib.h>
#include <sys/wait.h>
#include <string.h>
#include <errno.h>

#define MAX_LENGTH 1024
#define DELIMS " \t\r\n"

void exec_cmd (char *buf);

int main() {    
   char line[MAX_LENGTH];
   char * cmd;
   char curDir[100];

   while (1) {  
       getcwd(curDir, 100);
       printf("%s@%s$ ", getlogin(), curDir);
       if (!fgets(line, MAX_LENGTH, stdin))
           break;

       if ((cmd = strtok(line, DELIMS))) {
           errno = 0;
           if (strcmp(cmd, "cd") == 0) {
              char *arg = strtok(0, DELIMS);

              if (!arg) 
                 fprintf(stderr, "cd: argument is missing.\n");
              else chdir(arg);

           } else if (strcmp(cmd, "exit") == 0) {
                 exit(0);

           } else exec_cmd(line);

        if (errno) perror("Error. Command failure");
        }
   }
return 0;
}

void exec_cmd (char *buf) {
    int status = 0;
    char *argv[MAX_LENGTH];
    int j=0;
    pid_t pid;
    argv[j++] = strtok (buf, DELIMS);
    while (j<MAX_LENGTH && (argv[j++]=strtok(NULL,DELIMS))!=NULL); // EDIT: " " replaced by DELIMS 

    pid = fork();
    if(pid < 0) {
       printf("Error occured");
       exit(-1);
    } else if(pid == 0) {
         execvp(argv[0],argv);
    } else if( pid > 0) {
         wait(&status);
    }
}

score 1 · Answer 1 · edited May 23 '17 at 12:19

1

Check the arguments (eg, print them out, each enclosed in []) before you call fork/exec, there's a good chance they're not what you think.

While your first call to strtok uses your full delimiter set, subsequent calls do not. They instead just use a space. That means that the final argument will probably have the newline left on the string by fgets. I'd be using the same delimiter set in the subsequent calls. In other words:

while (j<MAX_LENGTH && (argv[j++]=strtok(NULL,DELIMS))!=NULL);

Having entered your code and done that debugging, I find that the string passed to the function only ever has one word in it. It turns out that's because of the strtok that happened in main to check for cd/exit. That left the nul character at the end of the first word, an effect inherent in the way strtok works.

Probably the quickest fix is to make a copy of the string before the initial strtok in main, then pass that to the function. In other words, use strdup (and, later, free). Now glibc has a strdup but, if you're in an environment that doesn't (it's POSIX rather than ISO), see here.

edited May 23 '17 at 12:19

Community

1
1

answered Nov 21 '13 at 20:01

paxdiablo

854,327
234
1,573
1,953

Yes, you are right. I missed the last strtok. I corrected this mistake, but the problem still exists. – Nov 21 '13 at 20:18
@user2965601: and, when you printed out the args, what did you see? Both before and after the fix. That's the crux of the answer, the best wat to debug what the actual problem is. – paxdiablo Nov 21 '13 at 20:21
I copied the content of line[] in another array with a while loop. It seems to be working. Should I still use strdup? – Nov 21 '13 at 20:39
@user2965601, yes, you should. You should generally prefer using language features to coding your own. You'll deliver your software a lot faster that way :-) – paxdiablo Nov 21 '13 at 20:44
I did it as you said. Also, thank you for your help. It helped a lot. :) – Nov 21 '13 at 21:13
I have another question. I don't know if it's ok to ask another question here, but I just go ahead and will ask you. You tell me, if it is not ok. I just tried to add an extra feature to my shell. It should return the running time of an program. I used clock(). I put "start = clock()" before execvp() and "end = clock()" after wait(&status). But it seems not to be working. Also, I'd like to have it formatted like hh:mm:ss. Do you know if it is there a clever way to do that? – Nov 21 '13 at 21:56
@user2965601, would get more visibility as separate question but I'll help out. 1/ clock() gives cpu time, not elapsed time. That may be < elapsed if child sleeps or > elapsed if child has many threads. `gettimeofday` may be better. 2/ You should set start before the fork rather than exec since you want to do all this in the parent. As to hh.mm.ss, best way is to probably just use division/modulo with 60. That's about as much detail as will fit in a comment, so you may want to ask a couple more questions. That's how SO works best. – paxdiablo Nov 21 '13 at 22:17
I will try your suggestion out. If I fail, I'll ask it as a separate question. Would it be ok a reference to this question or shall I put the whole code online again? – Nov 21 '13 at 22:43

score 1 · Answer 2 · answered Nov 21 '13 at 20:18

1

The bug is in main().

You are using strtok to find the first word of the line. But strtok modifies the line, making it actually contain just that word (a NUL terminator is written to the string right after it).

You need to make a copy of the line, or use strtok_s, or do something else to avoid modifying the line.

answered Nov 21 '13 at 20:18

n. m. could be an AI

112,515
14
128
243

To be clear if "line" contains "hello world", when you strtok line it will become "hello\0world" so when you pass line to exec_cmd it is interpreted as a string containing just "hello". – Duck Nov 21 '13 at 20:20
You are right. This was causing the problem. I would not have seen that without you, guys. Thank you! :) – Nov 21 '13 at 20:29
@Duck: Right before, I tried to print it out, but it did not work. Could you maybe show me how to print it correctly? – Nov 21 '13 at 20:31
Not exactly sure what you are trying to print. `printf("%s\n",line);` before and after the `strtok` to see this. – Duck Nov 21 '13 at 20:52
Or better, `printf("[%s]\n",line);` so you can see extraneous newlines easier. – paxdiablo Nov 21 '13 at 21:09

C - A simple shell on linux - Some trouble with commands

2 Answers2