CyberFox
CyberFox

Reputation: 363

Memory leaks and pointers

I have implemented a program which adds up the elements of an array using threads. Each thread adds the elements of a portion of the array and puts it inside an array. Finally, another thread sums those up.

This programs seems to work, BUT I have got a ton of memory leaks I cannot seem to be able to fix. I am using valgrind to detected those memory leaks. My bet is that my pointer knowledge is missing some key features.

I have also tried to free struct just before the thread returns, but that dumps the core. I have tried to free wherever I allocated some memory, but valgrind reports more allocations than frees.

Now, for some code:

#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>
#include <pthread.h>
#define MAX_RAND 100
/**
 * @brief global variables shared by threads
 *      array -> the array of values
 *      size-> the size of the array
 *      numberOfThreads -> the number of threads used to compute the sum
 * 
 */
int *array;
int size;
int numberOfThreads = 3;

/**
 * @brief each thread works on  disjoint segments of an array(i.e. two or more threads shall never work on the same segment). This
 * encapsulates the start and end indexes used by a thread.
 * 
 */
typedef struct
{
    int startIndex;
    int endIndex;
} wrapper;

/**
 * @brief this is used by the extra thread and contains the array of sums computed by the previous threads, along the size of the results
 * array
 * 
 */
typedef struct
{
    int *array;
    int size;
} resultsWrapper;

/**
 * @brief this computes the sum of a disjoint segment of the shared array
 * 
 * @param args 
 * @return void* 
 */
void *sum_runner(void *args)
{
    /**
     * @brief cast the argument and retrieve the indexes
     * 
     */
    wrapper *data = (wrapper *)args;
    int currentIndex = data->startIndex;
    int endIndex = data->endIndex;
    int *sum = (int *)malloc(sizeof(int *));
    *sum = 0;

    /**
     * @brief iterate that segment and compute the sum
     * 
     */
    while (currentIndex < endIndex)
    {
        *sum += array[currentIndex];
        currentIndex++;
    }

    /**
     * @brief return the sum
     * 
     */
    pthread_exit((void *)sum);
}

/**
 * @brief this is used by the an extra thread to sum up the elements computed by the previouse threads
 * 
 * @param args 
 * @return void* 
 */
void *results_sum_runner(void *args)
{
    /**
     * @brief cast the arguments and retrieve the array and its size
     * 
     */
    resultsWrapper *results = (resultsWrapper *)args;
    int size = results->size;
    int *array = results->array;
    int *sum = (int *)malloc(sizeof(int *));
    *sum = 0;

    /**
     * @brief sum its elements up
     * 
     */
    for (int i = 0; i < size; i++)
    {
        *sum += array[i];
    }

    free(results);
    free(array);

    /**
     * @brief return the result
     * 
     */
    pthread_exit((void *)sum);
}

/**
 * @brief populates the given vector with random numbers
 * 
 * @param array the target array
 * @param size the size of the array
 */
void populateWithRand(int *array, int size)
{
    for (int i = 0; i < size; i++)
    {
        array[i] = rand() % MAX_RAND;
    }
    printf("Finished populating vector\n");
}

/**
 * @brief this prints a given array
 * 
 * @param array the array to be printed
 * @param size the size of the array
 */
void printArray(int *array, int size)
{
    printf("Array: \n");
    for (int i = 0; i < size; i++)
    {
        printf("%d ", array[i]);
    }
    printf("\n");
}

/**
 * @brief this creates a normal partition, i.e. a partition containing size/threads elements
 * 
 * @param index the current index in array  
 * @param quotient the quotient
 * @return wrapper returns a new "pair"
 */
wrapper createNormalPartition(int index, int quotient)
{
    wrapper normalPartition;
    normalPartition.startIndex = index;
    normalPartition.endIndex = index + quotient - 1;
    printf("    Created normal partition (%d, %d)\n", normalPartition.startIndex, normalPartition.endIndex);
    return normalPartition;
}

