Executing a command from a file(shell)

Question

I'm trying to make a program that works as mini-shell(Which executes a command written in the terminal). To do so, I'm trying to test of this method works, I created a file where I put the arguments parsed in the terminal, read the file and then parse it to an array to execute using a system call such as execl or execvp..etc. The code I've written so far is this:

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

FILE* ouvrir(char *cmd)
{
    FILE *commandeLog = fopen("commande.txt", "w+");
    if (commandeLog == 0)
    {
        printf("Erreur de création du fichier");
    }
    else
    {
        fprintf(commandeLog,"%s", cmd);
    }
    fclose(commandeLog);
    return commandeLog;
}

char** afficher_fichier(FILE *f,char** argv)
{
    f=fopen("commande.txt", "r");
    if (f != NULL)
    {
        char ch;
        while ((ch=fgetc(f))!= EOF)
        {
            printf ("%c",ch);
            **argv=ch;
        }
        printf("\n");
    }
    return argv;
}

void  exec(char **argv)
{
     pid_t  pid;

     if ((pid=fork()) == 0)
     {
          if (execvp(*argv, argv) < 0) 
          {
               printf("*** ERREUR: exec echoué\n");
               exit(1);
          }
     }
     wait(NULL);
}

int  main()
{
    char  cmd[MAX];
    FILE* f=0;
    char** argv=0;
    printf("Entrer -> ");
    fgets(cmd,MAX,stdin);
    int i = strlen(cmd)-1;
    if( cmd[ i ] == '\n') 
        cmd[i] = '\0';
    printf("\n");
    f=ouvrir(cmd);
    afficher_fichier(f,argv);
    exec(argv);
    return 0;
}

Upon compiling the code and running, it returns a "Segmentation fault (core dumped)" message. The problem is basically in the "afficher_fichier" function.

Is the code I wrote has any logic in it?

If so, how do I correct the error?

If not, how do I reach the goal I seek?

Answer 1

First pass

Make the function ouvrir() return a FILE *; only ever pass FILE * around, never a FILE. A copy of the structure is not guaranteed to 'work' properly. (The C standard ISO/IEC 9899:2011 §7.21.3 Files says: The address of the FILE object used to control a stream may be significant; a copy of a FILE object need not serve in place of the original.) However, it is puzzling that you don't use the returned value.

FILE *ouvrir(char *cmd)
{
    FILE *commandeLog = fopen("commande.log", "a");
    if (commandeLog == 0)
    {
        …report error…
    }
    else
    {
        fprintf(commandeLog,"%s", cmd);
    }
    return commandeLog;
}

Then, in main():

FILE *f = 0;    // As now

…

f = ouvrir(cmd);
if (f == 0)
{
    …whatever you need to do that uses f…
    fclose(f);
}

If you're later going to read the file without reopening it, then you need to use "a+" or a similar mode ("w+", "r+" — it depends on whether the ouvrir() must create the file, or should use the existing file, and whether all write must be unconditionally at the end of the file). Alternatively, you can have ouvrir() unchanged and arrange to close the file and reopen it for reading (a new function, reouvrir(), perhaps).

Second pass

After the question was updated, the afficher_fichier() file causes most of the problem. It is currently:

char** afficher_fichier(FILE *f,char** argv)
{
    f=fopen("commande.txt", "r");
    if (f != NULL)
    {
        char ch;
        while ((ch=fgetc(f))!= EOF)
        {
            printf ("%c",ch);
            **argv=ch;
        }
        printf("\n");
    }
    return argv;
}

Issues include:

1. Why pass in f if the first thing you do is overwrite it with a new file stream with fopen(). It should just be a local variable, not an argument.
2. Why pass in argv when you are going to return it?
3. The passed in argv is a null pointer. There isn't much point in passing that; you could simply allocate the space in the function.
4. Why are you reading the file at all? You know it contains the value in cmd in main(), so why not simply pass that to afficher_fichier()?
5. If you pass cmd in, you have to decide whether it is OK for the function to mangle the string. If not, you need to make a copy of the arguments as you find them.
6. With the code as written, you simply copy all characters over the same character in the zeroth argument with no splitting (e.g. on spaces).
7. There's no checking for overflow, but since the memory wasn't allocated, there wasn't really a buffer to overflow, and since it always writes to the same place, maybe it doesn't matter.
8. You don't null terminate the string(s).
9. Your function is passed a null pointer; this does not lead to happiness.
10. Returning the null pointer isn't very helpful.
11. Because you open the file in the function but don't close it, you leak file streams.
12. …the list could probably continue, but is probably not productive…

The exec function is in decent shape. It would be better if the error message were printed to stderr instead of stdout, but otherwise, it'll do — assuming it gets a valid argv array of strings passed to it.

Making simplifying assumptions:

• cmd is passed to afficher_fichier()
• cmd can be modified
• spaces separate arguments
• the function doesn't need renaming — even though its current name is no longer appropriate

You might end up with code like:

#include <ctype.h>
#include <stdlib.h>
#include <stdio.h>

static
char **afficher_fichier(char *cmd)
{
    printf("-->> [%s]\n", cmd);
    size_t argx = 16;
    size_t argc = 0;
    char **argv = malloc(sizeof(*argv) * argx);
    if (argv == 0)
        return 0;

    char *src = cmd;
    while (*src != '\0')
    {
        while (isspace((unsigned char)*src))
            src++;
        if (*src == '\0')
        {
            printf("<<-- 1\n");
            if (argc == 0)
            {
                free(argv);
                argv = 0;
            }
            else
                argv[argc] = 0;
            return argv;
        }
        if (argc >= argx - 1)
        {
            size_t narg = argx * 2;
            char **argn = realloc(argv, sizeof(*argv) * narg);
            if (argn == 0)
            {
                fprintf(stderr, "Memory allocation failed\n");
                free(argv);
                return 0;
            }
            argv = argn;
            argx = narg;
        }
        argv[argc++] = src;
        while (!isspace((unsigned char)*src))
            src++;
        if (*src != '\0')
            *src++ = '\0';
        printf("%zu = [%s]\n", argc, argv[argc-1]);
    }
    *argv[argc] = 0;
    printf("<<-- 2\n");

    return argv;
}

int main(void)
{
    char cmd[] = "  \tcmd   arg1     arg2 arg3   ";
    char **argv = afficher_fichier(cmd);
    if (argv != 0)
    {
        char **arg = argv;
        while (*arg != 0)
            printf("[%s]\n", *arg++);
    }
    else
        printf("Bother!\n");
    return 0;
}

Example output:

-->> [      cmd   arg1     arg2 arg3   ]
1 = [cmd]
2 = [arg1]
3 = [arg2]
4 = [arg3]
<<-- 1
[cmd]
[arg1]
[arg2]
[arg3]

The printf() statements with double-headed arrows are diagnostic printing; they'd be omitted from production code (or made into code that prints conditionally if a debugging flag of some sort is set — see C #define macro for debug printing for more ideas on that).