Memory leak - How do I allocate memory for a Typdef Struct passed within another struct as thread arguments?

Question

I'm very new to C and am struggling with proper memory management and am coming across numerous seg faults with my program.

My ultimate goal here is to take a text file and number of threads entered as user arguments, get the file size, and then split the file based on number of threads entered by the user.

Each thread will then read a portion of the file, and then extract tokens from the chunk its reading. If the token is larger than 5 chars, it is to be added to an array along with the number of times that token has occurred in the entire text. So eventually I am hoping to get a list of top n words of > 5 used in the text.

However this is maybe my third time ever using C and I am struggling quite a bit and am getting quite a few errors. I've been trying to work through this myself using valgrind and other debugging tools, but I'm pretty lost.

From valgrind, I get the following messages:

==27634== 1 errors in context 1 of 5:
==27634== Conditional jump or move depends on uninitialised value(s)
==27634==    at 0x50B7B4C: vfprintf (vfprintf.c:1642)
==27634==    by 0x50BFF25: printf (printf.c:33)
==27634==    by 0x10919F: main (in /home/student/Desktop/hw2/main)
==27634== 
==27634== 
==27634== 1 errors in context 2 of 5:
==27634== Conditional jump or move depends on uninitialised value(s)
==27634==    at 0x50B7014: vfprintf (vfprintf.c:1642)
==27634==    by 0x50BFF25: printf (printf.c:33)
==27634==    by 0x10919F: main (in /home/student/Desktop/hw2/main)
==27634== 
==27634== 
==27634== 1 errors in context 3 of 5:
==27634== Conditional jump or move depends on uninitialised value(s)
==27634==    at 0x50B3875: _itoa_word (_itoa.c:179)
==27634==    by 0x50B6F0D: vfprintf (vfprintf.c:1642)
==27634==    by 0x50BFF25: printf (printf.c:33)
==27634==    by 0x10919F: main (in /home/student/Desktop/hw2/main)
==27634== 
==27634== 
==27634== 1 errors in context 4 of 5:
==27634== Use of uninitialised value of size 8
==27634==    at 0x50B386B: _itoa_word (_itoa.c:179)
==27634==    by 0x50B6F0D: vfprintf (vfprintf.c:1642)
==27634==    by 0x50BFF25: printf (printf.c:33)
==27634==    by 0x10919F: main (in /home/student/Desktop/hw2/main)
==27634== 
==27634== 
==27634== 1 errors in context 5 of 5:
==27634== Conditional jump or move depends on uninitialised value(s)
==27634==    at 0x50B78DA: vfprintf (vfprintf.c:1642)
==27634==    by 0x50BFF25: printf (printf.c:33)
==27634==    by 0x10919F: main (in /home/student/Desktop/hw2/main)
==27634== 
==27634== ERROR SUMMARY: 5 errors from 5 contexts (suppressed: 0 from 0)

I am also seeing the message that "Address xxx is 0 bytes after a block size 60 alloc'd"

I believe my issues lies within the line:

if (strcmp(words[i].word, token) == 0) {
  inArray = i;
}

Is this because I am not properly allocating memory for wordStruct words? I'm not sure how to fix it, any help would be appreciated.

Thanks

Full code below

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


typedef struct {
    char word[50];
    int count;
    int totalWords;
} wordsStruct ;



struct argStruct {
    FILE *file;
    int start;
    int end;
    int count;
    wordsStruct *words;
};



int stringLength(char s[]) {
    int c = 0;
    while (s[c] != '\0')
        c++;
    return c;
}



void groomString(char *line){

    for (int i = 0; line[i] != '\0'; ++i) {
        line[i] = tolower(line[i]);

        while (!((line[i] >= 'a' && line[i] <= 'z') || line[i] == '\0')) {


            for (int j = i; line[j] != '\0'; j++) {
                line[j] = line[j + 1];
                line[j] = '\0';
            }
        }
    }
}

int counter(int n){
    static int test;
    test = n;

    if(n = 0){
        return test;
    } else {
        n++;
        return n;
    }

}


