Why won't this C function return strings?

Question

I've been messing with this really simple C function to return the contents of a text file as a string for quite a while now, but no matter what I do, it doesn't work.

#include<stdio.h>

char* getContents(char filename[]) {
    FILE *fp;
    char text[1000];
    fp = fopen(filename, "r");
    int i = 0;
    while (feof(fp)) {
        text[i] = fgetc(fp);
        i++;
    } text[i] = '\0';
    return text;
}

int main() {
    printf("%s", getContents("text.txt));
}

I've looked at several other Stack Overflow discussions with similar premises to no avail — pretty much no combinations of *s in different places makes this work, I always get either "function returns address of local variable" or "return makes pointer from integer without a cast" errors. How can I make this function return the string like it should?

Answer 1

The function getContents has several problems.

char * getContents(char filename[]) {
    FILE *fp;
    char text[1000];
    fp = fopen(filename, "r");
    int i = 0;
    while (feof(fp)) {
        text[i] = fgetc(fp);
        i++;
    } text[i] = '\0';
    return text;
}

The main problem is that you are trying to return pointer to the first element of the local array text with automatic storage duration that will not be alive after exiting the function.

So the pointer will be invalid.

You could for example define the array as having static storage duration

static char text[1000];

In this case the function may return pointer to the first element of the array like

    return text;

because the array will be alive after exiting the function.

But it is better to allocate dynamically memory for the array like

char *text = malloc( 1000 );

Another problem is that you should check whether the specified file was opened successfully.

The condition in this while loop

    while (feof(fp)) {
        text[i] = fgetc(fp);
        i++;
    } 

is not correct. End of the file can occur in this statement

        text[i] = fgetc(fp);

In this case you will write in the array the value of the expression (char)EOF.

Also you need to check whether you are not writing outside the allocated dynamically array.

The loop can look like

    size_t i = 0;

    for ( int c; i + 1 < 1000 && ( c = fgetc( fp ) ) != EOF; i++ )
    {
        text[i] = c;
    } 
    text[i] = '\0';

Also you should close the file before exiting the function

    fclose( fp );

The function can look for example the following way

char * getContents( const char filename[]) 
{
    enum { N = 1000 };
    char *text = NULL;

    FILE *fp = fopen( filename, "r" );

    if ( fp != NULL )
    {
        text = malloc( N * sizeof( char ) );
        
        if ( text == NULL )
        {
            fclose( fp );
        }
        else
        {
            size_t i = 0;
            for ( int c; i + 1 < N && ( c = fgetc( fp ) ) != EOF; i++ )
            {
                text[i] = c;
            } 
            text[i++] = '\0';
            
            fclose( fp );
            
            char *tmp = realloc( text, i * sizeof( char ) );
            
            if ( tmp != NULL ) text = tmp;
        }
    }
    
    return text;
}

If the file was not opened successfully of if the memory for the array was not allocated the function returns a null pointer.

Answer 2

Arrays of any type are not first class types and when passed to or from a function, they are passed by reference. There are a number of patterns to overcome this:

• Pass in a pointer to an array in the caller's scope - the most idiomatic, and deterministic method.
• Make the array text static - you can then return a pointer as you are already doing. This has re-entrancy issues and you don't have a pointer to unique string for each call. So if the function is called a second time the pointer returned by the first will point to content read by the second.
• Wrap the array in a struct and return the struct - this is involves a mem-copy and not particularly efficient in memory or time terms.
• Dynamically allocate the memory within the function and return a pointer - a recipe for memory leaks and non-deterministic performance, overall most often a bad idea.

I'd only recommend the first solution:

#include <stdio.h>

char* getContents( const char* filename, char* content, size_t maxlen ) 
{
    FILE* fp = fopen( filename, "r" ) ;
    
    // Write up to maxlen - 1 characters from file to `content`
    // then append nul terminator
    ...

    return content ;
}

int main() 
{
    char text[1000] = {0} ;
    printf("%s\n", getContents( "text.txt", 
                                text, 
                                sizeof(text) ) ) ;
}

Answer 3

There are multiple problems:

• your loop exits immediately: while (feof(fp)). Testing for end of file should be done by comparing the return value of fgetc() with EOF.
• you return the address of a local array with automatic storage: it is discarded as soon as the function returns. Make this array static or better allocate a string from the heap.
• you do not close the input file.

Here is a modified version:

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

char *getContents(char filename[]) {
    char text[1000];
    int i, c;
    FILE *fp = fopen(filename, "r");
    if (fp == NULL)
        return NULL;
    for (i = 0; i < sizeof(text) - 1 && ((c = fgetc(fp)) != EOF; i++) {
        text[i] = c;
    }
    text[i] = '\0';
    fclose(fp);
    return strdup(text);
}

int main() {
    char *p = getContents("text.txt");
    if (p != NULL) {
        printf("%s\n", p);
        free(p);
    }
    return 0;
}

Answer 4

You cannot return variable sized stuffs in C. You can allocate it on the heap or return a fixed size thing.

To get the fixed sized return past the compiler do

struct {
    char data[1000];
} text;

struct text getContents(char filename[]) {
    struct text text;

If you'd rather do dynamic, there's a perfectly good example on Stephen Newell's proposed dupe target.

Answer 5

A '"' missing in your printf:

int main() {
    printf("%s", getContents("text.txt"));
}