/**
 * @brief this creates an overloaded partition, i.e. a partition containing size/threads+1 elements. We use overloaded partitions to spread
 * the load amongst r threads, where r is size%threads
 * 
 * @param index the current index in the array
 * @param quotient the quotient
 * @return wrapper returns a new "overloaded pair"
 */
wrapper createOverloadedPartition(int index, int quotient)
{
    wrapper normalPartition;
    normalPartition.startIndex = index;
    normalPartition.endIndex = index + quotient;
    printf("    Created normal partition (%d, %d)\n", normalPartition.startIndex, normalPartition.endIndex);
    return normalPartition;
}

/**
 * @brief core function. Splits the entire array by index to each thread
 * 
 * @return wrapper* returns an array of partitions to be used by threads
 */
wrapper *createPartitions()
{
    printf("Creating partitions......\n");
    /**
     * @brief compute the quotient, the remainder and initialize
     * 
     */
    int quotient = size / numberOfThreads;
    int remainder = size % numberOfThreads;
    int last = 0;

    /**
     * @brief create the array of partitions
     * 
     */
    wrapper *partitions = (wrapper *)malloc(sizeof(wrapper));

    /**
     * @brief populate the previously created array. If the we have a remainder, the last r threads will have an extra computation to perform.
     * 
     */
    for (int i = 0; i < numberOfThreads; i++)
    {
        /**
             * @brief check the current index and create the appropriate partitions
             * 
             */
        if (i < numberOfThreads - remainder)
        {
            partitions[i] = createNormalPartition(last, quotient);
            last += quotient;
            continue;
        }
        wrapper temp = createOverloadedPartition(last, quotient);

        partitions[i] = temp;
        last += quotient + 1;
    }
    printf("Finished creating partitions......\n");

    /**
     * @brief return the previously-populated partitions
     * 
     */
    return partitions;
}

/**
 * @brief this is a utility function. This creates the threads and assigns them the working partition
 * 
 * @param threads the array of threads
 */
void createThreads(pthread_t *threads)
{
    /**
     * @brief create a dummy wrapper to store the pairs at every step
     * 
     */
    wrapper *partitions = createPartitions();

    printf("Creating threads......\n");

    /**
     * @brief create some threads
     * 
     */
    for (int i = 0; i < numberOfThreads; i++)
    {
        pthread_create(&threads[i], NULL, sum_runner, (void *)&partitions[i]);
        printf("    Created thread %lu\n", threads[i]);
    }
    free(partitions);
    printf("Finished creating threads......\n");
}

/**
 * @brief this is a utility function. This performs join in the threads and stores the sums computed
 * 
 * @param threads the array of threads
 * @return int* the array of computed sums
 */
int *startThreads(pthread_t *threads)
{
    printf("Starting threads...\n");
    /**
     * @brief initialize local variables
     * 
     */
    int *temp;
    int *results = (int *)malloc(sizeof(int *) * numberOfThreads);

    /**
     * @brief performs join on threads and store the sums computed
     * 
     */
    for (int i = 0; i < numberOfThreads; i++)
    {
        temp = (int *)malloc(sizeof(temp));
        printf("    Starting thread %lu\n", threads[i]);
        pthread_join(threads[i], (void **)&temp);
        results[i] = *temp;
        free(temp);
    }

    printf("Exiting...\n");
    /**
     * @brief return the array of computed sums
     * 
     */
    return results;
}

/**
 * @brief this function calls the utility functions and computes the final sum using a separate thread
 * 
 * @return int 
 */
int run()
{
    /**
     * @brief create the threads ids array
     * 
     */
    int *results;
    int *finalResult;
    pthread_t *threads = (pthread_t *)malloc(sizeof(pthread_t *));
    pthread_t summingThread;

    /**
     * @brief pass the threads id's to create them
     * 
     */
    createThreads(threads);

    /**
     * @brief get the sums
     * 
     */
    results = startThreads(threads);

    /**
     * @brief print the array 
     * 
     */
    printArray(results, numberOfThreads);

    /**
     * @brief create a new thread and let him compute the final sum
     * 
     */
    resultsWrapper *resultData = (resultsWrapper *)malloc(sizeof(resultsWrapper *));
    resultData->size = numberOfThreads;
    resultData->array = results;

    /**
     * @brief add up the sums computed by the threads
     * 
     */
    pthread_create(&summingThread, NULL, results_sum_runner, (void *)resultData);
    pthread_join(summingThread, (void *)&finalResult);

    /**
     * @brief return the sum
     * 
     */
    free(threads);
    free(resultData);
    free(results);
    return *finalResult;
}

