PSet2 Crack: crypt() returning same hash

Question

EDIT: I solved my problem by entirely re-writing my code. I still am not sure what went wrong.

I've been trying to go at the crack problem for a while now and it keeps returning the same hash. For other people this seemed to be the case because crypt() returns a static pointer and they've already overwritten the value, but I think I am working around that. Here is my code:

#include <crypt.h>
#include <cs50.h>
#include <stdio.h>
#include <ctype.h>
#include <string.h>
#include <stdlib.h>

void increment(int *z); // this function will cycle through possible passwords
char *to_password(int *z); // this function will make a possible password from the counter

int main(int argc, char *argv[])
{
    if (argc != 2){ // error out if wrong arguments are given
        printf("Usage: ./crack hash\n");
        return 1;
    }
    else
    {
        bool found = false; // will be changed to true once password is found
        char *salt = malloc(3); // reserve space for salt
        salt[0] = argv[1][0];
        salt[1] = argv[1][1];
        int z_val[] = {0,-1,-1,-1,-1}; // each value represents progression in each character
        int *z = z_val; // pointer to z
        int counter = 1; // for debug
        char *pass; // the password generated by z
        char *hash; // the hash generated by pass and salt
        while (! found)
        {
            pass = to_password(z); // get a password from z
            printf("Trying %s    ",pass);

            hash = crypt(pass,salt); // generate hash from password
            printf("Hash: %s",hash);

            printf("    z is %i, %i, %i, %i, %i.\n",z[0],z[1],z[2],z[3],z[4]);
            if(counter == 10)
            {
                found = true; // to end the loop for now
            }
            increment(z); // increase the password to the next level
            counter++;
        }
    }
}

char *to_password(int *z) // turn the integer array z into a password string
{
    char output[] = "     "; // start with all spaces
    char *o = output;
    for(int i = 0; i < 5 && z[i] != -1; i++) // all z-values that are not -1 are converted into chars
    {
        int val = z[i];
        if(val > 25) // if uppercase
        {
            output[i] = 'A' + val - 26;
        }
        else // if lowercase
        {
            output[i] = 'a' + val;
        }
    }
    return o; // return pointer to output array
}


void increment(int *z) // increments the array z to the next password
{
    bool carry = true; // becomes false if addition doesnt lead to carry-over
    for (int i = 0; carry; i++){
        if (i > 4) // create negative value for z0 if max value is reached
        {
            carry = false;
            z[0] = -1;
        }
        else if (z[i] < 51) // increment by 1 if Z is not reached
        {
            z[i]++;
            carry = false;
        }
        else // set to 0 and go on to next character if Z is reached
        {
            z[i] = 0;
        }
    }
}

Quite long, I know, but the part in question is the while loop in main. I print every potential password before using crypt and I print the hash immediately after using the crypt function. This is the output:

$ ./crack 123
Trying     Hash: 12UFlHxel6uMM    z is 0, -1, -1, -1, -1.
Trying b        Hash: 12UFlHxel6uMM    z is 1, -1, -1, -1, -1.
Trying c        Hash: 12UFlHxel6uMM    z is 2, -1, -1, -1, -1.
Trying d        Hash: 12UFlHxel6uMM    z is 3, -1, -1, -1, -1.
Trying e        Hash: 12UFlHxel6uMM    z is 4, -1, -1, -1, -1.
Trying f        Hash: 12UFlHxel6uMM    z is 5, -1, -1, -1, -1.
Trying g        Hash: 12UFlHxel6uMM    z is 6, -1, -1, -1, -1.
Trying h        Hash: 12UFlHxel6uMM    z is 7, -1, -1, -1, -1.
Trying i        Hash: 12UFlHxel6uMM    z is 8, -1, -1, -1, -1.
Trying j        Hash: 12UFlHxel6uMM    z is 9, -1, -1, -1, -1.
$ ./crack 123
Trying a        Hash: 12UFlHxel6uMM    z is 0, -1, -1, -1, -1.
Trying b        Hash: 12UFlHxel6uMM    z is 1, -1, -1, -1, -1.
Trying c        Hash: 12UFlHxel6uMM    z is 2, -1, -1, -1, -1.
Trying d        Hash: 12UFlHxel6uMM    z is 3, -1, -1, -1, -1.
Trying e        Hash: 12UFlHxel6uMM    z is 4, -1, -1, -1, -1.
Trying f        Hash: 12UFlHxel6uMM    z is 5, -1, -1, -1, -1.
Trying g        Hash: 12UFlHxel6uMM    z is 6, -1, -1, -1, -1.
Trying h        Hash: 12UFlHxel6uMM    z is 7, -1, -1, -1, -1.
Trying i        Hash: 12UFlHxel6uMM    z is 8, -1, -1, -1, -1.
Trying j        Hash: 12UFlHxel6uMM    z is 9, -1, -1, -1, -1.

Notice also how the first time around it doesn't print the password 'a' properly. I think I got confused in messing with pointers and whatnot, but why are all the hashes the same if I am passing different arguments?

Answer 1

I'd say crypt works as expected, and is not the cause for your problem.

Passing z or z_val does the same thing, as arrays are passed as a pointer to their first element. Same goes for returning o or output. And here's a problem: The array output got declared on to_password's stack. When it returns, that area of the stack will be marked as "unused" (the stack pointer gets reset to the value it had before calling the function), and the next variable declaration or function call (for example to printf) might overwrite the content of the returned array.

A way around would be using malloc (together with free), which allocates on heap, and therefore is not automatically affected by block boundaries. Another way (which I would prefer here) would be to declare the array in the calling code, and passing it as another argument to the function, just like you do with z.

I changed pass to be an array instead of pointer, changed to_password to take the array instead of returning a pointer, and this fixes the missing "a" and the hash being same. BTW, shorter passwords should end in a null terminator, not a bunch of space characters.