7
\$\begingroup\$

I'm new to C and created this shell to test my knowledge. It takes input from stdin, checks whether the full path has been given, if not, appends input to /bin/ and then executes. How can I improve; what are your suggestions?

#include<stdio.h>
#include<string.h>
#include<stdlib.h>
#include<unistd.h>
#include<sys/wait.h>

// Takes input from stdin 
char *read_line(char *buf, size_t sz){

    printf("Enter command: ");
    if ( fgets(buf, sz, stdin) == NULL ) {
        return NULL;
    }
    buf[strlen(buf) - 1] = '\0';
}

// Splits input from stdin
void split(char *buf, char **split[], size_t max){

    char* token = strtok(buf, " \n");
    int index = 0;

    while(token != NULL){
        split[index] = malloc(strlen(token) + 1);
        strcpy(split[index], token);
        index++;
        token = strtok(NULL, " \n");
    }
}


int main(){

    char userInput[50];
    size_t szUI = sizeof(userInput); 

    char *inputSplit[50];
    size_t szIS = sizeof(inputSplit); 
    memset(inputSplit, 0, szIS);

    if ( read_line(userInput, szUI) == NULL ) {
        printf("Error: CTRL-D ENTERED");
        return 0;
    }

    split(userInput, inputSplit, szIS);

    while(1){

        if ( fork() == 0 ) {

            // Checks if / is present 
            if ( strpbrk(inputSplit[0], "/") != 0 ) { 
                if ( execv(inputSplit[0], inputSplit) == -1 ) {
                    perror("Error");
                    return 0;
                }
            } 
            // Otherwise / is not present
            char tmp[20] = "/bin/";
            strcat(tmp, inputSplit[0]);

            if ( execv(tmp, inputSplit) == -1 ) {
                perror("Error");
                return 0;
            }

            memset(tmp, 0, sizeof(tmp));
            exit(0);
        } 

        wait(NULL);

        memset(userInput, 0, szUI);
        memset(inputSplit, 0, szIS);

        char cont[20];
        printf("Continue?(y/n) ");

        if ( fgets(cont, sizeof(cont), stdin) ==  NULL ) {
            return 0; 
        }

        cont[strlen(cont)-1] = '\0';

        if ( strcmp(cont, "y") != 0 ) {
            return 0;
        }

        memset(cont, 0, sizeof(cont));

        if ( read_line(userInput, szUI) == NULL ) {
            printf("Error: CTRL-D ENTERED \n");
            return 0;
        }

        split(userInput, inputSplit, szIS);
    }   
}
\$\endgroup\$
  • \$\begingroup\$ Hi there, please feel free to @ping me here if you would like to talk about what lead to revision 3 of this post. In the meantime please avoid self-vandalizing your posts and/or modifying/removing/adding code to answered questions. Cheers! \$\endgroup\$ – Mathieu Guindon Nov 15 '17 at 20:34
7
\$\begingroup\$

Avoid a hacker exploit:

What happens when the first character read is a null character?

if ( fgets(buf, sz, stdin) == NULL ) {
    return NULL;
}
buf[strlen(buf) - 1] = '\0';  // same as buf[SIZE_MAX] = '\0';

Instead, use

buf[strcspn(buf, "\n")] = '\0';

Expand white-space list to the standard white spaces or use isspace()

// char* token = strtok(buf, " \n");
char* token = strtok(buf, " \n\t\r\v\f");`

What is the point of max in void split(char *buf, char **split[], size_t max){? I'd expect some check in code and a matching type for i

// int index = 0;
// while(token != NULL){
size_t index = 0;
while(token != NULL && i < max) {

Potential over-run. Why the magic 20?

// char tmp[20] = "/bin/";
char tmp[strlen(inputSplit[0]) + 5 + 1] = "/bin/";  // still not elegant
strcat(tmp, inputSplit[0]);

Flush after output to insure stdout is emptied before reading user input

printf("Continue?(y/n) ");
fflush(stdout);  // add
if ( fgets(cont, sizeof(cont), stdin) ==  NULL ) {

Replace with read_line(char *buf, size_t sz) with something more generic

char *read_line(const char *prompt, char *buf, size_t sz) {
  if (prompt) {
    fputs(prompt, stdout);   
    flush(stdout);
  }
  if (fgets(buf, sz, stdin) == NULL) {
    return NULL;
  }
  buf[strcspn(buf, "\n\r")] = '\0';
  return buf;
}

And use it in various places. note () not need with sizeof object.

  // printf("Continue?(y/n) ");
  // if ( fgets(cont, sizeof(cont), stdin) ==  NULL ) { return 0;  }
  // cont[strlen(cont)-1] = '\0';
  if (read_line("Continue?(y/n) ", cont, sizeof cont) == NULL) {  
    return 0;
  }

Your use of a helper function to handle user text input is a very good idea.


String duplication begs for a strdup() function that also checks for allocation success.

split[index] = strdup(token);
if (split[index] == NULL) return EXIT_FAILURE;

On failure from main(), do not return 0, which implies success. Use EXIT_FAILURE. Send error messages to stderr

#include <stdlib.h>
...
   if ( read_line(userInput, szUI) == NULL ) {
        //printf("Error: CTRL-D ENTERED \n");
        //return 0;
        fprintf(stderr, "Error: CTRL-D ENTERED \n");
        return EXIT_FAILURE;

Consider allowing a case insensitive response

// if ( strcmp(cont, "y") != 0 ) {
if ( strcmp(cont, "y") != 0 strcmp(cont, "Y") != 0 ) {

Or even "yes", etc.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.