/**
 * @brief this is the entry point
 * 
 * @return int success
 */
int main()
{
    /**
     * @brief initialize variables, run the program and print the result
     * 
     */
    size = 47;
    array = calloc(sizeof(array), size);
    populateWithRand(array, size);
    printArray(array, size);
    int sum;
    sum = run();
    free(array);
    printf("Sum of the array is %d\n", sum);
}

Sample output:

Finished populating vector
Array:
83 86 77 15 93 35 86 92 49 21 62 27 90 59 63 26 40 26 72 36 11 68 67 29 82 30 62 23 67 35 29 2 22 58 69 67 93 56 11 42 29 73 21 19 84 37 98
Creating partitions......
    Created normal partition (0, 14)
    Created normal partition (15, 30)
    Created normal partition (31, 46)
Finished creating partitions......
Creating threads......
    Created thread 139720910055168
    Created thread 139720901662464
    Created thread 139720893269760
Finished creating threads......
Starting threads...
    Starting thread 139720910055168
    Starting thread 139720901662464
    Starting thread 139720893269760
Exiting...
Array:
875 674 683
Sum of the array is 2232

Valgrind's report:

==4725== Memcheck, a memory error detector
==4725== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.==4725== Using Valgrind-3.14.0.GIT and LibVEX; rerun with -h for copyright info==4725== Command: ./counter
==4725==Finished populating vector
Array:
83 86 77 15 93 35 86 92 49 21 62 27 90 59 63 26 40 26 72 36 11 68 67 29 82 30 62 23 67 35 29 2 22 58 69 67 93 56 11 42 29 73 21 19 84 37 98
Creating partitions......
    Created normal partition (0, 14)
    Created normal partition (15, 30)
==4725== Invalid write of size 8
==4725==    at 0x109505: createPartitions (counter.c:215)
==4725==    by 0x109550: createThreads (counter.c:238)
==4725==    by 0x10973A: run (counter.c:312)
==4725==    by 0x10985C: main (counter.c:367)
==4725==  Address 0x4a61698 is 0 bytes after a block of size 8 alloc'd
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x10948F: createPartitions (counter.c:195)
==4725==    by 0x109550: createThreads (counter.c:238)
==4725==    by 0x10973A: run (counter.c:312)
==4725==    by 0x10985C: main (counter.c:367)
==4725==
    Created normal partition (31, 46)
Finished creating partitions......
Creating threads......
    Created thread 90576640
==4725== Invalid write of size 8
==4725==    at 0x48812C8: pthread_create@@GLIBC_2.2.5 (in /usr/lib/libpthread-2.28.so)
==4725==    by 0x1095A8: createThreads (counter.c:248)
==4725==    by 0x10973A: run (counter.c:312)
==4725==    by 0x10985C: main (counter.c:367)
==4725==  Address 0x4a61648 is 0 bytes after a block of size 8 alloc'd
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x10972A: run (counter.c:305)
==4725==    by 0x10985C: main (counter.c:367)
==4725==
==4725== Invalid read of size 8
==4725==    at 0x1095BD: createThreads (counter.c:249)
==4725==    by 0x10973A: run (counter.c:312)
==4725==    by 0x10985C: main (counter.c:367)
==4725==  Address 0x4a61648 is 0 bytes after a block of size 8 alloc'd
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x10972A: run (counter.c:305)
==4725==    by 0x10985C: main (counter.c:367)
==4725==
    Created thread 98969344
    Created thread 107362048