void processFile(void *input) {
    struct argStruct params = *(struct argStruct *) input;

    wordsStruct *words = params.words;
    FILE *textFile = params.file;
    int start = params.start;
    int end = params.end;
    int count = params.count;

    int size = (end - start) + 10;
    char delim[] = " \t\v\n\r";


    char *readFile = (malloc(sizeof(char) * size +10));
    fread(readFile, 1, size, textFile);


    char *copy = (malloc(sizeof(char) * size +10));
    strcpy(copy, readFile);

    char *saveptr;

    int inArray;
    int length;
    static int added;

    char *token = strtok_r(copy, delim, &saveptr);

    while (token) {

        groomString(token);
        length = stringLength(token);

        if (length > 5) {

            inArray = 0;


            for (int i = 0; i < added; i++) {
                if (strcmp(words[i].word, token) == 0) {
                    inArray = i;
                }
            }

            if (inArray == 0) {
                added++;
                strcpy(words[added].word, token);
                words[added].count = 1;
            } else {
                words[inArray].count++;
            }
        }

        token = strtok_r(NULL, delim, &saveptr);
    }


     words->totalWords = added;

    free(token);
    free(readFile);
}




int main(int argc, char *argv[])
{
    FILE *pfile;
    int threadCount = 0, fileSize = 0, divide = 0;
    wordsStruct *allWords = (wordsStruct *) malloc( sizeof(wordsStruct));


    if (argc > 2)
    {
        pfile = fopen( argv[1], "r");
        if (pfile == NULL){
            perror("FILE OPEN FAILURE");
        }

        threadCount = atoi(argv[2]);

        pthread_t * thread = malloc(sizeof(pthread_t)* threadCount *10);

        fseek(pfile, 0, SEEK_END);
        fileSize= ftell(pfile);
        fseek(pfile, 0, SEEK_SET);


        divide = (fileSize/threadCount);

        struct argStruct arguments;
        arguments.file = pfile;
        arguments.words = allWords;

        int j = 0;
        for(int i = 0; i < threadCount; i++) {

            arguments.start = j;
            arguments.end = j+divide;

            arguments.count = i;

            struct argStruct *passArgs = malloc(sizeof *passArgs);
            *passArgs = arguments;

            pthread_create(&thread[i], NULL, (void *) processFile, passArgs);

            j+=divide;
        }


        for (int i = 0; i < threadCount +1; i++){
            pthread_join(thread[i], NULL);
        }

        fclose(pfile);

    } else {
        printf("Please enter text file name and number of threads");
    }

return 0;
}

Answer 1

For starters, on the last thread, you need [after setting end]:

if (i == (threadCount - 1)) arguments.end = fileSize;

to get all the bytes in the last segment and not go beyond EOF. This is needed whenever the file size is not an exact multiple of the thread count.

To prevent leaking memory for the argStruct passed to the thread, at the bottom of processFile, you need free(input)

Also, threads share certain resources (see man pthreads). Notably, open file descriptors. So, you'll need a mutex around your access to textFile. (e.g.) pthread_mutex_lock(&text_mutex); et. al.

And, each thread must do its own fseek to the part of the file that it is trying to access.

And, size has a "slop factor" of 10. This is fine as a safety for the allocated area, but it causes too much data to be read. Better to either leave off the + 10, or do: int exact_size = end - start;. Also, note that you're doing the "slop" when setting size and an additional amount in the malloc, so this isn't needed.

Also, note that fread does not guarantee an EOS char (0x00) at the end of the buffer the way fgets does. So, if you're going to do string operations on the buffer, you'll need to enforce this yourself (and you would need a "slop" of at least 1):

So, we need:

pthread_mutex_lock(&text_mutex);

fseek(textFile,start,0);
fread(readFile,1,exact_size,textFile);   
readFile[exact_size] = 0;

pthread_mutex_unlock(&text_mutex);

And, remember that main has to initialize text_mutex with pthread_mutex_init before doing the pthread_create.

But...

