3

My assignment is to read words from a text file and store them in character arrays which are stored in an array of char*. All memory in these arrays needs to be dynamically allocated.

What I am doing is reading in each word with fscanf() and storing it into the variable str. I am then calculating the length of the word in str and dynamically allocating memory to store the value of str in the character array new_word. new_word is then inserted into the array of char* named words. When words runs out of space, I double its size and continue.

My problem lies in the commented code starting on line 62. I'm going to need to read these words later from words, so I'm testing my ability to access the pointers and their values. I can index new_word fine (in the lines above), but when I then store new_word in words and try to read from words, I get the following error:

hw1.c:63:25: error: subscripted value is not an array, pointer, or vector while (*(words[count])[k] != '\0'){

on lines 63 and 64. I know it has something to do with dereferencing the pointer, but I have tried a bunch of variations with no success. How can I fix this?

Here is the code:

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


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

    if (argc != 3){
        fprintf(stderr, "Incorrect number of arguments\n");
        exit(1);
    }

    char* infile = argv[1];
    FILE* finp = fopen(infile, "r");
    if (finp == NULL){
        fprintf(stderr, "Unable to open input file\n");
        exit(1);
    }

    char* prefix = argv[2];

    int count = 0;
    int size = 20;
    char* words = calloc(size, sizeof(char));
    printf("Allocated initial array of 20 character pointers.\n");
    char* str = malloc(30*sizeof(char));

    while (fscanf(finp, "%s", str) == 1){

        if (count == size){
            words = realloc(words, 2 * size);
            size *= 2;
            printf("Reallocated array of %d character pointers.\n", size);
        }

        int i = 0;
        while (str[i] != '\0'){
            i++;
        }

        char* new_word = malloc((i+1)*sizeof(char));

        int j = 0;
        while (str[j] != '\0'){
            new_word[j] = str[j];
            j++;
        }

        new_word[j] = '\0';

        int k = 0;

        while (new_word[k] != '\0'){
            printf("%c", new_word[k]);
            k++;
        }

        printf("\n");

        words[count] = *new_word;

        /*k = 0;
        while (*(words[count])[k] != '\0'){
            printf("%c", *(words[count])[k]);
            k++;
        }

        printf("\n");*/

        count++;

    }


}
10
  • 1
    int i = 0; while (str[i] != '\0'){ i++; } Ever heard of strlen? Do not reinvent the wheel. Commented Sep 8, 2015 at 19:25
  • 1
    Suggest char* words should be char* words[] and various code changes to support that. Commented Sep 8, 2015 at 19:26
  • int j = 0; while (str[j] != '\0'){ new_word[j] = str[j]; j++;}. Use strcpy or the secure version of it strncpy. Commented Sep 8, 2015 at 19:36
  • Since new_word is dynamically allocated, anyway, just use strdup, that takes care of both the malloc and strcpy in one instruction. Commented Sep 8, 2015 at 19:40
  • 1
    @ColeCameron strdup is not part of C, which can be a viable reason to avoid it. Commented Sep 8, 2015 at 19:42

2 Answers 2

5

Ok, dissecting that a bit:

char* words = calloc(size, sizeof(char));

this should probably read:

char **words = calloc(size, sizeof(char *));

Why? What you want here is a pointer to an array of pointers to char ... words points to the first char *, which points to your first "string".


char* str = malloc(30*sizeof(char));

while (fscanf(finp, "%s", str) == 1){

Buffer overflow here. Make sure to read at maximum 30 characters if you define your buffer not to hold more. Btw, just for convention, call your buffer buffer or buf (not str) and there's really no need to dynamically allocate it. Hint: Use a field size for fscanf() or, even better, some other function like fgets().


    if (count == size){
        words = realloc(words, 2 * size);
        size *= 2;
        printf("Reallocated array of %d character pointers.\n", size);
    }

The realloc here will not work, should read

        words = realloc(words, 2 * size * sizeof(char *));

You need to multiply the size of a single element, which, in this case, is a pointer to char.


No guarantee this will be all errors, but probably the most important ones. On a sidenote, strlen() and strncpy() will help you stop writing unnecessary code.

Sign up to request clarification or add additional context in comments.

7 Comments

This is perfect. Thanks! I'm getting the output I need now. However, I'm confused by the realloc() correction that you suggested. I tried both my original statement and your corrected one, and I'm getting identical output. I guess I just want to know what the difference is and if there is a way to check how much memory has been allocated for words (I tried to use sizeof(words) and I just get 8 (the size of a char*)).
Please note that this isn't technically portable, because NULL is not guaranteed to be represented by all-zeroes in memory (e.g., it might be 0xFFFFFFFF or 0xDEADBEEF or anything else). Although it's usually not a problem in practice, for max portability, initialize the array manually.
Assuming a typical 32bit architecture, sizeof(char *) will be 4 because a pointer needs 4 bytes. Say size is still 20 and you're hitting the 21st string, then your original line will change the size of the memory words points to to 40 bytes. Correct would be 160 bytes.
This will often not lead directly to observable problems, but you will corrupt the structure of your heap, leading to hard-to-debug crashes later (because you write outside the allocated region). There are tools for debugging such things, for example "valgrind".
@MattFenlon in general, you don't want to know, that's what sizeof() is for. (on amd64, sizeof(char *) would be 8). What you want to know is how many elements your dynamically allocated memory is able to hold, and you already know that, doing the right thing by keeping track of it in your size variable. For malloc() and friends, you just multiply that by the element size you get from sizeof().
|
1

A pointer to "A [dynamically-allocated] array of char*" would need to be recorded in a variable of type char **. That is, a pointer to the first element of the array, which element is of type char *. Thus ...

char **words;

If you want to have sufficient space for size words, then you could allocate it as ...

words = calloc(size, sizeof(char *));

(note the difference from your code), though it's harder to make a mistake with this form:

words = calloc(size, sizeof(*words));

Note in that case that the sizeof operator does not evaluate its operand, so it does not matter that words is not yet allocated.

Most importantly, be aware that the elements of array words are themselves pointers, not the ultimately pointed-to strings. Thus you assign a new word to the array by

words[count] = new_word;

(Again, note the difference from your version.) Other adjustments are needed as well.

The problematic while loop, though, is not fixed even then. Remember that the expression pointer[index] is equivalent to *((pointer) + (index)), so the expression *(words[count])[k] attempts to triply derference words. Even with the type correction, you want only to doubly dereference it: words[count][k].

But why re-invent the wheel? As Olaf observed with respect to strlen() and some of your earlier code, C already has perfectly good functions in its standard library for dealing with strings. In this case ...

printf("%s", words[count]);

... would be so much simpler than that while loop.

3 Comments

good addition for spotting the *(words[count])[k], too ... this one escaped me in my review :)
I guess I've just been hesitant to use built in C functions because of these dynamic allocation constraints. I don't want to use static memory when I shouldn't be which is leading to these over-complications. Thanks for the help!
@MattFenlon, the standard library functions work for dynamically-allocated strings exactly as they do for statically allocated ones (including their reliance on string termination). In fact, they cannot tell the difference.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.