==4725== Thread 3:
==4725== Invalid read of size 4
==4725==    at 0x1091F1: sum_runner (counter.c:52)
==4725==    by 0x4880A9C: start_thread (in /usr/lib/libpthread-2.28.so)
==4725==    by 0x4995B22: clone (in /usr/lib/libc-2.28.so)
==4725==  Address 0x4a61698 is 0 bytes after a block of size 8 alloc'd
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x10948F: createPartitions (counter.c:195)
==4725==    by 0x109550: createThreads (counter.c:238)
==4725==    by 0x10973A: run (counter.c:312)
==4725==    by 0x10985C: main (counter.c:367)
==4725==
==4725== Invalid read of size 4
==4725==    at 0x1091FA: sum_runner (counter.c:53)
==4725==    by 0x4880A9C: start_thread (in /usr/lib/libpthread-2.28.so)
==4725==    by 0x4995B22: clone (in /usr/lib/libc-2.28.so)
==4725==  Address 0x4a6169c is 4 bytes after a block of size 8 alloc'd
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x10948F: createPartitions (counter.c:195)
==4725==    by 0x109550: createThreads (counter.c:238)
==4725==    by 0x10973A: run (counter.c:312)
==4725==    by 0x10985C: main (counter.c:367)
==4725==
Finished creating threads......
Starting threads...
    Starting thread 90576640
==4725== Thread 1:
==4725== Invalid read of size 8
==4725==    at 0x10966B: startThreads (counter.c:278)
==4725==    by 0x109746: run (counter.c:318)
==4725==    by 0x10985C: main (counter.c:367)
==4725==  Address 0x4a61648 is 0 bytes after a block of size 8 alloc'd
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x10972A: run (counter.c:305)
==4725==    by 0x10985C: main (counter.c:367)
==4725==
    Starting thread 98969344
==4725== Invalid read of size 8
==4725==    at 0x109696: startThreads (counter.c:279)
==4725==    by 0x109746: run (counter.c:318)
==4725==    by 0x10985C: main (counter.c:367)
==4725==  Address 0x4a61648 is 0 bytes after a block of size 8 alloc'd
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x10972A: run (counter.c:305)
==4725==    by 0x10985C: main (counter.c:367)
==4725==
    Starting thread 107362048
