I see something worse each time I reread it...



  • Look beyond the pointless repetition, it gets worse. My favorite is the pwd1_num_hashes++ even though it only stores the last 10 (Can't wait until someone decides the history should be 16 and the null derefs start).

    int change_password(pwd_config_t *pwd_config, uint8_t *new_hash, int pwd_number)
    {
        int ret = ERR_OK;
    
    switch(pwd_number)
    {
        case PWD_NUM_1:
            switch(pwd_config->pwd1_num_hashes)
            {
                case 0:
                    FREE_IF_NOT_NULL(pwd_config->pwd1_hash1);
                    pwd_config->pwd1_hash1 = malloc(strlen((char *)new_hash));
                    strcpy(pwd_config->pwd1_hash1, (char *)new_hash);
                    pwd_config->pwd1_num_hashes++;
                    break;
                case 1:
                    if(strcmp((char *)new_hash, pwd_config->pwd1_hash1) != 0)
                    {
                        FREE_IF_NOT_NULL(pwd_config->pwd1_hash2);
                        pwd_config->pwd1_hash2 = malloc(strlen((char *)new_hash));
                        strcpy(pwd_config->pwd1_hash2, (char *)new_hash);
                        pwd_config->pwd1_num_hashes++;
                    }
                    else
                        return ERR_MATCH;
                    break;
                case 2:
                    if((strcmp((char *)new_hash, pwd_config->pwd1_hash1) != 0) &&
                            (strcmp((char *)new_hash, pwd_config->pwd1_hash2) != 0))
                    {
                        FREE_IF_NOT_NULL(pwd_config->pwd1_hash3);
                        pwd_config->pwd1_hash3 = malloc(strlen((char *)new_hash));
                        strcpy(pwd_config->pwd1_hash3, (char *)new_hash);
                        pwd_config->pwd1_num_hashes++;
                    }
                    else
                        return ERR_MATCH;
                    break;
                case 3:
                    if((strcmp((char *)new_hash, pwd_config->pwd1_hash1) != 0) &&
                            (strcmp((char *)new_hash, pwd_config->pwd1_hash2) != 0) &&
                            (strcmp((char *)new_hash, pwd_config->pwd1_hash3) != 0))
                    {
                        FREE_IF_NOT_NULL(pwd_config->pwd1_hash4);
                        pwd_config->pwd1_hash4 = malloc(strlen((char *)new_hash));
                        strcpy(pwd_config->pwd1_hash4, (char *)new_hash);
                        pwd_config->pwd1_num_hashes++;
                    }
                    else
                        return ERR_MATCH;
                    break;
                case 4:
    
                    /* ... Snip cases 4-9 ... */
    
                default:
                    if((strcmp((char *)new_hash, pwd_config->pwd1_hash1) != 0) &&
                            (strcmp((char *)new_hash, pwd_config->pwd1_hash2) != 0) &&
                            (strcmp((char *)new_hash, pwd_config->pwd1_hash3) != 0) &&
                            (strcmp((char *)new_hash, pwd_config->pwd1_hash4) != 0) &&
                            (strcmp((char *)new_hash, pwd_config->pwd1_hash5) != 0) &&
                            (strcmp((char *)new_hash, pwd_config->pwd1_hash6) != 0) &&
                            (strcmp((char *)new_hash, pwd_config->pwd1_hash7) != 0) &&
                            (strcmp((char *)new_hash, pwd_config->pwd1_hash8) != 0) &&
                            (strcmp((char *)new_hash, pwd_config->pwd1_hash9) != 0) &&
                            (strcmp((char *)new_hash, pwd_config->pwd1_hash10) != 0))
                    {
                        strcpy(pwd_config->pwd1_hash1, pwd_config->pwd1_hash2);
                        strcpy(pwd_config->pwd1_hash2, pwd_config->pwd1_hash3);
                        strcpy(pwd_config->pwd1_hash3, pwd_config->pwd1_hash4);
                        strcpy(pwd_config->pwd1_hash4, pwd_config->pwd1_hash5);
                        strcpy(pwd_config->pwd1_hash5, pwd_config->pwd1_hash6);
                        strcpy(pwd_config->pwd1_hash6, pwd_config->pwd1_hash7);
                        strcpy(pwd_config->pwd1_hash7, pwd_config->pwd1_hash8);
                        strcpy(pwd_config->pwd1_hash8, pwd_config->pwd1_hash9);
                        strcpy(pwd_config->pwd1_hash9, pwd_config->pwd1_hash10);
                        strcpy(pwd_config->pwd1_hash10, (char *)new_hash);
    
                        pwd_config->pwd1_num_hashes++;
                    }
                    else
                        return ERR_MATCH;
                    break;
            }
            break;
        case PWD_NUM_2:
            switch(pwd_config->pwd2_num_hashes)
            {
                /* ... Snip all the above with s/pwd1_hash/pwd2_hash/ ...*/
            }
            break;
        default:
            break;
    }
    
    return ret;
    

    }

    ...and the original had mixed tabs and spaces too--but only in the second case statement

    ...and there's a check_password_history() function that duplicates all of that, but without the strcpy's.


  • Discourse touched me in a no-no place

    @omega0 said:

    FREE_IF_NOT_NULL(...)
    If this is C or C++, then this is most certainly a WTF if all it does is simply free() the stuff in the parens - can you confirm what (I'm presuming) the #define is for this?



  • @PJH said:

    @omega0 said:
    FREE_IF_NOT_NULL(...)
    If this is C or C++, then this is most certainly a WTF if all it does is simply free() the stuff in the parens - can you confirm what (I'm presuming) the #define is for this?

    Well, you wouldn't want to call free() on a null pointer and have nothing happen, now would you?

    (I actually use a macro instead of calling free() directly, but mine kills the program if I try to free a null pointer. That's because I'd rather be sure my free()s and malloc()s are properly-balanced and not just have people come along and litter the code with calls to free(), trying to minimize the memory leaks they've introduced..)



  • This is pretty bad code, from casting new_hash to (char*), taking the strlen every step, NOT adding one to the length the pointless loop, ... WTF is this?



  • @TGV said:

    casting new_hash to (char*)

    Wait, what the shit? I just looked at the actual code for the first time.. maybe it's just because I'm not fully awake yet, but isn't a strlen() on a pointer to an int just going to go like "WHEEEEEEEEEEEEE" until it hits a byte with a zero value? So the strlen value will just sorta be random?


  • ♿ (Parody)

    @morbiuswilters said:

    @TGV said:
    casting new_hash to (char*)

    Wait, what the shit? I just looked at the actual code for the first time.. maybe it's just because I'm not fully awake yet, but isn't a strlen() on a pointer to an int just going to go like "WHEEEEEEEEEEEEE" until it hits a byte with a zero value? So the strlen value will just sorta be random?

    It's probably depending on the 8-bit integer being stored in a larger word and having the rest of the word zeroed out.

    The original coder figured uint8_t was more portable than char, but strlen() gets what strlen() wants.


  • Discourse touched me in a no-no place

    @morbiuswilters said:

    Well, you wouldn't want to call free() on a null pointer and have nothing happen, now would you?
    Considering free(NULL) is perfectly well defined, I can't see why not.



  • @PJH said:

    @morbiuswilters said:
    Well, you wouldn't want to call free() on a null pointer and have nothing happen, now would you?
    Considering free(NULL) is perfectly well defined, I can't see why not.

    Sorry, it was a joke. ("and have nothing happen")



  • @boomzilla said:

    The original coder figured uint8_t was more portable than char, but strlen() gets what strlen() wants.

    My first inclination was to assume he was storing an int value in the char *, but then with the strlen() it makes no fucking sense.

    My second inclination is to assume new_hash actually is a string, but for some fucking reason he passed it in as a uint8_t instead of a char.. I guess that's what's happening, but shiiiit.


  • ♿ (Parody)

    @morbiuswilters said:

    My second inclination is to assume new_hash actually is a string, but for some fucking reason he passed it in as a uint8_t instead of a char.. I guess that's what's happening, but shiiiit.

    Yeah. The question is where the uint8_t nonsense starts. Sometimes you've got some library that does something like this, and you have to cast it at some point. I suspect this is not a reasonable demonstration of that, though.



  • @boomzilla said:

    Yeah. The question is where the uint8_t nonsense starts. Sometimes you've got some library that does something like this, and you have to cast it at some point. I suspect this is not a reasonable demonstration of that, though.

    Even if it was an external library, they could just do the cast once, instead of copy-and-pasting it. Given the quality of the code, I think it's fair to assume the uint8_t retardity originated with this developer.



  • @morbiuswilters said:

    I think it's fair to assume the uint8_t retardity originated with this developer.
    TIL: retardity



  • Never even mind the uint8_t stuff, that's just a red herring to distract you from TRWTF: are these really hashes of the passwords or not? If they're really hashes, how can they be safely passed to all those str* functions when they might contain NUL bytes? If they're not hashes, are the passwords secure? And if the use of str* functions is safe, does that mean they're storing the hashes as ASCII representations of the hex bytes? I guess that's at least not insecure, but it's still daft.



  • @DaveK said:

    And if the use of str* functions is safe, does that mean they're storing the hashes as ASCII representations of the hex bytes? I guess that's at least not insecure, but it's still daft.

    This is what I assumed they were doing. What if they're coming from a database or XML file or something? It's pretty standard to store hashes as ASCII hex numbers.

    That still doesn't explain what the function is even trying to accomplish. I mean, clearly it's copying the same hash into ten different struct fields, then.. comparing it?? And if any of the comparisons fail it.. copies each field of the struct into the next field up?? uncontrollable eye twitch



  • @morbiuswilters said:

    That still doesn't explain what the function is even trying to accomplish.

    Looks to me as if it's trying to enforce Thou Shalt Not Change Thy Password To One Thou Hast Recently Used in the most bone-headed, ham-fisted way possible, with added WTF to promote bowel health.



  • @flabdablet said:

    @morbiuswilters said:
    That still doesn't explain what the function is even trying to accomplish.

    Looks to me as if it's trying to enforce Thou Shalt Not Change Thy Password To One Thou Hast Recently Used in the most bone-headed, ham-fisted way possible, with added WTF to promote bowel health.

    Oh...

    Oh God.



  • I'm also quite fond of the feature where if you've managed to change either of your passwords at least ten times, it starts making all ten history entries for that password the same (but continues to check against all of them, just to be sure to be sure to be sure to be sure to be sure to be sure to be sure to be sure to be sure to be sure) so that from that point on the only one you need to avoid is your immediately previous one. Lovely work.



  • @omega0 said:

    Can't wait until someone decides the history should be 16 and the null derefs start

    Ah, but you've missed the beauty of this technique. You won't see null derefs with this pattern, because the compiler will catch attempts to reference nonexistent struct fields like pwd_config->pwd2_hash16.



  • @flabdablet said:

    I'm also quite fond of the feature where if you've managed to change either of your passwords at least ten times, it starts making all ten history entries for that password the same (but continues to check against all of them, just to be sure to be sure to be sure to be sure to be sure to be sure to be sure to be sure to be sure to be sure) so that from that point on the only one you need to avoid is your immediately previous one. Lovely work.

    Strike that - strcpy is right-to-left, so cases 10+ do actually continue to work.



  • @TGV said:

    This is pretty bad code, from casting new_hash to (char*), taking the strlen every step, NOT adding one to the length the pointless loop, ... WTF is this?

    There isn't a loop, and to be fair the failure to add 1 to the strlen() result before passing it to malloc() is the only actual bug I've found so far.



  • @flabdablet said:

    There isn't a loop, and to be fair the failure to add 1 to the strlen() result before passing it to malloc() is the only actual bug I've found so far.

    ...um... assuming that the hash length will never increase.

    If the hash strings are always the same length, and that length will never change, and it happens not to be one less than a multiple of the block size malloc() rounds its requested allocation size up to, this code will probably fluke its way to working as intended.



  • There, I fixed it

    int change_password(pwd_config_t *pwd_config, uint8_t *new_hash, int pwd_number)
    {
        switch(pwd_number)
        {
            case PWD_NUM_1:
                switch(pwd_config->pwd1_num_hashes)
                {
                    default:
                        if(strcmp((char *)new_hash, pwd_config->pwd1_hash10) == 0) return ERR_MATCH;
                    case 9:
                        if(strcmp((char *)new_hash, pwd_config->pwd1_hash9) == 0) return ERR_MATCH;
                    case 8:
                        if(strcmp((char *)new_hash, pwd_config->pwd1_hash8) == 0) return ERR_MATCH;
                    case 7:
                        if(strcmp((char *)new_hash, pwd_config->pwd1_hash7) == 0) return ERR_MATCH;
                    case 6:
                        if(strcmp((char *)new_hash, pwd_config->pwd1_hash6) == 0) return ERR_MATCH;
                    case 5:
                        if(strcmp((char *)new_hash, pwd_config->pwd1_hash5) == 0) return ERR_MATCH;
                    case 4:
                        if(strcmp((char *)new_hash, pwd_config->pwd1_hash4) == 0) return ERR_MATCH;
                    case 3:
                        if(strcmp((char *)new_hash, pwd_config->pwd1_hash3) == 0) return ERR_MATCH;
                    case 2:
                        if(strcmp((char *)new_hash, pwd_config->pwd1_hash2) == 0) return ERR_MATCH;
                    case 1:
                        if(strcmp((char *)new_hash, pwd_config->pwd1_hash1) == 0) return ERR_MATCH;
                    case 0:
                        ;
                }
                switch (pwd_config->pwd1_num_hashes)
                {
                    default:
                        FREE_IF_NOT_NULL(pwd_config->pwd1_hash10); break;
                    case 8:
                        FREE_IF_NOT_NULL(pwd_config->pwd1_hash9); break;
                    case 7:
                        FREE_IF_NOT_NULL(pwd_config->pwd1_hash8); break;
                    case 6:
                        FREE_IF_NOT_NULL(pwd_config->pwd1_hash7); break;
                    case 5:
                        FREE_IF_NOT_NULL(pwd_config->pwd1_hash6); break;
                    case 4:
                        FREE_IF_NOT_NULL(pwd_config->pwd1_hash5); break;
                    case 3:
                        FREE_IF_NOT_NULL(pwd_config->pwd1_hash4); break;
                    case 2:
                        FREE_IF_NOT_NULL(pwd_config->pwd1_hash3); break;
                    case 1:
                        FREE_IF_NOT_NULL(pwd_config->pwd1_hash2); break;
                    case 0:
                        FREE_IF_NOT_NULL(pwd_config->pwd1_hash1); break;
                }
                switch (pwd_config->pwd1_num_hashes)
                {
                    default:
                        pwd_config->pwd1_hash10 = pwd_config->pwd1_hash9;
                    case 8:
                        pwd_config->pwd1_hash9 = pwd_config->pwd1_hash8;
                    case 7:
                        pwd_config->pwd1_hash8 = pwd_config->pwd1_hash7;
                    case 6:
                        pwd_config->pwd1_hash7 = pwd_config->pwd1_hash6;
                    case 5:
                        pwd_config->pwd1_hash6 = pwd_config->pwd1_hash5;
                    case 4:
                        pwd_config->pwd1_hash5 = pwd_config->pwd1_hash4;
                    case 3:
                        pwd_config->pwd1_hash4 = pwd_config->pwd1_hash3;
                    case 2:
                        pwd_config->pwd1_hash3 = pwd_config->pwd1_hash2;
                    case 1:
                        pwd_config->pwd1_hash2 = pwd_config->pwd1_hash1;
                    case 0:
                        pwd_config->pwd1_hash1 = malloc(strlen((char *)new_hash) + 1);
                        strcpy(pwd_config->pwd1_hash1, (char *)new_hash);
                        pwd_config->pwd1_num_hashes++;
                }
                break;
    
            case PWD_NUM_2:
                switch(pwd_config->pwd2_num_hashes)
                {
                    default:
                        if(strcmp((char *)new_hash, pwd_config->pwd2_hash10) == 0) return ERR_MATCH;
                    case 9:
                        if(strcmp((char *)new_hash, pwd_config->pwd2_hash9) == 0) return ERR_MATCH;
                    case 8:
                        if(strcmp((char *)new_hash, pwd_config->pwd2_hash8) == 0) return ERR_MATCH;
                    case 7:
                        if(strcmp((char *)new_hash, pwd_config->pwd2_hash7) == 0) return ERR_MATCH;
                    case 6:
                        if(strcmp((char *)new_hash, pwd_config->pwd2_hash6) == 0) return ERR_MATCH;
                    case 5:
                        if(strcmp((char *)new_hash, pwd_config->pwd2_hash5) == 0) return ERR_MATCH;
                    case 4:
                        if(strcmp((char *)new_hash, pwd_config->pwd2_hash4) == 0) return ERR_MATCH;
                    case 3:
                        if(strcmp((char *)new_hash, pwd_config->pwd2_hash3) == 0) return ERR_MATCH;
                    case 2:
                        if(strcmp((char *)new_hash, pwd_config->pwd2_hash2) == 0) return ERR_MATCH;
                    case 1:
                        if(strcmp((char *)new_hash, pwd_config->pwd2_hash1) == 0) return ERR_MATCH;
                    case 0:
                        ;
                }
                switch (pwd_config->pwd2_num_hashes)
                {
                    default:
                        FREE_IF_NOT_NULL(pwd_config->pwd2_hash10); break;
                    case 8:
                        FREE_IF_NOT_NULL(pwd_config->pwd2_hash9); break;
                    case 7:
                        FREE_IF_NOT_NULL(pwd_config->pwd2_hash8); break;
                    case 6:
                        FREE_IF_NOT_NULL(pwd_config->pwd2_hash7); break;
                    case 5:
                        FREE_IF_NOT_NULL(pwd_config->pwd2_hash6); break;
                    case 4:
                        FREE_IF_NOT_NULL(pwd_config->pwd2_hash5); break;
                    case 3:
                        FREE_IF_NOT_NULL(pwd_config->pwd2_hash4); break;
                    case 2:
                        FREE_IF_NOT_NULL(pwd_config->pwd2_hash3); break;
                    case 1:
                        FREE_IF_NOT_NULL(pwd_config->pwd2_hash2); break;
                    case 0:
                        FREE_IF_NOT_NULL(pwd_config->pwd2_hash1); break;
                }
                switch (pwd_config->pwd2_num_hashes)
                {
                    default:
                        pwd_config->pwd2_hash10 = pwd_config->pwd2_hash9;
                    case 8:
                        pwd_config->pwd2_hash9 = pwd_config->pwd2_hash8;
                    case 7:
                        pwd_config->pwd2_hash8 = pwd_config->pwd2_hash7;
                    case 6:
                        pwd_config->pwd2_hash7 = pwd_config->pwd2_hash6;
                    case 5:
                        pwd_config->pwd2_hash6 = pwd_config->pwd2_hash5;
                    case 4:
                        pwd_config->pwd2_hash5 = pwd_config->pwd2_hash4;
                    case 3:
                        pwd_config->pwd2_hash4 = pwd_config->pwd2_hash3;
                    case 2:
                        pwd_config->pwd2_hash3 = pwd_config->pwd2_hash2;
                    case 1:
                        pwd_config->pwd2_hash2 = pwd_config->pwd2_hash1;
                    case 0:
                        pwd_config->pwd2_hash1 = malloc(strlen((char *)new_hash) + 1);
                        strcpy(pwd_config->pwd2_hash1, (char *)new_hash);
                        pwd_config->pwd2_num_hashes++;
                }
        }
    
        return ERR_OK;
    }

    I've kept the original idiotic data structure and tried to stay true to the spirit of the thing while adding a mass of lint warnings for your entertainment.

    Note that with this version, the most recent passwords are always in pwd_config->pwd?_hash1, which might eliminate a few switch statements elsewhere in your code base. Sorry about that.



  • @flabdablet said:

    I've kept
     

    Had some time to kill, didya?



  • Re: There, I fixed it (Fuck Whitespace remix)

    int change_password(pwd_config_t *pwd_config, uint8_t *new_hash, int pwd_number)
    {
        switch(pwd_number)
        {
            case PWD_NUM_1:
                switch(pwd_config->pwd1_num_hashes)
                {
                    default: if(strcmp((char *)new_hash, pwd_config->pwd1_hash10) == 0) return ERR_MATCH;
                    case 9:  if(strcmp((char *)new_hash, pwd_config->pwd1_hash9)  == 0) return ERR_MATCH;
                    case 8:  if(strcmp((char *)new_hash, pwd_config->pwd1_hash8)  == 0) return ERR_MATCH;
                    case 7:  if(strcmp((char *)new_hash, pwd_config->pwd1_hash7)  == 0) return ERR_MATCH;
                    case 6:  if(strcmp((char *)new_hash, pwd_config->pwd1_hash6)  == 0) return ERR_MATCH;
                    case 5:  if(strcmp((char *)new_hash, pwd_config->pwd1_hash5)  == 0) return ERR_MATCH;
                    case 4:  if(strcmp((char *)new_hash, pwd_config->pwd1_hash4)  == 0) return ERR_MATCH;
                    case 3:  if(strcmp((char *)new_hash, pwd_config->pwd1_hash3)  == 0) return ERR_MATCH;
                    case 2:  if(strcmp((char *)new_hash, pwd_config->pwd1_hash2)  == 0) return ERR_MATCH;
                    case 1:  if(strcmp((char *)new_hash, pwd_config->pwd1_hash1)  == 0) return ERR_MATCH;
                    case 0:  ;
                }
                switch (pwd_config->pwd1_num_hashes)
                {
                    default: FREE_IF_NOT_NULL(pwd_config->pwd1_hash10); break;
                    case 8:  FREE_IF_NOT_NULL(pwd_config->pwd1_hash9);  break;
                    case 7:  FREE_IF_NOT_NULL(pwd_config->pwd1_hash8);  break;
                    case 6:  FREE_IF_NOT_NULL(pwd_config->pwd1_hash7);  break;
                    case 5:  FREE_IF_NOT_NULL(pwd_config->pwd1_hash6);  break;
                    case 4:  FREE_IF_NOT_NULL(pwd_config->pwd1_hash5);  break;
                    case 3:  FREE_IF_NOT_NULL(pwd_config->pwd1_hash4);  break;
                    case 2:  FREE_IF_NOT_NULL(pwd_config->pwd1_hash3);  break;
                    case 1:  FREE_IF_NOT_NULL(pwd_config->pwd1_hash2);  break;
                    case 0:  FREE_IF_NOT_NULL(pwd_config->pwd1_hash1);  break;
                }
                switch (pwd_config->pwd1_num_hashes)
                {
                    default: pwd_config->pwd1_hash10 = pwd_config->pwd1_hash9;
                    case 8:  pwd_config->pwd1_hash9  = pwd_config->pwd1_hash8;
                    case 7:  pwd_config->pwd1_hash8  = pwd_config->pwd1_hash7;
                    case 6:  pwd_config->pwd1_hash7  = pwd_config->pwd1_hash6;
                    case 5:  pwd_config->pwd1_hash6  = pwd_config->pwd1_hash5;
                    case 4:  pwd_config->pwd1_hash5  = pwd_config->pwd1_hash4;
                    case 3:  pwd_config->pwd1_hash4  = pwd_config->pwd1_hash3;
                    case 2:  pwd_config->pwd1_hash3  = pwd_config->pwd1_hash2;
                    case 1:  pwd_config->pwd1_hash2  = pwd_config->pwd1_hash1;
                    case 0:
                        pwd_config->pwd1_hash1  = malloc(strlen((char *)new_hash) + 1);
                        strcpy(pwd_config->pwd1_hash1, (char *)new_hash);
                        pwd_config->pwd1_num_hashes++;
                }
                break;
    
            case PWD_NUM_2:
                switch(pwd_config->pwd2_num_hashes)
                {
                    default: if(strcmp((char *)new_hash, pwd_config->pwd2_hash10) == 0) return ERR_MATCH;
                    case 9:  if(strcmp((char *)new_hash, pwd_config->pwd2_hash9)  == 0) return ERR_MATCH;
                    case 8:  if(strcmp((char *)new_hash, pwd_config->pwd2_hash8)  == 0) return ERR_MATCH;
                    case 7:  if(strcmp((char *)new_hash, pwd_config->pwd2_hash7)  == 0) return ERR_MATCH;
                    case 6:  if(strcmp((char *)new_hash, pwd_config->pwd2_hash6)  == 0) return ERR_MATCH;
                    case 5:  if(strcmp((char *)new_hash, pwd_config->pwd2_hash5)  == 0) return ERR_MATCH;
                    case 4:  if(strcmp((char *)new_hash, pwd_config->pwd2_hash4)  == 0) return ERR_MATCH;
                    case 3:  if(strcmp((char *)new_hash, pwd_config->pwd2_hash3)  == 0) return ERR_MATCH;
                    case 2:  if(strcmp((char *)new_hash, pwd_config->pwd2_hash2)  == 0) return ERR_MATCH;
                    case 1:  if(strcmp((char *)new_hash, pwd_config->pwd2_hash1)  == 0) return ERR_MATCH;
                    case 0:  ;
                }
                switch (pwd_config->pwd2_num_hashes)
                {
                    default: FREE_IF_NOT_NULL(pwd_config->pwd2_hash10); break;
                    case 8:  FREE_IF_NOT_NULL(pwd_config->pwd2_hash9);  break;
                    case 7:  FREE_IF_NOT_NULL(pwd_config->pwd2_hash8);  break;
                    case 6:  FREE_IF_NOT_NULL(pwd_config->pwd2_hash7);  break;
                    case 5:  FREE_IF_NOT_NULL(pwd_config->pwd2_hash6);  break;
                    case 4:  FREE_IF_NOT_NULL(pwd_config->pwd2_hash5);  break;
                    case 3:  FREE_IF_NOT_NULL(pwd_config->pwd2_hash4);  break;
                    case 2:  FREE_IF_NOT_NULL(pwd_config->pwd2_hash3);  break;
                    case 1:  FREE_IF_NOT_NULL(pwd_config->pwd2_hash2);  break;
                    case 0:  FREE_IF_NOT_NULL(pwd_config->pwd2_hash1);  break;
                }
                switch (pwd_config->pwd2_num_hashes)
                {
                    default: pwd_config->pwd2_hash10 = pwd_config->pwd2_hash9;
                    case 8:  pwd_config->pwd2_hash9  = pwd_config->pwd2_hash8;
                    case 7:  pwd_config->pwd2_hash8  = pwd_config->pwd2_hash7;
                    case 6:  pwd_config->pwd2_hash7  = pwd_config->pwd2_hash6;
                    case 5:  pwd_config->pwd2_hash6  = pwd_config->pwd2_hash5;
                    case 4:  pwd_config->pwd2_hash5  = pwd_config->pwd2_hash4;
                    case 3:  pwd_config->pwd2_hash4  = pwd_config->pwd2_hash3;
                    case 2:  pwd_config->pwd2_hash3  = pwd_config->pwd2_hash2;
                    case 1:  pwd_config->pwd2_hash2  = pwd_config->pwd2_hash1;
                    case 0:
                        pwd_config->pwd2_hash1  = malloc(strlen((char *)new_hash) + 1);
                        strcpy(pwd_config->pwd2_hash1, (char *)new_hash);
                        pwd_config->pwd2_num_hashes++;
                }
                break;
        }
    
        return ERR_OK;
    }


  • @dhromed said:

    @flabdablet said:

    I've kept
     

    Had some time to kill, didya?

    No, I'm very busy and important.


  • Discourse touched me in a no-no place

    I can't help thinking that an array (possibly two dimensional) would reduce the amount of code there.



  • @DaveK said:

    If they're really hashes, how can they be safely passed to all those str* functions when they might contain NUL bytes?

    As long as they're always copied and compared using str* functions, a NUL byte will simply cause the hash to be truncated. It will still compare correctly (although depending on where the first NUL is, it may compare correctly with a lot of other hashes too...). it's only when the hash doesn't contain a NUL that the (non-insidious) problems start.



  • @PJH said:

    I can't help thinking that an array (possibly two dimensional) would reduce the amount of code there.

    Now you're just being silly.



  • Re: FREE_IF_NOT_NULL:

    It expands to the expected if(p){ free(p); }.  While it is defined in a global header, is only used in this one file.  Everywhere else in the codebase they wrote out the if statement.

     

    Also on the topic of ignorance of what the standard library functions actually do, here's their one (and only) use of calloc:

        name = calloc(1,1);
        name[0] = '\0';

    Why??



  • @omega0 said:

    Why??

    Because they suck.

    Here, have a thing to debug.

    int change_password(pwd_config_t *pwd_config, uint8_t *new_hash, int pwd_number)
    {
        /* Marshal braindead *pwd_config parameter into something allowing looping */
    
        #define NUM_PREVIOUS 10
        struct
        {
            int pwd_number;
            int *num_changes;
            char **hashes[NUM_PREVIOUS];
        }
        *pwd, pwds[] =
        {
            {
                PWD_NUM_1,
                &pwd_config->pwd1_num_hashes,
                {
                    &pwd_config->pwd1_hash1, &pwd_config->pwd1_hash2,
                    &pwd_config->pwd1_hash3, &pwd_config->pwd1_hash4,
                    &pwd_config->pwd1_hash5, &pwd_config->pwd1_hash6,
                    &pwd_config->pwd1_hash7, &pwd_config->pwd1_hash8,
                    &pwd_config->pwd1_hash9, &pwd_config->pwd1_hash10
                }
            },
            {
                PWD_NUM_2,
                &pwd_config->pwd2_num_hashes,
                {
                    &pwd_config->pwd2_hash1, &pwd_config->pwd2_hash2,
                    &pwd_config->pwd2_hash3, &pwd_config->pwd2_hash4,
                    &pwd_config->pwd2_hash5, &pwd_config->pwd2_hash6,
                    &pwd_config->pwd2_hash7, &pwd_config->pwd2_hash8,
                    &pwd_config->pwd2_hash9, &pwd_config->pwd2_hash10
                }
            }
        };
    
        #define NUM_PWDS (sizeof pwds / sizeof pwds[0])
        #define MIN(a, b) ((a) < (b) ? (a) : (b))
    
        /* Match pwd_number to find parts of *pwd_config to use */
    
        for (pwd = pwds; pwd < pwds + NUM_PWDS; ++pwd)
        {
            if (pwd->pwd_number == pwd_number)
            {
                /* Disallow change if a previous password hash is matched */
    
                int i;
                int oldest = MIN(*pwd->num_changes, NUM_PREVIOUS) - 1;
                for (i = oldest; i >= 0; --i)
                    if (strcmp((char *)new_hash, *pwd->hashes[i]) == 0)
                        return ERR_MATCH;
    
                /* Allocate memory for new password hash */
    
                char *hash = malloc(strlen((char *)new_hash) + 1);
                if (hash == NULL) return ERR_MALLOC;
                strcpy(hash, (char *)new_hash);
    
                /* Age previous password hashes */
    
                if (oldest < NUM_PREVIOUS - 1) ++oldest;
                FREE_IF_NOT_NULL(*pwd->hashes[oldest]);
                for (i = oldest; i > 0; --i)
                    *pwd->hashes[i] = *pwd->hashes[i - 1];
                
                /* Save change */
                    
                *pwd->hashes[0] = hash;
                ++*pwd->num_changes;
                break;
            }
        }
        return ERR_OK;
    }


  • @omega0 said:

    Re: FREE_IF_NOT_NULL:

    It expands to the expected if(p){ free(p); }.  While it is defined in a global header, is only used in this one file.  Everywhere else in the codebase they wrote out the if statement.

    But why?? free() isn't going to error when you pass it a null.



  • If it did (free(p), p = 0) it might prevent a double-free once in a blue moon, but then its name would be misleading.


  • Discourse touched me in a no-no place

    @omega0 said:

    It expands to the expected if(p){ free(p); }.
    One or, more likely, more people at your shop don't seem to know what they're doing
    @omega0 said:
    Everywhere else in the codebase they wrote out the if statement.
    Definitely more than one then.



    If they're getting something as simple as free() wrong, I dread to think what they're doing with more complicated stuff...
    @omega0 said:
        name = calloc(1,1);
    name[0] = '\0';
    I see. That's just plain scary. Ignoring the fact they're duplicating initialising the memory, they're expecting to store a whole name in a single byte? Is this one of those weird machines where CHAR_BIT is something silly like 4096 instead of 8?



  • @flabdablet said:

    If it did (free(p), p = 0) it might prevent a double-free once in a blue moon, but then its name would be misleading.

    Here's what I do in a macro:

    if (!p) { // kill the program }

    free(p);

    p = 0;

    Because I don't want a double-free to ever happen.



  • @morbiuswilters said:

    @flabdablet said:

    If it did (free(p), p = 0) it might prevent a double-free once in a blue moon, but then its name would be misleading.

    Here's what I do in a macro:

    if (!p) { // kill the program }

    free(p);

    p = 0;

    Because I don't want a double-free to ever happen.

    void break_despite_precautions(void) {
        char *determined = malloc(1000);
        char *idiot = determined;
        MORBS_FREE(determined);
        MORBS_FREE(idiot);
    }

Log in to reply