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

void main(){
char *cmd;
pid_t pid;
while (1) {
printf("$ ");
fgets(cmd,1000,stdin);
if (pid = fork() == -1) {
exit(1);
}
else if (pid == 0){
execvp(cmd,&cmd);
}
else{
int status;
wait(&status);
}
}
}

I am making a simple shell that executes commands but when I enter the command at the prompt I keep getting segmentation fault. This is the most simple version that only works for one-argument commands like "ls"

Sourav Ghosh
  • 133,132
  • 16
  • 183
  • 261
Jimmy Samoladas
  • 19
  • 4
  • Welcome to Stack Overflow! It sounds like you may need to learn how to use a debugger to step through your code. With a good debugger, you can execute your program line by line and see where it is deviating from what you expect. This is an essential tool if you are going to do any programming. Further reading: [How to debug small programs](http://ericlippert.com/2014/03/05/how-to-debug-small-prgrams/). – Paul R Oct 30 '16 at 14:56
  • 1
    What made you decide to read in a maximum of `1000` `char`s? – alk Oct 30 '16 at 14:57
  • 1
    This `pid = fork() == -1` does not do what you assume it does (for details see here: http://en.cppreference.com/w/c/language/operator_precedence). – alk Oct 30 '16 at 14:59
  • well the 1000 maximum is partly experimental. When the program works properly I will work my way to a more generic way – Jimmy Samoladas Oct 30 '16 at 15:03

1 Answers1

2

For your problem, in your code,

 fgets(cmd,1000,stdin);

cmd is uninitialized. It does not point to a valid memory. Accessing invalid memory invokes undefined behavior.

You need to allocate memory to cmd before you ca use that. Alternatively, you can consider making cmd an array, like char cmd[1000] = {0}; to avoid the need to allocate memory yourself.

Then, execvp(cmd,&cmd); is not quite right, it's not what you think it is. Read the man page for a better understanding.

That said, for a hosted environment, void main() should at least be int main(void) to have standard conformance.

Sourav Ghosh
  • 133,132
  • 16
  • 183
  • 261
  • but I have the `char* cmd;` which initiliazes the pointer if I see this right – Jimmy Samoladas Oct 30 '16 at 14:56
  • @JimmySamoladas well, that defines the `cmd`, does not initialize it, let alone be a _valid_ one. – Sourav Ghosh Oct 30 '16 at 14:57
  • I tried the char array before but I get errors about the execvp arguments that asks for `const char*` and I give `const char*[1000]` – Jimmy Samoladas Oct 30 '16 at 14:59
  • @JimmySamoladas I don;t think you got the correct usage of `execvp()`, read the man page linked in my answer. – Sourav Ghosh Oct 30 '16 at 15:03
  • That's what puzzles me. I want this to run with no arguments just the command name so I don't know what to put in the second argument of `execvp` I have tried casting (const char*) NULL but that doesn't seem right – Jimmy Samoladas Oct 30 '16 at 15:07
  • @JimmySamoladas: If you type the bare command name into the line of input, you still need to remove the newline that `fgets()` includes with the input, and you need to provide an array of two `char *`, the second of which is a null pointer: `char *argv[2] = { cmd, 0 };` for example. – Jonathan Leffler Oct 30 '16 at 15:43