Char Arrays and Segmentation Fault

Question

New to C and pointers, and I can't figure out why I have a segmentation fault here...some of the code here is unused, I'm just trying to test whether or not my words are being read in properly and having the correct amount of space allocated. Eventually I'll have a char ** where the format when printed would look like "cat the", "new hey", .....
Words, in my code, represent each individual word

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

char *expand(char* source, int size);


char *expand(char *source, int size)
{
  char *expansion = (char *)malloc(size * sizeof(char));
  int i;
  for (i=0; i<size-1; i++)
    {
      expansion[i] = source[i];
    }
  free(source);
  return expansion;
}

int main(int argc, char **argv)
{
  int x;
  if (argc <= 2)
    {
      if (isdigit(*argv[1]))
        {
          x = atoi(argv[1]);
        }
    }
  else
    {
      fprintf(stderr, "Invalid Input\n");
      return 1;
    }


  //store pointers to each individual word
  char *words = (char *)malloc(3 * sizeof(char));
  int size = 3;//initial arbitrary size
  int count = 0;
  char *temp;
  while (1)
    {
      fscanf(stdin, "%s", temp);
      if (feof(stdin))
        {
          break; //break if end of file
        }
      if (count == size - 1)
        {
          size++;
          words = expand(words, size);
        }
      words[count] = *temp;
      count++;
    }

  int i;
  for(i=0; i<size-1; i++)
    {
      fprintf(stderr, "%s, ", words[i]);
    }
  fprintf(stderr, "\n");

  return 0;
}

Answer 1

Your immediate segmentation fault was due to:

char *temp;
...
fscanf(stdin, "%s", temp)

Where temp is an unallocated character pointer. When you declare a pointer, e.g. char *temp; the pointer does not point to anything. It is just an empty variable. For a pointer to function as you want temp to work, it must hold the address to an adequately sized block of memory as its value.

Think of it this way. When you declare int i;, you don't expect i to hold any specific value until you assign a value to it. 'i' is uninitialized. A pointer is no different. You wouldn't expect char *temp; to point to anything until you assign a valid address to it. When you call fscanf and attempt to store the input at the address pointed to by temp - boom, segfault because there is no telling where in the heck temp is pointing. It certainly isn't to an adequately sized allocated block of memory.

Beyond the immediate problem, it was difficult to follow the logic of your code as it was quite jumbled in reasoning. Guessing, it appears your goal was to pass the value for x from the command line, and then ultimately use that value for size to expand 'size' number of words read from stdin. Instead, it looks like you punted and just assigned size = 3; and decided to try and get it to work for 3-words. No need to give up, doing it correctly doesn't take any more effort.

When you convert the number entered as a string as argv[1] to an integer, you can use that value to create a variable length array of that many pointers. There isn't any compelling reason to declare some arbitrary number of pointers, as input you have taken presumably tells you how many words you wish to expand. There is also no need to allocate storage for the pointers at this point, as that appears to be the purpose of expand.

You are reading words from stdin. As long as they are normal words, you can simply use a statically declared buffer to hold the words read from stdin. As long as they are words from a dictionary, you know they will be no longer than 28-characters. So you will need no more than 29-characters (+1 for the nul-terminating character) to hold each word as your read it.[1]

The scanf family of functions returns the number of successful conversions based on the given format-string. You do not use feof to look for the end of input, you check the return of fscanf and limit the number of words you handle to size. To clean up the way you are handling your input (I have used size for your x), you can do something similar to the following:

