Basic shell in C

Question

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);
 } 
}

• Mathieu Guindon: 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!

Answer 1

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.