Shell program pipes C

Question

I am trying to run a small shell program and the first step to make sure my code is running properly is to make sure I get the correct command and parameters:

//Split the command and store each string in parameter[]
    cp = (strtok(command, hash));                      //Get the initial string (the command)
    parameter[0] = (char*) malloc(strlen(cp)+ 1);                     //Allocate some space to the first element in the array
    strncpy(parameter[0], cp, strlen(cp)+ 1);
    for(i = 1; i < MAX_ARG; i++)
    {
    cp = strtok(NULL, hash);                 //Check for each string in the array
    parameter[i] = (char*) malloc(strlen(cp)+ 1);
    strncpy(parameter[i], cp, strlen(cp)+ 1);                      //Store the result string in an indexed off array
        if(parameter[i]  == NULL)
        {
            break;
        }
    if(strcmp(parameter[i], "|") == 0)
    {
        cp = strtok(NULL, hash);
        parameter2[0] = (char*) malloc(strlen(cp)+ 1);
        strncpy(parameter2[0], cp, strlen(cp)+ 1);
        //Find the second set of commands and parameters
        for (j = 1; j < MAX_ARG; j++)
        {
            cp = strtok(NULL, hash);
            if (strlen(cp) == NULL)
            {
                break;
            }
            parameter2[j] = (char*) malloc(strlen(cp)+ 1);
            strncpy(parameter2[j], cp, strlen(cp)+ 1);
        }
        break;
    }

I am having a problem when I compare cp and NULL, my program crashes. What I want is to exit the loop once the entries for the second set or parameters have finished (which is what I tried doing with the if(strlen(cp) == NULL)

Answer 1

Let's look at your code:

parameter[0] = malloc(255);

Since strtok() carves up the original command array, you don't have to allocate extra space with malloc(); you could simply point the parameter[n] pointers to the relevant sections of the original command string. However, once you move beyond space-separated commands (in a real shell, the | symbol does not have to be surrounded by spaces, but it does in yours), then you will probably need to copy the parts of the command string around, so this is not completely wrong.

You should check for success of memory allocation.

cp = strtok(command, " ");                      //Get the initial string (the command)
strncpy(parameter[0], cp, 50);

You allocated 255 characters; you copy at most 49. It might be better to wait until you have the parameter isolated, and then duplicate it - allocating just the space that is needed. Note that if the (path leading to the) command name is 50 characters or more, you won't have a null-terminated string - the space allocated by malloc() is not zeroed and strncpy() does not write a trailing zero on an overlong string.

for (i = 1; i < MAX_ARG; i++)

It is not clear that you should have an upper limit on the number of arguments that is as simple as this. There is an upper limit, but it is normally on the total length of all the arguments.

{
    parameter[i] = malloc(255);

Similar comments about memory allocation - and checking.

    cp = strtok(NULL, " ");
    parameter[i] = cp;

Oops! There goes the memory. Sorry about the leak.

    if (strcmp(parameter[i], "|") == 0)

I think it might be better to do this comparison before copying... Also, you don't want the pipe in the argument list of either command; it is a notation to the shell, not part of the command's argument lists. You should also ensure that the first command's argument list is terminated with a NULL pointer, especially since i is set just below to MAX_ARG so you won't know how many arguments were specified.

    {
        i = MAX_ARG;
        cp = strtok(NULL, " ");
        parameter2[0] = malloc(255);
        strncpy(parameter2[0], cp, 50);

This feels odd; you isolate the command and then process its arguments separately. Setting i = MAX_ARG seems funny too since your next action is to break the loop.

        break;
    }
    if(parameter[i]  == NULL)
    {
        break;
    }
}

//Find the second set of commands and parameter
//strncpy(parameter2[0], cp, 50);
for (j = 1; j < MAX_ARG; j++)
{
    parameter2[j] = malloc(255);
    cp = strtok(NULL, " ");
    parameter2[j] = cp;
}

You should probably only enter this loop if you found a pipe. Then this code leaks memory like the other one does (so you're consistent - and consistency is important; but so is correctness).

You need to review your code to ensure it handles 'no pipe symbol' properly, and 'pipe but no following command'. At some point, you should consider multi-stage pipelines (three, four, ... commands). Generalizing your code to handle that is possible.

When writing code for Bash or an equivalent shell, I frequently use notations such as this script, which I used a number of times today.

ct find /vobs/somevob \
    -branch 'brtype(dev.branch)' \
    -version 'created_since(2011-10-11T00:00-00:00)' \
    -print |
grep -v '/0$' |
xargs ct des -fmt '%u %d %Vn %En\n' |
grep '^jleffler ' |
sort -k 4 |
awk '{ printf "%-8s %s %-25s %s\n", $1, $2, $3, $4; }'

It doesn't much matter what it does (but it finds all checkins I made since 11th October on a particular branch in ClearCase); it's the notation that I was using that is important. (Yes, it could probably be optimized - it wasn't worth doing so.) Equally, this is not necessarily what you need to deal with now - but it does give you an inkling of where you need to go.

Answer 2

I may have misunderstood the question, but your program won't ever see the pipe character, |.

The shell processes the entire command line, and your program will only be given it's share of the command line, so to speak.

Example:

cat file1 file2 | sed s/frog/bat/

In the above example, cat is invoked with only two arguments, file1, and file2. Also, sed is invoked with only a single argument: s/frog/bat/.