enum { MAXC = 32 };  /* constant for static buffer - longest word 28 char */
...
int main (int argc, char **argv) {
...
    int size;  /* validate numeric input */
    if ((size = isdigit (*argv[1]) ? atoi (argv[1]) : 0) <= 0) {
        fprintf (stderr, "error: invalid input. usage %s int > 0\n", argv[0]);
        return 1;
    }
    ...
    int count = 0, i = 0;
    char *words[size];          /* array of 'size' pointers to char */
    char temp[MAXC] = {0};      /* buffer to hold each word input   */

    /* read at most 'size' words from stdin */
    while (size-- && fscanf (stdin, "%s", temp) == 1)
        words[count++] = expand (temp, strlen (temp));  /* expand  */

Look over each part of the declaration and read loop. (understand the use of the ternary operator when setting size -- it is a helpful shortcut in many situations) Note how you have 2 conditions for the read size > 0 and fscanf (stdin, "%s", temp) == 1. If you read size or there is no additional input to read, the loop terminates.

The remainder of the cleanup is fairly straight forward. Note however, that there is no need to use the variadic fprintf simply to print a newline to stderr (e.g. fprintf (stderr, "\n");). Just print the single character (e.g. fputc ('\n', stderr);).

Also, any time you allocate memory, it is up to you to (1) preserve a pointer to the beginning of the memory block so it can be (2) freed when no longer needed. Always, Always use a memory/error checking program like valgrind on Linux to verify you are using the memory correctly and that all blocks have been freed when no longer needed. There are similar programs for every OS and they are simple to use. There is no excuse not to.

Putting all the pieces together, you could do something like:

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

enum { MAXC = 32 };  /* constant for static buffer - longest word 28 char */

char *expand (char *source, size_t size);

int main (int argc, char **argv) {

    if (argc != 2) {    /* validate number of arguments */
        fprintf (stderr, "error: insufficient input.\n");
        return 1;
    }

    int size;  /* validate numeric input */
    if ((size = isdigit (*argv[1]) ? atoi (argv[1]) : 0) <= 0) {
        fprintf (stderr, "error: invalid input. usage %s int > 0\n", argv[0]);
        return 1;
    }

    int count = 0, i = 0;
    char *words[size];          /* array of 'size' pointers to char */
    char temp[MAXC] = {0};      /* buffer to hold each word input   */

    /* read at most 'size' words from stdin */
    while (size-- && fscanf (stdin, "%s", temp) == 1)
        words[count++] = expand (temp, strlen (temp));  /* expand  */

    for (i = 0; i < count; i++) {       /* output each string read */
        char *fmt = i ? ", %s" : "%s";
        fprintf (stderr, fmt, words[i]);
    }
    fputc ('\n', stderr);

    for (i = 0; i < count; i++)         /* free allocated memory   */
        free (words[i]);

    return 0;
}

char *expand (char *source, size_t size)
{
    char *expansion = calloc (1, size * sizeof *expansion + 1);
    size_t i;

    if (!expansion) {   /* validate memory allocation */
        fprintf (stderr, "error: virtual memory exhausted.\n");
        exit (EXIT_FAILURE);
    }

    for (i = 0; i < size; i++)
        expansion[i] = source[i];

    return expansion;
}

(note: calloc was used above to avoid a quirk in some versions of valgrind. Depending on the version it may complain about the uninitialized use of expansion in the if(..) statement. This isn't an error, but a quirk in older versions. To make sure you didn't run into that issue, I used calloc instead of malloc which will initialize all new memory to zero and avoid the warning. note: it also insures nul-termination of expansion without an explicit expansion[size] = 0; following the loop.)

Example Input

$ cat ../dat/captnjack.txt
This is a tale
Of Captain Jack Sparrow
A Pirate So Brave
On the Seven Seas.

Output

$ ./bin/expansion 5 < ../dat/captnjack.txt
This, is, a, tale, Of

$ ./bin/expansion 4 < ../dat/captnjack.txt
This, is, a, tale

Memory/Error Check

$ valgrind ./bin/expansion 5 < ../dat/captnjack.txt
==12849== Memcheck, a memory error detector
==12849== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==12849== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==12849== Command: ./bin/expansion 5
==12849==
This, is, a, tale, Of
==12849==
==12849== HEAP SUMMARY:
==12849==     in use at exit: 0 bytes in 0 blocks
==12849==   total heap usage: 5 allocs, 5 frees, 18 bytes allocated
==12849==
==12849== All heap blocks were freed -- no leaks are possible
==12849==
==12849== For counts of detected and suppressed errors, rerun with: -v
==12849== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 2)

Let me know if you have any questions. I tried to guess as well as I could where you were going with your code. If I missed the mark, just let me know and I'm happy to help further.

footnote 1. - 'Antidisestablishmentarianism' is the longest word in the unabridged dictionary.

Answer 2

A major reason for your troubles that jumps right out is that you declare char *temp; and then, within your while loop, begin trying to read data into the memory pointed to by the pointer... However, you never bothered to actually allocate memory and point the pointer at it.

Here is a far from ideal example that you should use to modify both the char *words and char *temp. I would suggest that, until you are comfortable with how to properly manage dynamic memory (malloc/free), start with static declarations:

char words[512];
int size = 3;
int count = 0;
char temp[512];
words[0] = 0;
temp [0] = 0;
while...

While "magic number" declarations like this are clearly not wise or safe, they create a starting point using a number (like 512) that is likely larger than any line based input you might pass while testing. Once you have this working correctly, then come back and look at malloc again!

Consider this example of allocating memory for a theoretical argument passed in argv[1]:

char *ptr = NULL;
ptr = malloc(strlen(argv[1] + 1);
strcpy(ptr, argv[1]);

... Do lots of stuff ...
free(ptr);

Note that the allocation is for one greater than the string length required to allow room for the null terminator for the string.

Also of possible interest to you would be the strdup function which will perform the proper allocation for you, though you must still remember to free the memory when you're through.