Programming a shell in C

Question

I'm currently programming a shell in C and I'm running into a few issues. When I try to compare my command to an "exit" for example, it just runs write over it and acts like they don't equal according to gdb. I end with a segfault. If anyone could help me figure out what's wrong, I'd greatly appreciate it. This is my first shell ever btw!

#include <stdio.h>
#include <string.h>
#include <strings.h>
#include <limits.h>
#include <unistd.h>
#include <stdlib.h>
#include <pwd.h>
#include <dirent.h>e
#include <sys/types.h>
#include <sys/wait.h>    
#include <signal.h>
#include "sh.h"

int sh( int argc, char **argv, char **envp ){

    char *prompt = calloc(PROMPTMAX, sizeof(char));
    char *commandline = calloc(MAX_CANON, sizeof(char));
    char *command, *arg, *commandpath, *p, *pwd, *owd;
    char **args = calloc(MAXARGS, sizeof(char*));
    int uid, i, status, argsct, go = 1;
    struct passwd *password_entry;
    char *homedir;
    struct pathelement *pathlist;

    uid = getuid();
    password_entry = getpwuid(uid);
    homedir = password_entry->pw_dir; 

    if ( (pwd = getcwd(NULL, PATH_MAX+1)) == NULL ){
    perror("getcwd");
    exit(2);
    }

    owd = calloc(strlen(pwd) + 1, sizeof(char));
    memcpy(owd, pwd, strlen(pwd));
    prompt[0] = ' '; prompt[1] = '\0';

    pathlist = get_path();

    prompt = "[cwd]>";

    while ( go ){
    printf(prompt);

    commandline = fgets(commandline, 100, stdin);
    command = strtok(commandline, " ");

    printf(command);

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

    else if (strcmp(command, "which")==0){
    //  which();
    }

    else if (strcmp(command, "where")==0){
    //  where();
    }

    else if (strcmp(command, "cd")==0){
        chdir(argv[0]);
    }

    else if (strcmp(command, "pwd")==0){
        getcwd(pwd, PATH_MAX);
    }

    else if (strcmp(command, "list")==0){
        if (argc == 1){

        }

        else if (argc > 1){

        }
    }

    else if (strcmp(command, "pid")==0){
        getpid();
    }

    else if (strcmp(command, "kill")==0){

    }

    else if (strcmp(command, "prompt")==0){
        prompt = "argv[0] + prompt";
    }

    else if (strcmp(command, "printenv")==0){

    }

    else if (strcmp(command, "alias")==0){

    }

    else if (strcmp(command, "history")==0){

    }   

    else if (strcmp(command, "setenv")==0){

    }

    else{
        fprintf(stderr, "%s: Command not found.\n", args[0]);
    }



}
return 0;

}

Most of it is still bare bones so bear with me.

You might want to try the code review stack exchange. http://codereview.stackexchange.com/ — OmnipotentEntity, Sep 24 '12 at 03:47
I'd start by creating a "map" from names of commands to functions that carry out those functions instead of the giant `if`/`then`/`else` ladder. — Jerry Coffin, Sep 24 '12 at 03:49
Been there. May I make a suggestion? Use a despatch table instead of all those `else if`s, which frankly is not scalable. Create a struct of a char array to hold the built-in name, plus a function pointer to the built-in function (all function should have the same prototype). Create an array of these structs, manually sorted by built-in name and use `bsearch` to find the correct function for each built-in. — cdarke, Sep 24 '12 at 16:13

score 4 · Accepted Answer · edited May 23 '17 at 12:04

4

If you change:

printf(command);

into:

printf("<<%s>>\n",command);

you'll see why it will never match any of those strings. That's because fgets does not strip off the trailing newline ^(a):

[cwd]>ls
<<ls
>>

Which means it will execute this line of code:

fprintf(stderr, "%s: Command not found.\n", args[0]);

And, since you've initialised all those args[] values to NULL with your calloc, BANG! goes your code (trying to dereference a null pointer is undefined behaviour).

Go and have a look at this answer for a robust user input solution with prompting, buffer overflow protection, ignoring the remainder of too-long-lines and, most importantly here, stripping off the newline.

^(a) As an aside, you shouldn't be passing user input to printf as the format string anyway. Guess what happens if I enter %s%s%s%s%s%s at your prompt :-)

And, unrelated to your problem, ISO C mandates that sizeof(char) is always 1, so you don't need to use it in your allocation statements - I find it just clogs up the code unnecessarily.

edited May 23 '17 at 12:04

Community

1
1

answered Sep 24 '12 at 03:51

paxdiablo

854,327
234
1,573
1,953

I thought that was the point of my strtok part for command though was to get rid of all of the "white space". Maybe I'm just missing the point of strtok? I'm not sure. – Sep 24 '12 at 04:41
@Requiem: yes, except your strtok _doesn't_ get rid of white space. It tokenises the string based on _spaces._ If your input string is "ls\n", your strtok won't get rid of the newline. It will work on multi-word commands with spaces in them (well, up to the last, which will still have the newline on it) but not on something without spaces at all. If you want to strtok on spaces _and_ newlines, you need to provide them both in the separator string. – paxdiablo Sep 24 '12 at 04:47
So if I want to tokenize the command but also get rid of the new line issue I just have to do strtok(commandline, " \n"); ? – Sep 24 '12 at 04:51
It seems to have worked I'm just wondering if it will still let me tokenize or not? – Sep 24 '12 at 04:52
@Requium, I would just get rid of the newline straight after the `fgets` myself, but tokenising with both characters should also work. – paxdiablo Sep 24 '12 at 04:53
Alright, thank you so much for your help! Now it's time to get rid of this segfault! – Sep 24 '12 at 04:54

Programming a shell in C

1 Answers1