Using fread here may be problematic. When you segment the file in chunks of length divide, you are [probably] chopping up the file in such a way that the first thread has its last line truncated and the next thread will see the remainder of this line before seeing its first full line, and so on ...

You may be better off doing a single mmap on the entire file in main, and have it scan the buffer, looking for newline [or whitespace], and give each thread a segment that is guaranteed to align to newlines.

---

UPDATE:

I've coded up a version that uses mmap and fixes a small bug [please pardon the gratuitous style cleanup]. It compiles cleanly, even with -O2 -Wall [which you should always use to catch all warnings]. I've not tested it, but it should get you further.

#include <time.h>
#include <ctype.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <fcntl.h>
#include <unistd.h>
#include <pthread.h>
#include <sys/stat.h>
#include <sys/mman.h>

char *filebuf;

typedef struct {
    char word[50];
    int count;
    int totalWords;
} wordsStruct;

struct argStruct {
    pthread_t threadid;
    int start;
    int end;
    int count;
    wordsStruct *words;
};

int
stringLength(char s[])
{
    int c = 0;

    while (s[c] != '\0')
        c++;
    return c;
}

void
groomString(char *line)
{

    for (int i = 0; line[i] != '\0'; ++i) {
        line[i] = tolower(line[i]);

        while (!((line[i] >= 'a' && line[i] <= 'z') || line[i] == '\0')) {

            for (int j = i; line[j] != '\0'; j++) {
                line[j] = line[j + 1];
                line[j] = '\0';
            }
        }
    }
}

