Thread: Help with memory leaks

Hi all, I'm still new to C and memory leaks are the bane of my existence as I create them often I know I have leaks in this small program, but can't reason where/why. I tried using valgrind but found the output to be extremely complicated and over my head. I'm hoping someone could help point out my mem leaks and would LOVE if you added a quick note on why/how and better practices. Running 'top' while this program runs shows it pretty quickly chewing up all the memory until it freezes if not stopped. I'm sure I'm not following best practices in many places, so feel free to point those out if you want since I'm trying to improve (aren't we all?)

Code:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <sys/time.h>
#include <unistd.h>
#include <openssl/crypto.h>
#include <openssl/sha.h>


//Prototypes
char * hash(char * str);
void brute(char * PW, int index, int depth, char * pass, char * alphabet);




//recursive brute force algo
void brute(char * PW, int index, int depth, char * pass, char * alphabet){
    int alphSize = strlen(alphabet);
    int i;
    for(i=0; i < alphSize; ++i){
        PW[index] = alphabet[i];
        char * curr_hash = hash(PW);
        if(index == depth){
            // test the current combination  agienst the password
            // if they are the same the program reports and ends.
            if(strcmp(curr_hash,pass) == 0){
                //shouldnt need any free's since the program exits and OS frees
                struct timeval tv;
                gettimeofday(&tv, NULL);
                fprintf(stdout, "%s %ld\n", PW, tv.tv_sec); //print time ended and plain text password
                exit(0);
            // if they arent it just prints the current combonation
            }else{
                free(curr_hash); //hash no longer needed so free it
            }
        // if its not the max depth it calls itself again
        }else{
            free(curr_hash); //hash no longer needed so free it
            brute(PW,index+1,depth, pass, alphabet);
        }
    }
}




//Generate and return the sha256 hash of the given string
char * hash(char * str){
    char * hash = malloc(64 + 1);
    hash[strlen(hash)] = '\0';
    unsigned char out_buf[SHA256_DIGEST_LENGTH];
    SHA256(str, strlen(str), out_buf);
    int i;
    for (i = 0; i < SHA256_DIGEST_LENGTH; i++) {
        unsigned char * b = malloc(3);
        b[2] = '\0';
        //must be %02x or filler zeros aren't added
        sprintf(b, "%02x", out_buf[i]); //out_buf[i] is int but needs to be stored as hex
        hash = strcat(hash, b);
        free(b); //shouldnt need since stack mem will be dealloc'd?
    }
    return hash;
}




int main(int argc, char * argv[])
{
    if(argc != 4){
        fprintf(stderr, "Cracker takes a password hash to crack, and a password length, and an alphabet\n");
        exit(1);
    }
    struct timeval tv;
    gettimeofday(&tv, NULL);
    fprintf(stdout, "%ld ", tv.tv_sec); // print time started
    free(&tv); //free struct
    int len = atoi(argv[2]) - 1; // -1, so 'hello' is len 5 but start at aaaa (4)
    char * posPW = malloc(len + 1);
    brute(posPW, 0 , len, argv[1], argv[3]);
    printf("NULL -1\n"); //only gets here if brute didn't crack the pass, so report failure here
}

One way to avoid memory leaks is not to use dynamic memory.
For example:

Code:

char * hash = malloc(64 + 1);

Could easily be: char hash[65];

Code:

free(&tv); //free struct

Why do you call free on a static variable in main

Originally Posted by OldGuy2

One way to avoid memory leaks is not to use dynamic memory.
For example:

Code:

char * hash = malloc(64 + 1);

Could easily be: char hash[65];

Code:

free(&tv); //free struct

Why do you call free on a static variable in main

I thought that char * hash[65] would be in local scope only and would be lost when the method returned (since I need the return value), is that not the case? The only reason I used malloc was to make it heap memory instead of stack, but maybe I've misunderstood how that works. The free on the timeval struct in main() was a silly oversight, in my head I thought it was malloc'ed.. whoops! Thanks for the help so far, I appreciate it!

There's too much to go into here, so I rewrote it. I removed all the dynamic allocations except for PW in main.

Code:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <openssl/sha.h>

char *make_hash(char *str, char *hash);
int brute(char *PW, int index, int depth, char *pass, char *alphabet);

int brute(char *PW, int index, int depth, char *pass, char *alphabet){
    int alphSize = strlen(alphabet);
    for (int i = 0; i < alphSize; ++i){
        PW[index] = alphabet[i];
        if (index == depth){
            PW[index+1] = 0;
            char hash[SHA256_DIGEST_LENGTH * 2 + 1];
            make_hash(PW, hash);
            if (strcmp(hash, pass) == 0)
                return 1;
        }
        else if (brute(PW, index+1, depth, pass, alphabet))
            return 1;
    }
    return 0;
}

char *make_hash(char *str, char *hash){
    char buf[SHA256_DIGEST_LENGTH];
    SHA256((unsigned char*)str, strlen(str), (unsigned char*)buf);
    int i = 0;
    for ( ; i < SHA256_DIGEST_LENGTH; i++)
        sprintf(hash + i * 2, "%02x", (unsigned char)buf[i]);
    hash[i * 2] = 0;
}

int main(int argc, char * argv[]) {
    if(argc != 4){
        fprintf(stderr, "cracker hash length alphabet\n");
        exit(EXIT_FAILURE);
    }

    int len = atoi(argv[2]);
    char *PW = malloc(len + 1);

    if (brute(PW, 0, len - 1, argv[1], argv[3]))
        printf("Password: %s\n", PW);
    else
        printf("failed\n");

    free(PW);
    return 0;
}

$ gcc -std=c99 -o crack crack.c -lcrypto
$ ./crack 2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73 043362938b9824 5 abcdefghijklmno

Password: hello

Be forewarned that trying to crack a real password this way is pointless.

Originally Posted by algorism

It needs to be dynamic since it's returned from the function.



You mean "automatic", not "static".

EDIT: Since I got "moderated" trying to post my answer to the OP, I'll put it here instead. (Hopefully the duplicate can be deleted.)

There's too much to go into here, so I rewrote it. I removed all the dynamic allocations except for PW in main.

Code:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <openssl/sha.h>

char *make_hash(char *str, char *hash);
int brute(char *PW, int index, int depth, char *pass, char *alphabet);

int brute(char *PW, int index, int depth, char *pass, char *alphabet){
    int alphSize = strlen(alphabet);
    for (int i = 0; i < alphSize; ++i){
        PW[index] = alphabet[i];
        if (index == depth){
            PW[index+1] = 0;
            char hash[SHA256_DIGEST_LENGTH * 2 + 1];
            make_hash(PW, hash);
            if (strcmp(hash, pass) == 0)
                return 1;
        }
        else if (brute(PW, index+1, depth, pass, alphabet))
            return 1;
    }
    return 0;
}

char *make_hash(char *str, char *hash){
    char buf[SHA256_DIGEST_LENGTH];
    SHA256((unsigned char*)str, strlen(str), (unsigned char*)buf);
    int i = 0;
    for ( ; i < SHA256_DIGEST_LENGTH; i++)
        sprintf(hash + i * 2, "%02x", (unsigned char)buf[i]);
    hash[i * 2] = 0;
}

int main(int argc, char * argv[]) {
    if(argc != 4){
        fprintf(stderr, "cracker hash length alphabet\n");
        exit(EXIT_FAILURE);
    }

    int len = atoi(argv[2]);
    char *PW = malloc(len + 1);

    if (brute(PW, 0, len - 1, argv[1], argv[3]))
        printf("Password: %s\n", PW);
    else
        printf("failed\n");

    free(PW);
    return 0;
}

$ gcc -std=c99 -o crack crack.c -lcrypto
$ ./crack 2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73 043362938b9824 5 abcdefghijklmno

Password: hello

Note that the forum has added a space in the hash that shouldn't be there.

And be forewarned that trying to crack a real password this way is pointless.

Wow thanks for taking the time to go that in depth. The weird control flow was a product of a different algorithm that got modified to this and it seemed to function so I left it. Bad choice as its ugly no doubt. I like you're solution to make_hash not even needing a return value, very cool. Also don't worry, I'm not expecting to crack passwords over a few characters, this is part of a bigger project to highlight the differences between rainbow tables and brute force cracking.