Stack smashing detected when allocating array C [closed]

Question

As the title explains the problem. In this function in my program , whenever I allocate a certain size in the array I get huge chunks of error code on my terminal. The array gets copied properly and everything but right after printing it the program crashes. The purpose of the program is to read from file and then store each line in an array index. Your help is much appreciated. Thanks

The array is declared as a pointer in the main and then it is dynamically allocated inside.

    void read_file_into_array(char **filearray, FILE * fp)
        {
            /* use get line to count # of lines */
            int numlines = -1;
            numlines = linecount(fp);
            size_t size = 50;
            char *buffer;

            buffer = (char *)malloc(size * sizeof(char));
            if (buffer == NULL) {
                perror("Unable to allocate buffer");
                exit(1);
            }

            if (numlines < 0) {
                fprintf(stderr, "error: unable to determine file length.\n");
                return;
            }

            printf(" number of lines counted are ; %d\n", numlines);

            /* allocate array of size numlines + 1 */

            if (!(*filearray = (char *)malloc(numlines + 1 * sizeof(char)))) {
                fprintf(stderr, "error: virtual memory exhausted.\n");
                return;
            }

            fseek(fp, 0, SEEK_SET);
            for (int i = 0; i < numlines; i++) {
                if (!feof(fp)) {
                    fgets(buffer, size, fp);
                    filearray[i] = (char *)malloc(size * sizeof(char *));
                    strcpy(filearray[i], buffer);
                    printf("buffer at %d : %s\n", i, buffer);
                    printf("array at %d : %s\n", i, filearray[i]);
                }
            }
            free(buffer);
        }



        /* THIS IS MY MAIN BELOW */

        int main (int argc, char **argv)

        {
        FILE *fp = NULL;
        char*  array;

        /* open file for reading (default stdin) */
            fp = argc > 1 ? fopen (argv[1], "r") : stdin;

            if (!fp) {  /* validate file open */
                fprintf (stderr, "error: file open failed '%s'\n", argv[1]);
                return 1;
            }

            read_file_into_array (&array, fp);

        if (fp != stdout) 
                if (fclose (fp) == EOF) {
                    fprintf (stderr, "error: fclose() returned EOF\n");
                    return 1;
                }


        return 0;
        }


    /* MY LNE COUNT FUNCITON */
    int linecount(FILE *fp) {
    char buff[MAXLINE];
    int count = 0;

    while(fgets(buff,MAXLINE,fp) != NULL) {
    count++;
    }
    return count;
}

Answer 1

OK, let's get you sorted out. First, to pass a pointer to pointer to char as a parameter to a function, allocate within the function and have the results available back in the calling function (without more), you must pass the address of the pointer to pointer to char. Otherwise, the function receives a copy and any changes made in the function are not available back in the caller (main here) because the copy is lost (the address to the start of the allocated block is lost) when the function returns.

Passing the address of the pointer to pointer to char means you have to become a 3-STAR Programmer (it is not a compliment). A better way is to not pass the pointer at all, simply allocate within the function and return a pointer to the caller (but there are some cases where you will still need to pass it as a parameter, so that is shown below)

The program below will read any text file passed as the first argument (or it will read from stdin if no argument is given). It will dynamically allocate pointers and needed (in MAXL blocks rather than one-at-a-time which is highly inefficient). It will allocate (and reallocate) lines as required to accommodate lines of any length. It uses a fixed buffer of MAXC chars to read continually until a complete line is read saving the offset to the position in the current line to append subsequent reads as required.

Finally after reading the file, it will print the text and associated line number to stdout, and free all memory allocated before exiting. Look it over and let me know if you have any questions:

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

enum { MAXL = 32, MAXC = 1024 };

char **read_file_into_buf (char ***buf, FILE *fp, int *nlines);

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

    char **buf = NULL;  /* buffer to hold lines of file */
    int n;              /* number of lines read */
    FILE *fp = argc > 1 ? fopen (argv[1], "r") : stdin;

    if (!fp) {  /* validate file open for reading */
        fprintf (stderr, "error: file open failed '%s'.\n", argv[1]);
        return 1;
    }
    
    if (read_file_into_buf (&buf, fp, &n)) {        /* read file into buf  */
        for (int i = 0; i < n; i++) {               /* loop over all lines */
            printf ("line[%3d]: %s\n", i, buf[i]);  /* output line */
            free (buf[i]);      /* free line */
        }
        free (buf);             /* free buffer */
    }

    if (fp != stdin) fclose (fp);     /* close file if not stdin */

    return 0;
}