int
counter(int n)
{
    static int test;

    test = n;

    // NOTE/BUG: this is the assignment operator and you want equality
#if 0
    if (n = 0) {
#else
    if (n == 0) {
#endif
        return test;
    }
    else {
        n++;
        return n;
    }

}

void
processFile(void *input)
{
    struct argStruct *params = input;

    wordsStruct *words = params->words;

    int start = params->start;
    int end = params->end;
#if 0
    int count = params->count;
#endif

    int size = (end - start) + 10;
    char delim[] = " \t\v\n\r";

    char *copy = malloc(size + 1);
    memcpy(copy,&filebuf[start],size);
    copy[size] = 0;

    char *saveptr;

    int inArray;
    int length;
    static int added;

    char *token = strtok_r(copy, delim, &saveptr);

    while (token) {

        groomString(token);
        length = stringLength(token);

        if (length > 5) {

            inArray = 0;

            for (int i = 0; i < added; i++) {
                if (strcmp(words[i].word, token) == 0) {
                    inArray = i;
                }
            }

            if (inArray == 0) {
                added++;
                strcpy(words[added].word, token);
                words[added].count = 1;
            }
            else {
                words[inArray].count++;
            }
        }

        token = strtok_r(NULL, delim, &saveptr);
    }

    words->totalWords = added;

    free(copy);
    free(token);
}

int
main(int argc, char *argv[])
{
    int pfile;
    int threadCount = 0,
        fileSize = 0,
        divide = 0;
    struct stat st;
    off_t curpos;

    wordsStruct *allWords = (wordsStruct *) malloc(sizeof(wordsStruct));

    if (argc > 2) {
        pfile = open(argv[1],O_RDONLY);
        if (pfile < 0) {
            perror("FILE OPEN FAILURE");
        }

        threadCount = atoi(argv[2]);

        struct argStruct *threads =
            malloc(sizeof(struct argStruct) * threadCount);
        struct argStruct *arg;

        fstat(pfile,&st);
        fileSize = st.st_size;

        filebuf = mmap(NULL,fileSize,PROT_READ,MAP_PRIVATE,pfile,0);

        divide = (fileSize / threadCount);

#if 0
        int j = 0;
#endif

        for (int i = 0; i < threadCount; i++) {
            arg = &threads[i];

            arg->words = allWords;

            if (i == 0)
                arg->start = 0;
            else
                arg->start = arg[-1].end;

            curpos = arg->start + divide;

            for (;  curpos < fileSize;  ++curpos) {
                if (filebuf[curpos] == '\n') {
                    ++curpos;
                    break;
                }
            }

            if (curpos > fileSize)
                curpos = fileSize;

            arg->end = curpos;
            arg->count = i;
        }

        for (int i = 0; i < threadCount; i++) {
            arg = &threads[i];
            pthread_create(&arg->threadid, NULL, (void *) processFile, arg);
        }

        for (int i = 0; i < threadCount + 1; i++) {
            arg = &threads[i];
            pthread_join(arg->threadid, NULL);
        }

        munmap(filebuf,fileSize);
        close(pfile);

        free(threads);
    }
    else {
        printf("Please enter text file name and number of threads");
    }

    return 0;
}

---

UPDATE #2:

Oops, I missed a few things ...

Because added is defined with static, then all threads try to use it. They race. This would be "bad". And, the indexes will be wrong for the params->words list.

Access to this would would require either a mutex lock/unlock pair at the top/bottom of the thread loop or use of atomic primitives (e.g. stdatomic.h).

But, the mutex pairs would have the effect that whichever thread first got the mutex would "monopolize" it and all the threads would run, more or less, sequentially. Thus defeating the purpose of having multiple threads.

So, first, we want to remove the static so that each thread has its own copy.

But, now, it turns out that words (i.e. params->words) doesn't "grow" when we add a new word to the list. So, we have to grow the list size whenever we add a new word, so we'll need to add a realloc call.

Doing this on a single common list (e.g. where you allocate allWords in main) is problematic. Because of the searching and realloc, the "critical section" of the code that has to be mutex guarded is pretty much the entire body of the loop.

So, one solution is to have each thread maintain it's own per-thread list (i.e. params->words is different for each thread). Then, the threads do not race and don't require any mutex locking while running.

However, this will mean that there are duplicates between threads.

So, after main does pthread_join on all the threads, main has to recreate a single, unified list that eliminates duplicates.

Using arrays, this is more cumbersome. A linked list might make things easier to recombine the various lists.

The easiest way is to copy all entries from each per-thread list, appending to a big list.

Then, sort this list.

Then, create a new list that eliminates the duplicates.

Here's an updated version that fixes these issues [again, untested]:

#include <time.h>
#include <ctype.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <fcntl.h>
#include <unistd.h>
#include <pthread.h>
#include <sys/stat.h>
#include <sys/mman.h>

char *filebuf;

typedef struct {
    char word[50];
    int count;
#if 0
    int totalWords;
#endif
} wordsStruct;

struct argStruct {
    pthread_t threadid;
    int start;
    int end;
    int count;
    int totalWords;
    wordsStruct *words;
};

int
stringLength(char s[])
{
    int c = 0;

    while (s[c] != '\0')
        c++;
    return c;
}

void
groomString(char *line)
{

    for (int i = 0; line[i] != '\0'; ++i) {
        line[i] = tolower(line[i]);

        while (!((line[i] >= 'a' && line[i] <= 'z') || line[i] == '\0')) {

            for (int j = i; line[j] != '\0'; j++) {
                line[j] = line[j + 1];
                line[j] = '\0';
            }
        }
    }
}

int
counter(int n)
{
    static int test;

    test = n;

    // NOTE/BUG: this is the assignment operator and you want equality
#if 0
    if (n = 0) {
#else
    if (n == 0) {
#endif
        return test;
    }
    else {
        n++;
        return n;
    }

}

void *
processFile(void *input)
{
    struct argStruct *params = input;

    int start = params->start;
    int end = params->end;

    int size = (end - start) + 10;
    char delim[] = " \t\v\n\r";

    char *copy = malloc(size + 1);
    memcpy(copy,&filebuf[start],size);
    copy[size] = 0;

    char *saveptr;

    int inArray;
    int length;

    char *token = strtok_r(copy, delim, &saveptr);

    int added = 0;
    params->words = NULL;
    params->count = 0;

    while (token) {
        groomString(token);
        length = stringLength(token);

        if (length > 5) {
            wordsStruct *words = params->words;

            // try to find an existing word struct for the current token
            inArray = 0;
            for (int i = 0; i < added; i++) {
                if (strcmp(words[i].word, token) == 0) {
                    inArray = i;
                    break;
                }
            }

            // found a token that is already in the words list -- just increment
            // the count
            if (inArray != 0) {
                ++words[inArray].count;
                continue;
            }

            // add a new word struct to the list
            ++added;
            words = realloc(words,sizeof(wordsStruct) * added);
            params->words = words;

            // fill it in and initialize its count
            words += added;
            strcpy(words->word, token);
            words->count = 1;
        }

        token = strtok_r(NULL, delim, &saveptr);
    }

    params->totalWords = added;

    free(copy);
    free(token);

    return (void *) 0;
}

int
wordcmp(const void *a,const void *b)
{
    const wordsStruct *wa = a;
    const wordsStruct *wb = b;

    int cmpflg = strcmp(wa->word,wb->word);

    return cmpflg;
}

int
main(int argc, char *argv[])
{
    int pfile;
    int threadCount = 0,
        fileSize = 0,
        divide = 0;
    struct stat st;
    off_t curpos;

#if 0
    wordsStruct *allWords = malloc(sizeof(wordsStruct));
#endif

    if (argc <= 2) {
        printf("Please enter text file name and number of threads");
        return 1;
    }

    pfile = open(argv[1],O_RDONLY);
    if (pfile < 0) {
        perror("FILE OPEN FAILURE");
    }

    threadCount = atoi(argv[2]);

    struct argStruct *threads =
        malloc(sizeof(struct argStruct) * threadCount);
    struct argStruct *arg;

    fstat(pfile,&st);
    fileSize = st.st_size;

    filebuf = mmap(NULL,fileSize,PROT_READ,MAP_PRIVATE,pfile,0);

    divide = (fileSize / threadCount);

#if 0
    int j = 0;
#endif

    for (int i = 0; i < threadCount; i++) {
        arg = &threads[i];

#if 0
        arg->words = allWords;
#endif

        if (i == 0)
            arg->start = 0;
        else
            arg->start = arg[-1].end;

        curpos = arg->start + divide;

        for (;  curpos < fileSize;  ++curpos) {
            if (filebuf[curpos] == '\n') {
                ++curpos;
                break;
            }
        }

        if (curpos > fileSize)
            curpos = fileSize;

        arg->end = curpos;
        arg->count = i;
    }

    for (int i = 0; i < threadCount; i++) {
        arg = &threads[i];
        pthread_create(&arg->threadid, NULL, (void *) processFile, arg);
    }

    for (int i = 0; i < threadCount; i++) {
        arg = &threads[i];
        pthread_join(arg->threadid, NULL);
    }

    munmap(filebuf,fileSize);
    close(pfile);

    // get total number of words in all lists
    int totalcnt = 0;
    for (int i = 0; i < threadCount; i++) {
        arg = &threads[i];
        totalcnt += arg->totalWords;
    }

    // create a unified list [that may have duplicates]
    wordsStruct *biglist = malloc(sizeof(wordsStruct) * totalcnt);
    int bigidx = 0;
    for (int i = 0; i < threadCount; i++) {
        arg = &threads[i];

        for (int wordidx = 0;  wordidx < arg->totalWords;  ++wordidx)
            biglist[bigidx++] = arg->words[wordidx];

        free(arg->words);
    }
    free(threads);

    // sort the list
    qsort(biglist,totalcnt,sizeof(wordsStruct),wordcmp);

    // remove duplicates
    int cleancnt = 0;
    wordsStruct *cleanlist = malloc(sizeof(wordsStruct) * totalcnt);

    if (totalcnt > 0)
        cleanlist[cleancnt++] = biglist[0];

    for (int bigidx = 1; bigidx < totalcnt; ++bigidx) {
        if (strcmp(cleanlist[cleancnt - 1].word,biglist[bigidx].word) == 0)
            continue;
        cleanlist[cleancnt++] = biglist[bigidx];
    }

    free(biglist);

    // trim the list
    cleanlist = realloc(cleanlist,sizeof(wordsStruct) * cleancnt);

    return 0;
}