Remove all pointer casts in C code

[briansmith]

The current shortlist, which I got after grepping for “*)” and doing a bit of filtering, is below. All of them seem pretty easy to eliminate. However, it would be better to find a tool that finds such casts that is smarter than grep, that we can run as a pre-commit hook and that we can run during builds to verify we don't add any new casts, especially when we merge changes from other forks.

I will take a stab at eliminating these soon.

crypto/bn/exponentiation.c(347):  BN_ULONG *table = (BN_ULONG *) buf;
crypto/bn/exponentiation.c(364):  volatile BN_ULONG *table = (volatile BN_ULONG *)buf;
crypto/bn/exponentiation.c(444):  ((unsigned char *)(x_) +               \
crypto/bn/exponentiation.c(552):  tmp.d = (BN_ULONG *)(powerbuf + sizeof(m->d[0]) * top * numPowers);
crypto/bn/exponentiation.c(664):      const uint8_t *p_bytes = (const uint8_t *)p->d;
crypto/bn/exponentiation.c(686):        wvalue = *(const uint16_t *) (p_bytes + (first_bit >> 3));
crypto/bn/internal.h(196):    (BN_ULONG *)x, sizeof(x) / sizeof(BN_ULONG),        \
crypto/bn/montgomery.c(338):    nrp = (BN_ULONG *)(((uintptr_t)rp & ~m) | ((uintptr_t)ap & m));
crypto/bn/random.c(294):                                        (const uint8_t *)attempt,
crypto/bn/random.c(295):                                        sizeof(attempt), (const uint8_t *)done,
crypto/bn/rsaz_exp.c(218):  const uint8_t *p_str = (const uint8_t *)exponent;
crypto/bn/rsaz_exp.c(230):      wvalue = *((const unsigned short*)&p_str[index / 8]);
crypto/ec/p256-64.c(82):  out[0] = *((const u64 *)&in[0]);
crypto/ec/p256-64.c(83):  out[1] = *((const u64 *)&in[8]);
crypto/ec/p256-64.c(84):  out[2] = *((const u64 *)&in[16]);
crypto/ec/p256-64.c(85):  out[3] = *((const u64 *)&in[24]);
crypto/ec/p256-64.c(91):  *((u64 *)&out[0]) = in[0];
crypto/ec/p256-64.c(92):  *((u64 *)&out[8]) = in[1];
crypto/ec/p256-64.c(93):  *((u64 *)&out[16]) = in[2];
crypto/ec/p256-64.c(94):  *((u64 *)&out[24]) = in[3];
crypto/ec/p256-64.c(1414):    const u64 *inlimbs = (const u64 *)&pre_comp[i][0][0];
crypto/ec/p256-64.c(1679):  batch_mul(x_out, y_out, z_out, (const felem_bytearray(*))secrets,
crypto/ec/p256-64.c(1681):            (const smallfelem(*)[17][3])pre_comp);
crypto/ec/p256-x86_64.c(392):        (const PRECOMP256_ROW *)ecp_nistz256_precomputed;
crypto/ecdsa/ecdsa_test.cc(73):  out->reset((uint8_t *)OPENSSL_malloc(der_len));
crypto/modes/gcm.c(153):  nlo = ((const uint8_t *)Xi)[15];
crypto/modes/gcm.c(177):    nlo = ((const uint8_t *)Xi)[cnt];
crypto/modes/gcm.c(211):    nlo = ((const uint8_t *)Xi)[15];
crypto/modes/gcm.c(236):      nlo = ((const uint8_t *)Xi)[cnt];
crypto/poly1305/poly1305.c(108):  struct poly1305_block_state_st *state = (struct poly1305_block_state_st *)ctx;
crypto/poly1305/poly1305.c(147):  struct poly1305_block_state_st *state = (struct poly1305_block_state_st *)ctx;
crypto/poly1305/poly1305.c(211):  struct poly1305_block_state_st *state = (struct poly1305_block_state_st *)ctx;
crypto/poly1305/poly1305.c(271):  struct poly1305_state_st *state = (struct poly1305_state_st *)statep;
crypto/poly1305/poly1305.c(287):  struct poly1305_state_st *state = (struct poly1305_state_st *)statep;
crypto/poly1305/poly1305.c(319):  struct poly1305_state_st *state = (struct poly1305_state_st *)statep;
crypto/rand/rand.c(104):    memcpy((uint8_t *)buf + len_multiple8, rand_buf, len);
crypto/rand/rand.c(172):      buf = (uint8_t *)buf + todo;
crypto/rand/urandom.c(91):    out = (uint8_t *)out + r;
crypto/rand/windows.c(51):    out = (uint8_t *)out + output_bytes_this_pass;
crypto/rsa/blinding.c(137):  ret = (BN_BLINDING*) OPENSSL_malloc(sizeof(BN_BLINDING));
crypto/rsa/padding.c(86):  p = (uint8_t *)to;
crypto/rsa/padding.c(163):  p = (unsigned char *)to;

[briansmith]

See also #152.

[briansmith]

Here's some of the easier ones:
8952537
cf41e87
f2801ad
27950d2
b738c2b

[briansmith]

8cb3191

[briansmith]

c4e7659
febfa61

Also ac6ee42 and 7d5fbab remove the last union type punning from gcm.c.

[briansmith]

My current shortlist of what's left is:

crypto/bn/exponentiation.c(347):  BN_ULONG *table = (BN_ULONG *) buf;
crypto/bn/exponentiation.c(364):  volatile BN_ULONG *table = (volatile BN_ULONG *)buf;
crypto/bn/exponentiation.c(444):  ((unsigned char *)(x_) +               \
crypto/bn/exponentiation.c(552):  tmp.d = (BN_ULONG *)(powerbuf + sizeof(m->d[0]) * top * numPowers);
crypto/bn/exponentiation.c(664):      const uint8_t *p_bytes = (const uint8_t *)p->d;
crypto/bn/exponentiation.c(686):        wvalue = *(const uint16_t *) (p_bytes + (first_bit >> 3));
crypto/bn/internal.h(196):    (BN_ULONG *)x, sizeof(x) / sizeof(BN_ULONG),        \
crypto/bn/montgomery.c(338):    nrp = (BN_ULONG *)(((uintptr_t)rp & ~m) | ((uintptr_t)ap & m));
crypto/bn/rsaz_exp.c(218):  const uint8_t *p_str = (const uint8_t *)exponent;
crypto/bn/rsaz_exp.c(230):      wvalue = *((const unsigned short*)&p_str[index / 8]);
crypto/rand/rand.c(104):    memcpy((uint8_t *)buf + len_multiple8, rand_buf, len);
crypto/rand/rand.c(172):      buf = (uint8_t *)buf + todo;
crypto/rand/urandom.c(91):    out = (uint8_t *)out + r;
crypto/rand/windows.c(51):    out = (uint8_t *)out + output_bytes_this_pass;

The crypto/rand casts are only casting (void *) to (uint8_t *) for the purpose of pointer arithmetic. However, maybe it was a mistake to make them (void *) instead of (uint8_t *). In particular, the C implementation of ChaCha20 does indeed write its output as bytes. I guess I'll undo that change.

[briansmith]

The cast listed above in crypto/bn/internal.h will take more time to resolve. This is the definition of STATIC_BIGNUM and it is a const_cast. I'm not sure there's a great way to remove it and also it is benign even under the most pedantic reading of the C spec. So, we might need to let that one slide.

[briansmith]

The patches for removing these have been written but aren't committed yet, in case the BoringSSL team wants them to be adjusted:

crypto/bn/exponentiation.c(347):  BN_ULONG *table = (BN_ULONG *) buf;
crypto/bn/exponentiation.c(364):  volatile BN_ULONG *table = (volatile BN_ULONG *)buf;
crypto/bn/exponentiation.c(444):  ((unsigned char *)(x_) +               \
crypto/bn/exponentiation.c(552):  tmp.d = (BN_ULONG *)(powerbuf + sizeof(m->d[0]) * top * numPowers);

Current shortlist:

crypto/bn/exponentiation.c(664):      const uint8_t *p_bytes = (const uint8_t *)p->d;
crypto/bn/exponentiation.c(686):        wvalue = *(const uint16_t *) (p_bytes + (first_bit >> 3));
crypto/bn/montgomery.c(338):    nrp = (BN_ULONG *)(((uintptr_t)rp & ~m) | ((uintptr_t)ap & m));
crypto/bn/rsaz_exp.c(218):  const uint8_t *p_str = (const uint8_t *)exponent;
crypto/bn/rsaz_exp.c(230):      wvalue = *((const unsigned short*)&p_str[index / 8]);

Excluded because they are better than alternatives, AFAICT:

crypto/bn/internal.h(196):    (BN_ULONG *)x, sizeof(x) / sizeof(BN_ULONG),        \
crypto/rand/urandom.c(91):    out = (uint8_t *)out + r;
crypto/rand/windows.c(51):    out = (uint8_t *)out + output_bytes_this_pass;

[briansmith]

We've made a lot of progress that has replaced a lot of C code. Here's the updated list:

crypto/cpu-arm-linux.c(71): out = (uint8_t *)out + ret;

This code will be replaced as in #357. (It only affects 32-bit ARM Linux targets.)

crypto/bn/montgomery.c(267): nrp = (BN_ULONG *)(((uintptr_t)rp & ~m) | ((uintptr_t)ap & m));

This will get replaced as a side effect of improving the Montgomery math code.

crypto/cipher/e_aes.c(247): GFp_gcm128_init_serialized((uint8_t *)ctx_buf + sizeof(ks), &ks, aes_block());
crypto/cipher/e_aes.c(257): GFp_gcm128_init(gcm, ks, aes_block(), (const uint8_t *)ctx_buf + sizeof(*ks),

These are (void *) to uint8_t casts purely for the sake of doing pointer arithmetic. But, they probably should be replaced anyway.

crypto/ec\ecp_nistz256.c(226): (const PRECOMP256_ROW *)GFp_nistz256_precomputed;

It would be straightforward to fix this by changing the type of GFp_nistz256_precomputed to be an array of PRECOMP256_ROWs. That type is defined as:

typedef P256_POINT_AFFINE PRECOMP256_ROW[64];

Question: Is it guaranteed that sizeof(P256_POINT_AFFINE) * 64 == sizeof(PRECOMP256_ROW)? Maybe it doesn't matter because we could static_assert that that is the case.

crypto/bn/rsaz_exp.c(220): const uint8_t *p_str = (const uint8_t *)exponent;
crypto/bn/rsaz_exp.c(232): wvalue = ((const unsigned short)&p_str[index / 8]);
crypto/bn/exponentiation.c(333): BN_ULONG *table = (BN_ULONG *) buf;
crypto/bn/exponentiation.c(350): volatile BN_ULONG *table = (volatile BN_ULONG *)buf;
crypto/bn/exponentiation.c(430): ((unsigned char *)(x_) +
crypto/bn/exponentiation.c(518): tmp.d = (BN_ULONG *)(powerbuf + sizeof(m->d[0]) * top * numPowers);
crypto/bn/exponentiation.c(628): const uint8_t *p_bytes = (const uint8_t *)p->d;
crypto/bn/exponentiation.c(650): wvalue = *(const uint16_t *) (p_bytes + (first_bit >> 3));

Basically, we need to write a function that, given GFp_Limb[] and a bit index i, extracts n bits starting at bit index i, then use it in this code.

[briansmith]

Done.