Writing a shell in C, doesn't return anything

Question

I run my shell and it prompts: "Shell>". I type a command, like ls, and it just makes a new line that says "Shell>" again.

Any idea why it doesn't seem to be hitting execv?

            int no_of_args = count(buffer);
            // plus one to make it NULL
            char** array_of_strings = malloc((sizeof(char*)*(no_of_args+1)));

            //  break the string up and create an array of pointers that
            // point to each of the arguments.
            int count=0;
            char* pch2;
            pch2 = strtok (buffer," ");
            while (pch2 != NULL)
            {
                array_of_strings[count]=(char*)malloc((sizeof(char)*strlen(pch2)));
                strcpy(array_of_strings[count], pch2);

                pch2 = strtok (NULL, " ");
                count++;
            }

            //format for command is eg. ls -a -l
            //therefore the first element in the array will be the program name
            //add the path so it'll be /bin/command eg. /bin/ls
            char* prog = malloc(sizeof(char)*(strlen(array_of_strings[0]+strlen(path))));
            prog = strcat(strcpy(prog, path),array_of_strings[0]);


}

Answer 1

First things first, you never need to use sizeof(char) since it's always 1.

ISO C99 defines byte thus:

addressable unit of data storage large enough to hold any member of the basic character set of the execution environment.

and later states in 6.5.3.4 The sizeof operator:

When applied to an operand that has type char, unsigned char, or signed char, (or a qualified version thereof) the result is 1.

This is unchanged in C11.

So, basically, a byte is the size of your char. ISO generally reserves the term octet for an 8-bit value.

---

Secondly, a statement sequence like:

array_of_strings[count]=(char*)malloc((sizeof(char)*strlen(pch2)));
strcpy(array_of_strings[count], pch2);

is undefined behaviour since strlen(pch2) is just one short of being enough space to store a copy of the string pointed to by pch2. You should be using something like:

array_of_strings[count] = malloc (strlen (pch2) + 1);

You'll also notice I removed the cast. You should never cast the return value of memory allocation functions in C since it can hide problems under some circumstances.

Thirdly, you don't seem to be following the rules with your argv array. The last element in this array should be a NULL pointer, as in the command ls x.txt would generate:

• "ls".
• "x.txt".
• NULL.

---

Now, on to your specific problem. You should be checking the return value from your execv call since there's no guarantee that the executable will run (for example, if ls were not in the /bin directory). I would change:

int rv = execv(prog, array_of_strings);

into:

printf ("DEBUG: [%s]\n", prog);
int rv = execv(prog, array_of_strings);
printf ("DEBUG: execv returned %d/%d\n", rv, errno); // need errno.h

for debugging purposes, and see what that outputs.

If the execv works, you'll never see that final message. If it appears, it will tell you why the execv didn't work. When I do that, I see:

DEBUG: [/bin/ls
]
DEBUG: execv returned -1/2

In other words, the executable name that you're trying to run is /bin/lsX, where X is the newline character. There's no such executable, hence the error 2 (ENOENT = No such file or directory) from execv - you need to fix up the parsing code so that the newline is not left in.

As a quick debug fix, I changed the line:

prog = strcat(strcpy(prog, path),array_of_strings[0]);

into:

prog = strcat(strcpy(prog, path),array_of_strings[0]);
if (prog[strlen(prog)-1] == '\n') prog[strlen(prog)-1] = '\0';

to get rid of the trailing newline if it's there, and the file listing was then successful:

Shell>ls
DEBUG: [/bin/ls]
accounts2011.ods  birthdays    shares    workspace_android
accounts2012.ods  development  wildlife
Shell>_

That's just a debugging thing for proof, unsuitable for real code, so you still have to go and fix your parsing.

---

You might want to have a look at this answer as it shows a good way to get input from the user with buffer overflow protection, cleaning up the rest of the input line if too long, prompting and (most important in this case) removal of the newline.

Answer 2

The line

char* prog = malloc(sizeof(char)*(strlen(array_of_strings[0]+strlen(path))));

seems wrong. Are you sure you don't mean

char* prog = malloc(sizeof(char)*(strlen(array_of_strings[0])+strlen(path)));

(note moved parenthesis). strlen(array_of_strings[0]+strlen(path)) (the number of bytes you're reserving) will bear an unpredictable relationship to the sum of the lengths of array_of_strings[0] and path (the strings you're trying to concatenate). This may be causing a segfault.

Answer 3

There is also something weird happening with the current working directory after the execv. Try "Shell>pwd", or even "Shell>ls /home/".

Anyway, I think I might have solved it by removing the '\n' character at the end of the "buffer" string, right after the fgets. See if it works for you:

 fgets(buffer, 512, stdin);
 int j = strlen(buffer) - 1;
 if (buffer[j] == '\n')
     buffer[j] = 0;

Still a mystery to me why the weird CWD behaviour happened...

Hope this helps.