Exiting...
Array:
875 674 683
==4725== Invalid write of size 4
==4725==    at 0x109777: run (counter.c:331)
==4725==    by 0x10985C: main (counter.c:367)
==4725==  Address 0x4a62508 is 0 bytes after a block of size 8 alloc'd
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x109768: run (counter.c:330)
==4725==    by 0x10985C: main (counter.c:367)
==4725==
==4725== Thread 2:
==4725== Invalid read of size 4
==4725==    at 0x10926E: results_sum_runner (counter.c:87)
==4725==    by 0x4880A9C: start_thread (in /usr/lib/libpthread-2.28.so)
==4725==    by 0x4995B22: clone (in /usr/lib/libc-2.28.so)
==4725==  Address 0x4a62508 is 0 bytes after a block of size 8 alloc'd
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x109768: run (counter.c:330)
==4725==    by 0x10985C: main (counter.c:367)
==4725==
==4725== Thread 1:
==4725== Invalid free() / delete / delete[] / realloc()
==4725==    at 0x48389AB: free (vg_replace_malloc.c:530)
==4725==    by 0x1097CE: run (counter.c:346)
==4725==    by 0x10985C: main (counter.c:367)
==4725==  Address 0x4a62500 is 0 bytes inside a block of size 8 free'd
==4725==    at 0x48389AB: free (vg_replace_malloc.c:530)
==4725==    by 0x1092DB: results_sum_runner (counter.c:101)
==4725==    by 0x4880A9C: start_thread (in /usr/lib/libpthread-2.28.so)
==4725==    by 0x4995B22: clone (in /usr/lib/libc-2.28.so)
==4725==  Block was alloc'd at
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x109768: run (counter.c:330)
==4725==    by 0x10985C: main (counter.c:367)
==4725==
==4725== Invalid free() / delete / delete[] / realloc()
==4725==    at 0x48389AB: free (vg_replace_malloc.c:530)
==4725==    by 0x1097DA: run (counter.c:347)
==4725==    by 0x10985C: main (counter.c:367)
==4725==  Address 0x4a61b20 is 0 bytes inside a block of size 24 free'd
==4725==    at 0x48389AB: free (vg_replace_malloc.c:530)
==4725==    by 0x1092E7: results_sum_runner (counter.c:102)
==4725==    by 0x4880A9C: start_thread (in /usr/lib/libpthread-2.28.so)
==4725==    by 0x4995B22: clone (in /usr/lib/libc-2.28.so)
==4725==  Block was alloc'd at
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x109638: startThreads (counter.c:269)
==4725==    by 0x109746: run (counter.c:318)
==4725==    by 0x10985C: main (counter.c:367)
==4725==
Sum of the array is 2232
==4725==
==4725== HEAP SUMMARY:
==4725==     in use at exit: 1,652 bytes in 8 blocks
==4725==   total heap usage: 21 allocs, 15 frees, 3,996 bytes allocated
==4725==
==4725== LEAK SUMMARY:
==4725==    definitely lost: 32 bytes in 4 blocks
==4725==    indirectly lost: 0 bytes in 0 blocks
==4725==      possibly lost: 0 bytes in 0 blocks
==4725==    still reachable: 1,620 bytes in 4 blocks
==4725==         suppressed: 0 bytes in 0 blocks
==4725== Rerun with --leak-check=full to see details of leaked memory
==4725==
==4725== For counts of detected and suppressed errors, rerun with: -v
==4725== ERROR SUMMARY: 20 errors from 11 contexts (suppressed: 0 from 0)

Upvotes: 3

Views: 657

Answers (2)

dbush
dbush

Reputation: 224072

There are some memory leaks here, for sure, but the more pressing problem you have is multiple instances of reading / writing past the end of allocated memory and double-frees.

Let's take these one at a time starting from the top:

==4725== Invalid write of size 8
==4725==    at 0x109505: createPartitions (counter.c:215)
==4725==    by 0x109550: createThreads (counter.c:238)
==4725==    by 0x10973A: run (counter.c:312)
==4725==    by 0x10985C: main (counter.c:367)
==4725==  Address 0x4a61698 is 0 bytes after a block of size 8 alloc'd
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x10948F: createPartitions (counter.c:195)
==4725==    by 0x109550: createThreads (counter.c:238)
==4725==    by 0x10973A: run (counter.c:312)
==4725==    by 0x10985C: main (counter.c:367)
==4725==

This is complaining about writing past what was allocated. Here's the code that does both the allocation and the invalid access:

wrapper *partitions = (wrapper *)malloc(sizeof(wrapper));

for (int i = 0; i < numberOfThreads; i++)
{
    if (i < numberOfThreads - remainder)
    {
        partitions[i] = createNormalPartition(last, quotient);
        last += quotient;
        continue;
    }
    wrapper temp = createOverloadedPartition(last, quotient);

    partitions[i] = temp;
    last += quotient + 1;
}

You're allocating space for a single instance of wrapper, but writing to it as if it were an array of numberOfThreads instances. You need to allocate space for this many instances:

wrapper *partitions = malloc(sizeof(wrapper) * numberOfThreads);

Next one:

==4725== Invalid write of size 8
==4725==    at 0x48812C8: pthread_create@@GLIBC_2.2.5 (in /usr/lib/libpthread-2.28.so)
==4725==    by 0x1095A8: createThreads (counter.c:248)
==4725==    by 0x10973A: run (counter.c:312)
==4725==    by 0x10985C: main (counter.c:367)
==4725==  Address 0x4a61648 is 0 bytes after a block of size 8 alloc'd
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x10972A: run (counter.c:305)
==4725==    by 0x10985C: main (counter.c:367)
==4725==
==4725== Invalid read of size 8
==4725==    at 0x1095BD: createThreads (counter.c:249)
==4725==    by 0x10973A: run (counter.c:312)
==4725==    by 0x10985C: main (counter.c:367)
==4725==  Address 0x4a61648 is 0 bytes after a block of size 8 alloc'd
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x10972A: run (counter.c:305)
==4725==    by 0x10985C: main (counter.c:367)
==4725==

