I have no idea what could possibly be bugged

Question

So I'm trying to write my own "keccaksum" program, except running

for i in {1..50}; do ./keccaksum 256 test.o; done

outputs

4d4cc035e544cd4837b45550094dd3c419e380af3b0c74109c00053c7ed82040  test.o

most of the time and

b19d21947b7228da366b4d26f232b87e21999ff1a220c37c9bed6553260068c0  test.o

some of the time.

The relevant function(s) here is

void printsum(const char *_fname, FILE *_f, size_t size){   
    uint64_t state[25];
    uint8_t *hash = malloc(size * sizeof(uint8_t));
    uint8_t buf[25];
    uint8_t tmp[144];
    register int i, rsize, rsizew;
    register size_t j;

    rsize = 200 - 2 * size;
    rsizew = rsize / 8;

    //Clear the state
    memset(state, 0, sizeof(state));

    while(1) {
        //read up to rsize bytes, then do work
        j = fread(buf, 1, rsize, _f);

        //check some stuff
        if(feof(_f)){
            break;
        } else if(ferror(_f)){
            fprintf(stderr, "Error when reading %s.\n", _fname);
            goto fin;
        } else {
            //First few blocks (i.e. not last block)
            for(i = 0; i < rsizew; i++)
                state[i] ^= ((uint64_t *)buf)[i];
            keccakf(state, KECCAK_ROUNDS);
        }
    }

    //Last block + padding
    memcpy(tmp, buf, j);
    tmp[j++] = 1;
    memset(tmp + j, 0, rsize - j);
    tmp[rsize - 1] |= 0x80;

    for(i = 0; i < rsizew; i++)
        state[i] ^= ((uint64_t *)tmp)[i];
    keccakf(state, KECCAK_ROUNDS);

    //copy hash
    memcpy(hash, state, size);

    //print
    for(i = 0; i < size; i++) printf("%02x", hash[i]);
    printf("  %s\n", _fname);

fin:    
    if(_f != stdin) fclose(_f);
    free(hash);
}

This leads me to believe that some part of this code is undefined and I have no idea what could possibly be undefined.

Answer 1

First of all replace this:

if(feof(_f)){
        break;
    } else if(ferror(_f)){
        fprintf(stderr, "Error when reading %s.\n", _fname);
        goto fin;
    } else {
        for(i = 0; .....

with this:

if ( j < rsize )
    break;

for (i = 0; .......

If you care about which error message to show, you can check ferror after the loop.

Next, this may cause undefined behaviour depending on your system's alignment requirements:

state[i] ^= ((uint64_t *)buf)[i];

To be safe you could replace this with:

{
    uint64_t temp;
    memcpy(&temp, buf + i * sizeof temp, sizeof temp);
    state[i] ^= temp;
}

However it's not clear to me whether this is the correct behaviour for the keccak algorithm if your system is little-endian; nor if this is correct in the case that size is not an exact multiple of 8.

There are various other errors which may occur for certain values of size. You made it difficult by not specifying which values of size are causing the problem. It would be very helpful if you update your post to show a full program. (i.e. something that someone else can compile unchanged to reproduce the problem). In the meantime:

If size > 100 then this will go haywire.

For small values of size this is a buffer overflow:

uint8_t tmp[144];
memcpy(tmp, buf, j);
tmp[j++] = 1;
memset(tmp + j, 0, rsize - j);
tmp[rsize - 1] |= 0x80;

because rsize could be up to 200 and this will overflow tmp. Also, think about whether you want to execute this block in the case j == 0.

In this line memcpy(hash, state, size); it's weird that the number of bytes you operate on is rsizew * 8, but the number of bytes you copy is size, these will not be the same in general.

I'll update my post after you post the full program, to take into account the actual values of size in use.

NB. Is this supposed to be SHA-3?