/** read text file from 'fp' into 'buf' update 'nlines'.
 *  Being a 3-STAR Programmer, is NOT a compliment. However,
 *  to pass a pointer to pointer to char as a parameter and 
 *  allocate within the function, it is required. You must
 *  pass the address of buf from main otherwise the function
 *  recieves a copy whose allocation is lost on return. A better
 *  approach is to simply assign the return in 'main' and not 
 *  pass buf at all.
 */
char **read_file_into_buf (char ***buf, FILE *fp, int *nlines)
{
    /* current allocation, current index, and offset (if less 
     * than a whole line is read into line on fgets call), and
     * eol indicates '\n' present.
     */
    size_t n = MAXL, idx = 0, offset = 0, eol = 0;
    char line[MAXC] = "";   /* temp buffer for MAXC chars */
    void *tmp = NULL;       /* pointer for realloc */
    
    /* validate address, file ptr & nlines address */
    if (!buf || !fp || !nlines) {
        fprintf (stderr, "error; invalid parameter.\n");
        return NULL;
    }
    
    /* allocate initial MAXL pointers, calloc used to avoid valgrind
     * warning about basing a conditional jump on uninitialized value.
     * calloc allocates and initializes.
     */
    if (!(*buf = calloc (sizeof *buf, MAXL))) {
        fprintf (stderr, "error: virtual memory exhausted.\n");
        return NULL;
    }
    
    while (fgets (line, MAXC, fp))  /* read every line in file */
    {
        /* save offset from prior read (if any), get len */
        size_t end = offset, len = strlen (line);
    
        if (len && line[len - 1] == '\n') { /* test for new line */
            line[--len] = 0;                /* overwrite with nul */
            offset = 0;                     /* zero offset, all read */
            eol = 1;                        /* POSIX eol present */
        }
        else {
            line[len] = 0;  /* nul-terminate */
            offset += len;  /* short read, save offset to last char */
            eol = 0;        /* no POSIX eol */
        }
        
        /* allocate/reallocate for current line + nul-byte */
        tmp = realloc ((*buf)[idx], sizeof ***buf * (end + len + 1));
        if (!tmp) {
            fprintf (stderr, "error: realloc, memory exhausted.\n");
            return *buf;  /* return current buf */
        }
        (*buf)[idx] = tmp;  /* assign block to current index */
        strcpy ((*buf)[idx] + end, line);  /* copy line to block */
        
        if (!eol) continue;   /* chars remain in line, go read them */
        
        if (++idx == n) {   /* check pointer allocation, realloc as needed */
            tmp = realloc (*buf, sizeof **buf * (n + MAXL));
            if (!tmp) {
                fprintf (stderr, "error: realloc buf, memory exhausted.\n");
                return *buf;
            }
            *buf = tmp; /* assign new block to *buf */
            memset (*buf + n, 0, sizeof **buf * MAXL);  /* zero new memory */
            n += MAXL;  /* update the current number of ptrs allocated */
        }
        *nlines = idx;  /* update the number of lines read */
    }
    if (!eol) {         /* protect against file with no POSIX ending '\n' */
        idx++;          /* account for final line */
        *nlines = idx;  /* update nlines */
    }
    
    /* final realloc to size buf to exactly fit number of lines */
    tmp = realloc (*buf, sizeof **buf * (idx));
    if (!tmp)   /* if it fails, return current buf */
        return *buf;
    
    *buf = tmp; /* assign reallocated block to buf */
    
    return *buf;
}

Note: zeroing the new pointers allocated with memset (*buf + n, 0, sizeof **buf * MAXL); is optional, but it eliminates valgrind warnings about basing a conditional jump on an uninitialized value, just as with calloc/malloc of the initial pointers. There is no option to zero with realloc, so a separate call to memset is required.

Example Use/Output