Another invalid write along with an invalid read. Here's the allocation in run:

pthread_t *threads = (pthread_t *)malloc(sizeof(pthread_t *));

And the read / write in createThreads:

for (int i = 0; i < numberOfThreads; i++)
{
    pthread_create(&threads[i], NULL, sum_runner, (void *)&partitions[i]);
    printf("    Created thread %lu\n", threads[i]);
}

As before, you're allocating space for a single instance, not an array. Also, you're allocating space for a pthread_t * instead of a pthread_t, which is likely to be too small. Change the allocation to make space for an array, and use the object type, not the pointer type:

pthread_t *threads = malloc(sizeof(pthread_t) * numberOfThreads);

Next:

==4725== Invalid read of size 4
==4725==    at 0x1091F1: sum_runner (counter.c:52)
==4725==    by 0x4880A9C: start_thread (in /usr/lib/libpthread-2.28.so)
==4725==    by 0x4995B22: clone (in /usr/lib/libc-2.28.so)
==4725==  Address 0x4a61698 is 0 bytes after a block of size 8 alloc'd
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x10948F: createPartitions (counter.c:195)
==4725==    by 0x109550: createThreads (counter.c:238)
==4725==    by 0x10973A: run (counter.c:312)
==4725==    by 0x10985C: main (counter.c:367)
==4725==
==4725== Invalid read of size 4
==4725==    at 0x1091FA: sum_runner (counter.c:53)
==4725==    by 0x4880A9C: start_thread (in /usr/lib/libpthread-2.28.so)
==4725==    by 0x4995B22: clone (in /usr/lib/libc-2.28.so)
==4725==  Address 0x4a6169c is 4 bytes after a block of size 8 alloc'd
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x10948F: createPartitions (counter.c:195)
==4725==    by 0x109550: createThreads (counter.c:238)
==4725==    by 0x10973A: run (counter.c:312)
==4725==    by 0x10985C: main (counter.c:367)
==4725==

A pair of invalid reads originating from the same allocation and access in adajent lines. This is the same invalid allocation from the first message, which we've already addressed. Next one:

==4725== Thread 1:
==4725== Invalid read of size 8
==4725==    at 0x10966B: startThreads (counter.c:278)
==4725==    by 0x109746: run (counter.c:318)
==4725==    by 0x10985C: main (counter.c:367)
==4725==  Address 0x4a61648 is 0 bytes after a block of size 8 alloc'd
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x10972A: run (counter.c:305)
==4725==    by 0x10985C: main (counter.c:367)
==4725==
    Starting thread 98969344
==4725== Invalid read of size 8
==4725==    at 0x109696: startThreads (counter.c:279)
==4725==    by 0x109746: run (counter.c:318)
==4725==    by 0x10985C: main (counter.c:367)
==4725==  Address 0x4a61648 is 0 bytes after a block of size 8 alloc'd
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x10972A: run (counter.c:305)
==4725==    by 0x10985C: main (counter.c:367)
==4725==

A pair of invalid reads pointing back to the same bad allocation from the second message. This was also already addressed. Next:

==4725== Invalid write of size 4
==4725==    at 0x109777: run (counter.c:331)
==4725==    by 0x10985C: main (counter.c:367)
==4725==  Address 0x4a62508 is 0 bytes after a block of size 8 alloc'd
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x109768: run (counter.c:330)
==4725==    by 0x10985C: main (counter.c:367)
==4725==
==4725== Thread 2:
==4725== Invalid read of size 4
==4725==    at 0x10926E: results_sum_runner (counter.c:87)
==4725==    by 0x4880A9C: start_thread (in /usr/lib/libpthread-2.28.so)
==4725==    by 0x4995B22: clone (in /usr/lib/libc-2.28.so)
==4725==  Address 0x4a62508 is 0 bytes after a block of size 8 alloc'd
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x109768: run (counter.c:330)
==4725==    by 0x10985C: main (counter.c:367)

