CODEWITHSUNDEEP
cbashshellpointersfgets
gonidelis
gonidelis

Reputation: 1063

Read multiple lines from file using fgets() and store to array of pointers

I am trying to implement a custom shell on "batch-file mode". Let's say I have a file named "batchfile" containing:

ls –l
pwd
ps
touch hello
ls -l ; cat file ; grep foo file2
ls -l && cat file
quit

Calling ./myShell batchfile, I want to execute the commands separately. But when I try to read the lines from the file using fgets() and then store them to an array (char * batch_cmds[512]) I get :

Segmentation fault (core dumped)

This is my code so far:

int main(int argc, char *argv[]){
    if (argc >=2){
        char str[512];
        char *batch_cmds[512];
        int i=0; 
        FILE *fp;

        fp = fopen(argv[1], "r");

        while(fgets(str,512, fp)!=NULL){
            strcpy(batch_cmds[i], str);
            i++;
        }
    fclose(fp);

I can 't understand why this error pops.

Upvotes: 2

Views: 861

Answers (2)

Stephan Lechner
Stephan Lechner

Reputation: 35154

You have reserved space for the pointers themselves, but not for each string to which each such pointer shall point to. Each batch_cmds[i] points "somewhere", at least not to an object you allocated; when you then call strcpy(batch_cmds[i], str);, you write the contents of str to "somewhere" and produce undefined behaviour (e.g. a crash; actually statement batch_cmds[i] accesses an uninitialized array and is for itself already undefined behaviour; yet this alone seldomly leads to a crash).

Instead of

strcpy(batch_cmds[i], str);

write

batch_cmds[i] = strdup(str);

Command strdup does both - (1) reserving memory large enough to hold the contents of str and (2) copying the contents of str then. This is equivalent to batch_cmds[i] = malloc(strlen(str)+1); strcpy(batch_cmds[i], str));.

Additionally, check that i is <512, such that batch_cmds[i] will not exceed array bounds.

Upvotes: 4

Govind Parmar
Govind Parmar

Reputation: 21542

The line:

char *batch_cmds[512];

Declares an array of 512 pointers to char, but those pointers are uninitialized (wild, dangling, take your pick).

Your options:

  1. Declare a 2D array on the stack:

    char batch_cmds[512][512];

  2. Use dynamic memory management to allocate the strings on the heap:

    char *batch_cmds[512];
int i;
for(i = 0; i < 512; i++)
{
    batch_cmds[i] = malloc(string_length);
    if(batch_cmds[i] == NULL)
    {
         // handle alloc error
    }
}

// Code that uses batch_cmds...

for(i = 0; i < 512; i++)
{
    free(batch_cmds[i]);
    batch_cmds[i] = NULL;
}

  3. Use strdup to assign to the pointers to char in batch_cmds (demonstrated in Stephan Lechner's answer). Note that strdup also uses heap memory behind the scenes so the returned pointers must also be passed to free when you're done with them to avoid memory leaks.

Upvotes: 1

Related Questions