$ ./bin/fgets_readfile < dat/damages.txt
line[  0]: Personal injury damage awards are unliquidated
line[  1]: and are not capable of certain measurement; thus, the
line[  2]: jury has broad discretion in assessing the amount of
line[  3]: damages in a personal injury case. Yet, at the same
line[  4]: time, a factual sufficiency review insures that the
line[  5]: evidence supports the jury's award; and, although
line[  6]: difficult, the law requires appellate courts to conduct
line[  7]: factual sufficiency reviews on damage awards in
line[  8]: personal injury cases. Thus, while a jury has latitude in
line[  9]: assessing intangible damages in personal injury cases,
line[ 10]: a jury's damage award does not escape the scrutiny of
line[ 11]: appellate review.
line[ 12]:
line[ 13]: Because Texas law applies no physical manifestation
line[ 14]: rule to restrict wrongful death recoveries, a
line[ 15]: trial court in a death case is prudent when it chooses
line[ 16]: to submit the issues of mental anguish and loss of
line[ 17]: society and companionship. While there is a
line[ 18]: presumption of mental anguish for the wrongful death
line[ 19]: beneficiary, the Texas Supreme Court has not indicated
line[ 20]: that reviewing courts should presume that the mental
line[ 21]: anguish is sufficient to support a large award. Testimony
line[ 22]: that proves the beneficiary suffered severe mental
line[ 23]: anguish or severe grief should be a significant and
line[ 24]: sometimes determining factor in a factual sufficiency
line[ 25]: analysis of large non-pecuniary damage awards.

In any code you write that dynamically allocates memory, you have 2 responsibilities regarding any block of memory allocated: (1) always preserve a pointer to the starting address for the block of memory so, (2) it can be freed when it is no longer needed.

It is imperative that you use a memory error checking program to insure you haven't written beyond/outside your allocated block of memory, attempted to read or base a jump on an uninitialized value and finally to confirm that you have freed all the memory you have allocated.

For Linux valgrind is the normal choice.

$ valgrind ./bin/fgets_readfile < dat/damages.txt
==5863== Memcheck, a memory error detector
==5863== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==5863== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==5863== Command: ./bin/fgets_readfile
==5863==
line[  0]: Personal injury damage awards are unliquidated
line[  1]: and are not capable of certain measurement; thus, the
line[  2]: jury has broad discretion in assessing the amount of
line[  3]: damages in a personal injury case. Yet, at the same
line[  4]: time, a factual sufficiency review insures that the
line[  5]: evidence supports the jury's award; and, although
line[  6]: difficult, the law requires appellate courts to conduct
line[  7]: factual sufficiency reviews on damage awards in
line[  8]: personal injury cases. Thus, while a jury has latitude in
line[  9]: assessing intangible damages in personal injury cases,
line[ 10]: a jury's damage award does not escape the scrutiny of
line[ 11]: appellate review.
line[ 12]:
line[ 13]: Because Texas law applies no physical manifestation
line[ 14]: rule to restrict wrongful death recoveries, a
line[ 15]: trial court in a death case is prudent when it chooses
line[ 16]: to submit the issues of mental anguish and loss of
line[ 17]: society and companionship. While there is a
line[ 18]: presumption of mental anguish for the wrongful death
line[ 19]: beneficiary, the Texas Supreme Court has not indicated
line[ 20]: that reviewing courts should presume that the mental
line[ 21]: anguish is sufficient to support a large award. Testimony
line[ 22]: that proves the beneficiary suffered severe mental
line[ 23]: anguish or severe grief should be a significant and
line[ 24]: sometimes determining factor in a factual sufficiency
line[ 25]: analysis of large non-pecuniary damage awards.
==5863==
==5863== HEAP SUMMARY:
==5863==     in use at exit: 0 bytes in 0 blocks
==5863==   total heap usage: 28 allocs, 28 frees, 1,733 bytes allocated
==5863==
==5863== All heap blocks were freed -- no leaks are possible
==5863==
==5863== For counts of detected and suppressed errors, rerun with: -v
==5863== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Always confirm All heap blocks were freed -- no leaks are possible and equally important ERROR SUMMARY: 0 errors from 0 contexts. (although note: some OS's do not provide adequate memeory exclusion files (the file that excludes system and OS memory from being reported as in use) which will cause valgrind to report that some memory has not yet been freed (despite the fact you have done your job and freed al blocks you allocoted and under your control.)

Answer 2

One of the main issues why you get compilation warnings (+pay attention to the comments which point out other different bugs in your code) is

1. if (!(*filearray = (char**)malloc(sizeof(char*) * (numlines + 1))))

*filearray is of type char* not char**; + there is no need to cast the value returned by malloc.

2. char **filearray is actually &array from the main() which is defined as char *array;

Now the following totally breaks the stability and leads to undefined behavior

filearray[i] = (char *)malloc(size * sizeof(char *));

this is equivalent to (&array)[i] = (char *)malloc(size * sizeof(char *)); from the main, this is totally broken. You are walking through the stack, starting from the location of the array from the main and just override everything with the values of char* that are returned by malloc.

If you want your array be array of strings, defined it as char **array, pass it by pointer &array, update the declaration and the usage of filearray accordingly.