An invalid read and write originating from the same allocation. Here's the allocation in run:

resultsWrapper *resultData = (resultsWrapper *)malloc(sizeof(resultsWrapper *));

The read in results_sum_runner:

resultsWrapper *results = (resultsWrapper *)args;
int size = results->size;

And the write in run:

resultData->size = numberOfThreads;

Here, you're allocating space for a pointer to resultsWrapper, not an instance of resultsWrapper. Fix the allocation as follows:

resultsWrapper *resultData = malloc(sizeof(resultsWrapper));

Next one:

==4725== Invalid free() / delete / delete[] / realloc()
==4725==    at 0x48389AB: free (vg_replace_malloc.c:530)
==4725==    by 0x1097CE: run (counter.c:346)
==4725==    by 0x10985C: main (counter.c:367)
==4725==  Address 0x4a62500 is 0 bytes inside a block of size 8 free'd
==4725==    at 0x48389AB: free (vg_replace_malloc.c:530)
==4725==    by 0x1092DB: results_sum_runner (counter.c:101)
==4725==    by 0x4880A9C: start_thread (in /usr/lib/libpthread-2.28.so)
==4725==    by 0x4995B22: clone (in /usr/lib/libc-2.28.so)
==4725==  Block was alloc'd at
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x109768: run (counter.c:330)
==4725==    by 0x10985C: main (counter.c:367)
==4725==

Here's double free. The allocation and second free are in run:

resultsWrapper *resultData = (resultsWrapper *)malloc(sizeof(resultsWrapper *));
resultData->size = numberOfThreads;
resultData->array = results;

pthread_create(&summingThread, NULL, results_sum_runner, (void *)resultData);
pthread_join(summingThread, (void *)&finalResult);

free(threads);
free(resultData);
free(results);

And here's the first free in results_sum_runner:

void *results_sum_runner(void *args)
{
    resultsWrapper *results = (resultsWrapper *)args;
    int size = results->size;
    int *array = results->array;
    int *sum = (int *)malloc(sizeof(int *));
    *sum = 0;

    for (int i = 0; i < size; i++)
    {
        *sum += array[i];
    }

    free(results);
    free(array);

    pthread_exit((void *)sum);
}

Here you're allocating memory for resultData and passing it to the thread function results_sum_runner. That thread frees the memory, but then so does the calling thread afterward. Remove either free(resultData) in run or free(results) in results_sum_runner.

Last one:

==4725== Invalid free() / delete / delete[] / realloc()
==4725==    at 0x48389AB: free (vg_replace_malloc.c:530)
==4725==    by 0x1097DA: run (counter.c:347)
==4725==    by 0x10985C: main (counter.c:367)
==4725==  Address 0x4a61b20 is 0 bytes inside a block of size 24 free'd
==4725==    at 0x48389AB: free (vg_replace_malloc.c:530)
==4725==    by 0x1092E7: results_sum_runner (counter.c:102)
==4725==    by 0x4880A9C: start_thread (in /usr/lib/libpthread-2.28.so)
==4725==    by 0x4995B22: clone (in /usr/lib/libc-2.28.so)
==4725==  Block was alloc'd at
==4725==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==4725==    by 0x109638: startThreads (counter.c:269)
==4725==    by 0x109746: run (counter.c:318)
==4725==    by 0x10985C: main (counter.c:367)
==4725==

Another double free similar to the last one. free(results) in run and free(array) in results_sum_runner refer to the same memory, so get rid of one of them.

Now if we compile with these changes and run again under valgrind, we get one more issue:

