Weird error in C program

ar1 · 02-08-2005, 08:41 PM

Hi, I wrote the following code which creates a simple shell (like bash). It works fine, can take a command along with arguments and run it or give error if it doesn't exist. However, the strange problem is that it doesn;t execute anything the first time "enter" is hit!! But after that it works perfectly.

I couldn't find the bug, any help would be great. Thanks
---------------------------------------------------------------------------------------------

#include <string.h>
#include <errno.h>

#define TRUE 1

void type_prompt();
void *read_command(char *argv[]);
int n = 0;

int main(int argc, char *argv[], char *envp[])
{
char *command;
int status;
argv[0] = NULL;

while (TRUE) /* repeat forever */
{
type_prompt(); /* display prompt on screen */
read_command(argv); /* read input from terminal */

if (fork() != 0) /* fork off child process */
{
waitpid(-1, &status, 0); /* wait for child to exit */
}
else
{
if (execve(argv[0], argv, envp) == -1) /* execute command */
printf("Ouch, can't execute: %s\n", strerror(errno));
}
}
return 0;
}

void type_prompt()
{
printf("[arajgarh]$ ");
}

void *read_command(char *argv[])
{
n = 0;
char temp[256];

gets(temp);
argv[n++] = strtok (temp," ");

while (argv[n-1] != NULL)
argv[n++] = strtok (NULL, " ");
}

itsme86 · 02-09-2005, 10:08 AM

Have you tried any debugging? Maybe print what temp is after the gets() in read_command().

By the way, don't ever ever use gets(). Use fgets() instead.

ar1 · 02-09-2005, 11:00 AM

Yes, I tried debugging and temp gives the correct value even the first time. Yet the first time, as I said, the value of temp gets messed up when fork() is called. I do not know why this happens

itsme86 · 02-09-2005, 11:36 AM

Well, I don't think the way you're using argv is very reliable. Ideally, you'd create your own array of strings that you can store the user's input in.

ar1 · 02-09-2005, 11:43 AM

I tried using my own array, but it didn't help. Here's a sample output with ot without my own array. You can see what I mean:

----------------------------------------------------------------------------------------------------------------

[arajgarh@localhost ass1]$ ./q3
[arajgarh]$ /bin/ls
Ouch, can't execute: No such file or directory
[arajgarh]$ /bin/ls
debug_me debug_me.c listing9 listing9.c q3 q3b q3b.c q3.c q3.o simple-shell simple-shell.c simple-shell.o temp temp.c
[arajgarh]$ /bin/ls -a
. .. debug_me debug_me.c listing9 listing9.c q3 q3b q3b.c q3.c q3.o simple-shell simple-shell.c simple-shell.o temp temp.c
[arajgarh]$

----------------------------------------------------------------------------------------------------------------

Thanks

itsme86 · 02-09-2005, 12:30 PM

You should try getting your code to compile correctly (warning free):

Code:

itsme@dreams:~/C$ gcc -Wall shell.c -o shell
shell.c: In function `main':
shell.c:22: warning: implicit declaration of function `fork'
shell.c:24: warning: implicit declaration of function `waitpid'
shell.c:28: warning: implicit declaration of function `execve'
shell.c:29: warning: implicit declaration of function `printf'
shell.c:12: warning: unused variable `command'
shell.c: In function `read_command':
shell.c:43: parse error before `char'
shell.c:45: warning: implicit declaration of function `gets'
shell.c:45: `temp' undeclared (first use in this function)
shell.c:45: (Each undeclared identifier is reported only once
shell.c:45: for each function it appears in.)
shell.c:50: warning: control reaches end of non-void function

itsme86 · 02-09-2005, 01:16 PM

Here's a version of your code that works:

Code:

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

#define TRUE 1

void type_prompt();
void read_command(char ***args);

int main(int argc, char *argv[])
{
  char **args;
  int i, status;

  while(TRUE) /* repeat forever */
  {
    type_prompt(); /* display prompt on screen */
    read_command(&args); /* read input from terminal */

    if(fork() != 0) /* fork off child process */
    {
      waitpid(-1, &status, 0); /* wait for child to exit */
    }
    else
    {
      if(execvp(args[0], args) == -1) /* execute command */
        printf("Ouch, can't execute '%s': %s\n",
          args[0], strerror(errno));
      for(i = 0;args[i];++i)
        free(args[i]);
      free(args);
    }
  }
  return 0;
}

void type_prompt(void)
{
  printf("[arajgarh]$ ");
  fflush(stdout);
}

void read_command(char ***args)
{
  char temp[256], *p;
  int n = 0;

  fgets(temp, sizeof(temp), stdin);
  temp[strlen(temp)-1] = '\0';

  *args = malloc(sizeof(char *));
  p = strtok(temp, " ");
  if(p)
  {
    (*args)[n] = malloc(strlen(p)+1);
    strcpy((*args)[n++], p);
  }
  else
    (*args)[n++] = NULL;

  while((*args)[n-1])
  {
    *args = realloc(*args, sizeof(char *) * n);
    p = strtok(NULL, " ");
    if(p)
    {
      (*args)[n] = malloc(strlen(p)+1);
      strcpy((*args)[n++], p);
    }
    else
      (*args)[n++] = NULL;
  }
}

Hko · 02-09-2005, 01:25 PM

Code:

void *read_command(char *argv[])
{
    n = 0;
    char temp[256];

    gets(temp);
    argv[n++] = strtok (temp," ");

    while (argv[n-1] != NULL)
    argv[n++] = strtok (NULL, " ");
}

What this function does is bad:
It assigns pointers to the local variable 'temp' to the argv 'owned' by main(). After this function returns, 'temp' doesn't exist anymore, while main() will have its argv[] point to 'temp'... Because local var's like 'temp' are on the stack you didn't get segementation fault crashes. It's more or less coincidence that your argv[] still has values that seem correct after calling read_command().

Even without the error above, I fully agree with itsme86's doubts about the way you are fiddling with argv[]. While it's not necessarily wrong to change argv-strings: e.g. the getopt() library function shuffles them, and some software change argv[0] which reflects in top, ps, etc. But: "argv[0] = NULL" looks very "experimental" to me. I'm not sure if its really wrong to do that, it's "unusual" to say the least. Also, imagine you start your program without any parameters on the command line, so argv[1]....argv[x] will be empty or don't even exist. What would happen if a user of your program enters a command with 1 or more arguments? Then argv[1]...argv[x] will be assinged those arguments, but do argv[1], argv[2] exist, e.g. have memory space allocated for all of them? I don't know, and maybe Linux does allocate space for some argv[]'s for each program even if they were not given on the command line that started the program. But even if Linux does this, how many?