0

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.

Improve this question
4
  • You might want to try the code review stack exchange. codereview.stackexchange.com Commented Sep 24, 2012 at 3:47
  • 1
    @OmnipotentEntity: No -- CodeReview is for code that works. Commented Sep 24, 2012 at 3:48
  • 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. Commented Sep 24, 2012 at 3:49
  • Been there. May I make a suggestion? Use a despatch table instead of all those else ifs, 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. Commented Sep 24, 2012 at 16:13

1 Answer 1

Reset to default
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.

Improve this answer
Sign up to request clarification or add additional context in comments.

6 Comments

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.
@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.
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"); ?
It seems to have worked I'm just wondering if it will still let me tokenize or not?
@Requium, I would just get rid of the newline straight after the fgets myself, but tokenising with both characters should also work.
|