Skip to main content
Code Review

Return to Answer

added 92 characters in body
Source Link
chux
chux
  • 33.7k
  • 2
  • 38
  • 92
// (c->blk)[56] = 0;
...
// (c->blk)[63] = (uint8_t)(c->msglen << 3);

uint64_t bit_len = c->msglen>msglen;
bit_len **= 8;  // 8 bits per byte
// I like doing the above in 2 lines in case `c->msglen` is narrowed.

for (index = 63; index >= 56; index--) {
  (c->blk)[index] = (uint8_t)bit_len;
  bit_len /= 256;
}

No need to save a few byesbytes. Let blklen be unsigned rather than uint8_t.

// (c->blk)[56] = 0;
...
// (c->blk)[63] = (uint8_t)(c->msglen << 3);

uint64_t bit_len = c->msglen * 8;  // 8 bits per byte
for (index = 63; index >= 56; index--) {
  (c->blk)[index] = (uint8_t)bit_len;
  bit_len /= 256;
}

No need to save a few byes. Let blklen be unsigned rather than uint8_t.

// (c->blk)[56] = 0;
...
// (c->blk)[63] = (uint8_t)(c->msglen << 3);

uint64_t bit_len = c->msglen;
bit_len *= 8;  // 8 bits per byte
// I like doing the above in 2 lines in case `c->msglen` is narrowed.

for (index = 63; index >= 56; index--) {
  (c->blk)[index] = (uint8_t)bit_len;
  bit_len /= 256;
}

No need to save a few bytes. Let blklen be unsigned rather than uint8_t.

Source Link
chux
chux
  • 33.7k
  • 2
  • 38
  • 92

I am looking for is comments on whether my use of the context struct makes sense in the way it is handled, whether there may be some way around having to dynamically allocate the context behind the scene

The usual work-around it to expose struct ctx_t to provide space. Yet what OP is doing is fine.

Any other general comments are welcome as well.

Code curiously unnecessarily limits the length to 229 even though c->msglen is 64-bit.

Suggested alternative code to use byte lengths up to 261.

// (c->blk)[56] = 0;
...
// (c->blk)[63] = (uint8_t)(c->msglen << 3);

uint64_t bit_len = c->msglen * 8;  // 8 bits per byte
for (index = 63; index >= 56; index--) {
  (c->blk)[index] = (uint8_t)bit_len;
  bit_len /= 256;
}

It is not clear c->blklen is always in the range [0...63] when (c->blk)[(c->blklen)++] = 0x80; is executed. I'd expect cleaner code in sha256_update() that shows this.

if() not needed.

   // if (c)
     free (c);

Alternative code that I find clearer

// ctx *c = NULL;
....
// if (!(c = sha256_init()))
//    return hdefault;

ctx *c = sha256_init();
if (c == NULL) {
    return hdefault;
}

Use size_t rather than uint64_t to prevent accessing outside array range.

// uint8_t *sha256 (const uint8_t *msg, const uint64_t msglen, uint8_t *hash)
uint8_t *sha256(const uint8_t *msg, size_t msglen, uint8_t *hash)

Insure at least 32-bit math. unsigned may be 16 bit.

// W[i] = ((unsigned)blk[i*4    ] << 24)
W[i] = ((uint32_t)blk[i*4    ] << 24)
// or 
W[i] = ((uint_fast32_t)blk[i*4    ] << 24)

No need to save a few byes. Let blklen be unsigned rather than uint8_t.

Pedantic code would check for overflow in sha256_update()

if (msglen > (UINT64_MAX/8 - c->msglen)) Oops();

Unclear why code uses different names sha256d.h and SHA256_DCR_H. I'd expect SHA256D_H

// file sha256d.h
#ifndef _SHA256_DCR_H_  // why CR?