==24305== Invalid read of size 4
==24305==    at 0x40090E: sum_runner (x1.c:52)
==24305==    by 0x4E416B9: start_thread (pthread_create.c:333)
==24305==  Address 0x54216a8 is 8 bytes inside a block of size 24 free'd
==24305==    at 0x4C2EDEB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==24305==    by 0x400CEF: createThreads (x1.c:251)
==24305==    by 0x400E3C: run (x1.c:312)
==24305==    by 0x400F5C: main (x1.c:367)
==24305==  Block was alloc'd at
==24305==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==24305==    by 0x400B98: createPartitions (x1.c:195)
==24305==    by 0x400C57: createThreads (x1.c:238)
==24305==    by 0x400E3C: run (x1.c:312)
==24305==    by 0x400F5C: main (x1.c:367)
==24305== 
==24305== Invalid read of size 4
==24305==    at 0x400917: sum_runner (x1.c:53)
==24305==    by 0x4E416B9: start_thread (pthread_create.c:333)
==24305==  Address 0x54216ac is 12 bytes inside a block of size 24 free'd
==24305==    at 0x4C2EDEB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==24305==    by 0x400CEF: createThreads (x1.c:251)
==24305==    by 0x400E3C: run (x1.c:312)
==24305==    by 0x400F5C: main (x1.c:367)
==24305==  Block was alloc'd at
==24305==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==24305==    by 0x400B98: createPartitions (x1.c:195)
==24305==    by 0x400C57: createThreads (x1.c:238)
==24305==    by 0x400E3C: run (x1.c:312)
==24305==    by 0x400F5C: main (x1.c:367)
==24305== 

Here we have a pair of reads from an already free'ed block of memory. The reads happen in sum_runner:

wrapper *data = (wrapper *)args;
int currentIndex = data->startIndex;
int endIndex = data->endIndex;

And the free in createThreads:

for (int i = 0; i < numberOfThreads; i++)
{
    pthread_create(&threads[i], NULL, sum_runner, (void *)&partitions[i]);
    printf("    Created thread %lu\n", threads[i]);
}
free(partitions);

Based on your comments elsewhere in the code, you seems to be under the impression that calling pthread_join starts a thread. This is not the case. Calling pthread_create actually starts the thread, while pthread_join tells the calling thread to wait for the given thread to finish. So you end up freeing this memory before the thread has a chance to use it.

So remove this free and change createThreads to return this pointer. Then in run (where createThreads is called from) and do the free here after the call to startThreads (which should really be renamed to something like waitForThreadsToFinish):

wrapper *createThreads(pthread_t *threads)
{
    wrapper *partitions = createPartitions();

    printf("Creating threads......\n");

    for (int i = 0; i < numberOfThreads; i++)
    {
        pthread_create(&threads[i], NULL, sum_runner, (void *)&partitions[i]);
        printf("    Created thread %lu\n", threads[i]);
    }
    // remove free
    printf("Finished creating threads......\n");
    return partitions;
}

...

int run() 
{
    ...

    wrapper *partitions = createThreads(threads);
    results = startThreads(threads);
    free(partitions);

And that should take care of the invalid accesses.

There are also a few places where you allocate space for an int * (or an array of int *) where you should be allocating space for one or more int. It doesn't cause a problem on your system because a int * is at least as big as a int, but you should still use the correct type regardless.

Also notice that my proposed changes removed the cast on the return value of malloc. See Do I cast the result of malloc? for more details.

There are still a few memory leaks you can find if you pass --leak-check=full to valgrind but I'll leave that as a exercise to the reader.

Upvotes: 1

Paul Floyd
Paul Floyd

Reputation: 6916

Fix your invalid writes before you look for leaks.

Firstly, in createPartitions you allocate memory for partitions, but only enough to store a single wrapper

wrapper *partitions = (wrapper *)malloc(sizeof(wrapper));

You then have a loop the runs numberOfThreads times (3) and copies into the above array. The 2nd and later writes are outside the bounds of the array, and are causing an invalid write.

You need to allocate more memory for partitions, e.g.,

wrapper *partitions = (wrapper *)malloc(numberOfThreads*sizeof(wrapper));

Upvotes: 1

Related Questions