0

I am trying to copy some of the arguments passed in argv to an array of strings. Here is my program.

#include <string.h>
#include <stdlib.h>
#include <unistd.h>

int main (int argc, char * argv[]) {
    char **args = malloc(argc * sizeof(char *));
    for (int i = 0; i < argc - 1; ++i) {
        strcpy(*(args+i), argv[i+1]);
    }
}

I am getting a segmentation fault in the for loop. Why is this happening?

pgp
  • 83
  • 6
  • You are only allocating an array of pointers. You need to also allocate every item in the array. – Victor Jan 25 '20 at 07:18

3 Answers3

5

You get the segmentation fault because you are using uninitialized pointers.

In other words: *(args+i) is uninitialized.

Let's look at your memory:

char **args = malloc(argc * sizeof(char *));

This will give a local variable args that points to a dynamic allocated memory area consisting of argc pointers to char. Looks like this:

enter image description here

But the argc pointers to char are uninitialized (i.e. malloc doesn't initialized anything) so the real picture is:

enter image description here

That is - the argc pointers to char may point anywhere.

So when you do

strcpy(*(args+i), argv[i+1]);
       ^^^^^^^^^
       Read uninitialized pointer

you read the i'th uninitialized pointer and copy the string at argv[i+1] to that location. In other words - to a location that we can't know where is and most like doesn't belong to your program. This is likely to result in a seg fault.

So before you copy anything you want those char-pointers to point to some chars. Like:

enter image description here

So basically you need to do additional malloc.

Now one problem is: How many chars do you need to malloc?

Well, you can't know until you know the length of the input strings. So you need to put the malloc inside the loop. Like:

int main (int argc, char * argv[]) {
    char **args = malloc(argc * sizeof(char *));
    for (int i = 0; i < argc - 1; ++i) {
        *(args+i) = malloc(strlen(argv[i+1]) + 1);  // Allocate memory
        strcpy(*(args+i), argv[i+1]);
    }
}

Note:

  • You don't need to write sizeof(char) as it is always 1

  • You have to add 1 to the string length of the input string in order to reserve memory for the string termination.

  • You should always check that malloc doesn't return NULL

  • Instead of *(args+i) you can use the more readable form args[i]

So after a real execution the picture could be:

enter image description here

Support Ukraine
  • 42,271
  • 4
  • 38
  • 63
1

The problem lies in this part of your code:

malloc(argc * sizeof(char *));

Don't think that sizeof(char*) would give you the size of a string.

What is the size of a pointer?

Solution:

int main(int argc, char* argv[])

{
    char* copyOfArgv;

    copyOfArgv = strdup(argv[1]);
}

strdup() - what does it do in C?

Ardent Coder
  • 3,777
  • 9
  • 27
  • 53
0

The problem is you allocated memory for the pointers not for the string (arguments from argv itself) hence the strcpy is invalid - causing segmentation fault.

It would've been better to just:

Allocate memory for the pointers (as you have done) then make those pointers point to the arguments passed. Example:

 #include <string.h>
 #include <stdlib.h>
 #include <unistd.h>

 int main (int argc, char * argv[]) {
     char **args = malloc(argc*sizeof(char *)); //allocated memory for the pointers only
     int i=0;

     for(;i<argc;i++)
     {
         args[i]=argv[i];  //making the pointers POINT to the arguments including the 0th argument as well
         i++;
     }
     return 0;
 }

For using strcpy you should've allocated memory for the arguments itself and then copied those.

Agrudge Amicus
  • 1,033
  • 7
  • 19
  • While this is correct code wise, I don't see the point. If a program doesn't need a real copy of the strings, why make a copy of the pointers? In thus cases the program could simply use `argv` and not copy anything at all. – Support Ukraine Jan 25 '20 at 07:27