execvp() causing EXC_SOFTWARE and bizarre cin.getline loop?

Question

I'm running some code on Mac OSX 10.6.6 and XCode 3.2.4 and I have some pretty standard code: fork(), if pid == 0 then execvp with a command and the args (the args include the command as the first element in the array, and the array is null terminated).

We're going over this in my Operating Systems class and our assignment is to write a simple shell. Run commands with their args and switches, both redirects (< and >) and pipe (|). I'm getting several problems.

1) Sometimes I get the EXC_SOFTWARE signal while debugging (so far I haven't gotten it if I run the app outside of XCode, but I'm new to Mac and wouldn't know what that would look like if I did)

2) Sometimes the getline for the next command gets junk that seems to be printed by other couts. This begins looping forever, exponentially breaking. I have tested with printing getpid() with every prompt and only the beginning process prints these out, I don't appear to have an accidental "fork bomb."

Here's what I have so far:

#include <iostream>
#include <string>
#include <unistd.h>

using namespace std;

char** Split(char* buffer, int &count) {
    count = 1;
    for (int i = 0; i < strlen(buffer); i++) {
        if (buffer[i] == ' ') {
            count++;
        }
    }
    const char* delim = " ";
    char* t = strtok(buffer, delim);
    char** args = new char*[count + 1];
    for (int i = 0; i < count; i++) {
        args[i] = t;
        t = strtok(NULL, delim);
    }
    args[count] = 0;
    return args;
}

void Run(char** argv, int argc) {
    int pid = 0;
    if ((pid = fork()) == 0) {
        //for testing purposes, print all of argv
        for (int i = 0; i < argc; i++) {
            cout << "{" << argv[i] << "}" << endl;
        }
        execvp(argv[0], argv);
        cout << "ERROR 1" << endl;
        exit(1);
    } else if (pid < 0) {
        cout << "ERROR 2" << endl;
        exit(2);
    }
    wait(NULL);
}

int main(int argc, char * const argv[]) {
    char buffer[512];
    char prompt[] = ":> ";
    int count = 0;
    while (true) {
        cout << prompt;
        cin.getline(buffer, 512);
        char **split = Split(buffer, count);
        Run(split, count);
    }
}

It's exactly what I have, you should be able to cut, paste, and build.

I'm not the best at C++, and chances are there's a memory leak when I don't delete split but my main focus is the EXC_SOFTWARE signal and see what I'm doing wrong with my looping issue. Any thoughts?

EDIT:

The assignment requires very limited error checking and I'm assuming all input is correct. By correct I mean properly formatted and limited for my app to run the command, i.e. no bizarre space count, no & to run async, no multi piping commands, etc.

Answer 1

One problem is that you do not check the return from cin.getline(), so if you type EOF, the code goes into a tight loop. You're also leaking memory.

Try:

while (cout << prompt && cin.getline(buffer, sizeof(buffer))
{
    int count = 0;
    char **split = Split(buffer, count);
    Run(split, count);
    delete[] split;
}

The code in Split() does not really handle blank lines at all well. It seems to take an aeon to run execvp() when the only arguments are null pointers, which is what happens if you return a blank line.

---

I'm able to run multiple simple commands (such as 'vim makefile' and 'make shell' and 'ls -l' and 'cat shell.cpp' and so on - I even did a few with more than two arguments) OK with this, and I can quit the command (shell) with Control-D and so on. I have fixed it so it compiles with no warnings from g++ -O -Wall -o shell shell.cpp. I have not fixed the splitting code so that it handles empty lines or all blank lines correctly.

#include <iostream>
#include <string>
#include <unistd.h>

using namespace std;

char** Split(char* buffer, int &count) {
    count = 1;
    for (size_t i = 0; i < strlen(buffer); i++) {  // #1
        if (buffer[i] == ' ') {
            count++;
        }
    }
    char** args = new char*[count + 1];
    const char* delim = " ";
    char* t = strtok(buffer, delim);
    for (int i = 0; i < count; i++) {
        args[i] = t;
        t = strtok(NULL, delim);
    }
    args[count] = 0;
    return args;
}

void Run(char** argv, int argc) {
    int pid = 0;
    if ((pid = fork()) == 0) {
        //for testing purposes, print all of argv
        for (int i = 0; i < argc; i++)
        {
            if (argv[i] != 0)  // #2
                cout << "{" << argv[i] << "}" << endl;
            else
                cout << "{ NULL }" << endl;  // #3
        }
        execvp(argv[0], argv);
        cout << "ERROR 1" << endl;
        exit(1);
    } else if (pid < 0) {
        cout << "ERROR 2" << endl;
        exit(2);
    }
    wait(NULL);
}

int main(int argc, char * const argv[]) {
    char buffer[512];
    char prompt[] = ":> ";
    while (cout << prompt && cin.getline(buffer, sizeof(buffer)))  // #4
    {
        int count = 0;
        char **split = Split(buffer, count);
        if (count > 0)  // #5
            Run(split, count);
        delete[] split;  // #6
    }
}

I've marked the significant changes (they mostly aren't all that big). I'm compiling with GCC 4.2.1 on MacOS X 10.6.6.

I can't readily account for the garbage characters you are seeing in the buffer.

Answer 2

You're making the assumption that the input line contains one more token than spaces. This assumption may fail if the input line is empty, ends or begins with a space or contains multiple consecutive spaces. In these cases, one of the calls to strtok will return NULL, and this will crash the forked process when you try to print that argument in Run. These are the only cases in which I've encountered problems; if you've encountered any others, please specify your input.

To avoid that assumption, you could do the counting with strtok the same way you do the tokenizing. That's generally a good idea: if you need two things to coincide and you can do them the same way, you introduce an additional source of errors if you do them differently instead.