olafironfoot olafironfoot - 1 year ago 68
C Question

Segmentation fault when trying to declare an array of strings

In my program, I am trying to copy each argv[i] to keyword[i], but my program fails with a segmentation fault. What am I doing wrong?

#include <stdio.h>
#include <cs50.h>
#include <ctype.h>
#include <string.h>

int main(int argc, string argv[])
string keyword = "";
//int j;

for (int i = 0, n = strlen(argv[1]); i < n; i++)
keyword[i] = toupper(argv[1][i]);
printf("%i-- printing letters\n", keyword[i]);

Answer Source

As others have observed, you initialize variable keyword either as an empty string or as a pointer to an empty string literal, depending on the definition of type string. Either way, it is then valid to evaluate keyword[i] only for i equal to zero; any other value -- for read or write -- is out of bounds. Furthermore, in the latter (pointer to string literal) case, you must not attempt to modify the array keyword points to.

Note in particular that C does not automatically expand strings if you try to access an out of bounds element. Instead, an attempt to do so produces "undefined behavior", and a common way for that to manifest in such cases is in the form of a segmentation fault. You can view a segmentation fault as the system slapping down your program for attempting to access memory that does not belong to it.

Since you don't know a priori how long the argument string will be before you copy it, the most viable type for keyword is char *. I will use that type instead of string in what follows, for clarity.

If you indeed do want to make a copy of the argument, then by far the easiest way to do so is via the for-purpose function strdup():

    char *keyword = strdup(argv[1]);

That allocates enough memory for a copy of its argument, including the terminator, copies it, and returns a pointer to the result. You are then obligated to free the resulting memory via the free() function when you're done with it. Having made a copy in that way, you can then upcase each element in place:

    for (int i = 0, n = strlen(keyword); i < n; i++)
        keyword[i] = toupper(keyword[i]);
        printf("%c-- printing letters\n", keyword[i]);

Note, by the way, that the printf() format descriptor for a single character is %c, not %i. You must use that to print the characters as characters, rather than their integer values.

That's one of the simplest ways to write C code for what you're trying to do, though there are many variations. The only other one I'll offer for your consideration is to not copy the argument at all:

    char *keyword = argv[1];

If you initialize keyword that way then you do not allocate any memory or make a copy; instead, you set keyword to point to the same string that argv[1] points to. You can modify that string in-place (though you cannot lengthen it), provided that you do not need to retain its original contents.

Before I wrap this up, I should also observe that your program does not check whether there actually is an argument. In the event that there is not (i.e. argc < 2), argv[1] either contains a null pointer (argc == 1) or is undefined (argc == 0; you're unlikely ever to run into this). Either way, your program produces undefined behavior in that case if it attempts to use argv[1] as if it were a pointer to a valid string. You should test for this case first off, and terminate with a diagnostic message if no program argument is available.