From: Vitaliy Makkoveev Subject: Re: isakmpd: avoid undefined function pointer casts To: Theo Buehler Cc: tech@openbsd.org Date: Fri, 18 Jul 2025 00:27:48 +0300 On Thu, Jul 17, 2025 at 08:39:17AM +0200, Theo Buehler wrote: > On Wed, Jul 16, 2025 at 09:46:22PM +0200, Theo Buehler wrote: > > This is a bit of a stupid diff with lots of ugly boilerplate, but I > > don't see a better way. The problem is the second sentence of > > C99 6.3.2.3, 8: > > > > A pointer to a function of one type may be converted to a pointer to a > > function of another type and back again; the result shall compare equal > > to the original pointer. If a converted pointer is used to call a > > function whose type is not compatible with the pointed-to type, the > > behavior is undefined. > > > > The notion of compatibility for the functions here boils down to > > compatible return type and compatible argument list, which in particular > > means the same number of arguments. See 6.7.5.3, 15 for the gory details. > > There's also a version understandable to a human being lacking the brain > > damage necessary for successfully deciphering the C99 standard's language: > > https://en.cppreference.com/w/c/language/type.html > > > > So calling 'void CKSUM_Final(CKSUM_CTX *);' through the final member of > > struct hash_function is clearly UB since the numbers of parameters don't > > match. If that was all, the fix would be trivial. But the same goes for > > void MD5Final(MD5_CTX *, char *); because MD5_CTX * and void * aren't > > compatible in this sense: by definition, two pointer types are compatible > > if they point to compatible types. Or, if you prefer: void * has looser > > alignment requirements than MD5_CTX *, so they aren't compatible. > > > > The below avoids this by adding wrappers that have the correct types > > and that then propagate their parameters in a defined way. As I said, > > it's stupid and ugly and the one upside apart from appeasing the > > language lawyers I can see is that we no longer have function pointer > > casts. > > > > PS: there is similar code in isakmpd/hash.c. > > Here's the corresponding change for isakmpd: > ok mvs > Index: hash.c > =================================================================== > RCS file: /cvs/src/sbin/isakmpd/hash.c,v > diff -u -p -r1.25 hash.c > --- hash.c 21 Nov 2024 10:07:30 -0000 1.25 > +++ hash.c 17 Jul 2025 06:31:42 -0000 > @@ -43,7 +43,7 @@ void hmac_init(struct hash *, unsigned c > void hmac_final(unsigned char *, struct hash *); > > /* Temporary hash contexts. */ > -static union { > +static union ANY_CTX { > MD5_CTX md5ctx; > SHA1_CTX sha1ctx; > SHA2_CTX sha2ctx; > @@ -54,45 +54,135 @@ static unsigned char digest[HASH_MAX]; > > /* Encapsulation of hash functions. */ > > +static void > +md5_init(union ANY_CTX *ctx) > +{ > + MD5Init(&ctx->md5ctx); > +} > + > +static void > +md5_update(union ANY_CTX *ctx, const unsigned char *data, size_t len) > +{ > + MD5Update(&ctx->md5ctx, data, len); > +} > + > +static void > +md5_final(unsigned char *digest, union ANY_CTX *ctx) > +{ > + MD5Final(digest, &ctx->md5ctx); > +} > + > +static void > +sha1_init(union ANY_CTX *ctx) > +{ > + SHA1Init(&ctx->sha1ctx); > +} > + > +static void > +sha1_update(union ANY_CTX *ctx, const unsigned char *data, size_t len) > +{ > + SHA1Update(&ctx->sha1ctx, data, len); > +} > + > +static void > +sha1_final(unsigned char *digest, union ANY_CTX *ctx) > +{ > + SHA1Final(digest, &ctx->sha1ctx); > +} > + > +static void > +sha256_init(union ANY_CTX *ctx) > +{ > + SHA256Init(&ctx->sha2ctx); > +} > + > +static void > +sha256_update(union ANY_CTX *ctx, const unsigned char *data, size_t len) > +{ > + SHA256Update(&ctx->sha2ctx, data, len); > +} > + > +static void > +sha256_final(unsigned char *digest, union ANY_CTX *ctx) > +{ > + SHA256Final(digest, &ctx->sha2ctx); > +} > + > +static void > +sha384_init(union ANY_CTX *ctx) > +{ > + SHA384Init(&ctx->sha2ctx); > +} > + > +static void > +sha384_update(union ANY_CTX *ctx, const unsigned char *data, size_t len) > +{ > + SHA384Update(&ctx->sha2ctx, data, len); > +} > + > +static void > +sha384_final(unsigned char *digest, union ANY_CTX *ctx) > +{ > + SHA384Final(digest, &ctx->sha2ctx); > +} > + > +static void > +sha512_init(union ANY_CTX *ctx) > +{ > + SHA512Init(&ctx->sha2ctx); > +} > + > +static void > +sha512_update(union ANY_CTX *ctx, const unsigned char *data, size_t len) > +{ > + SHA512Update(&ctx->sha2ctx, data, len); > +} > + > +static void > +sha512_final(unsigned char *digest, union ANY_CTX *ctx) > +{ > + SHA512Final(digest, &ctx->sha2ctx); > +} > + > static struct hash hashes[] = { > { > HASH_MD5, 5, MD5_SIZE, MD5_BLOCK_LENGTH, (void *)&Ctx.md5ctx, digest, > sizeof(MD5_CTX), (void *)&Ctx2.md5ctx, > - (void (*)(void *))MD5Init, > - (void (*)(void *, unsigned char *, unsigned int))MD5Update, > - (void (*)(unsigned char *, void *))MD5Final, > + md5_init, > + md5_update, > + md5_final, > hmac_init, > hmac_final > }, { > HASH_SHA1, 6, SHA1_SIZE, SHA1_BLOCK_LENGTH, (void *)&Ctx.sha1ctx, > digest, sizeof(SHA1_CTX), (void *)&Ctx2.sha1ctx, > - (void (*)(void *))SHA1Init, > - (void (*)(void *, unsigned char *, unsigned int))SHA1Update, > - (void (*)(unsigned char *, void *))SHA1Final, > + sha1_init, > + sha1_update, > + sha1_final, > hmac_init, > hmac_final > }, { > HASH_SHA2_256, 7, SHA2_256_SIZE, SHA256_BLOCK_LENGTH, > (void *)&Ctx.sha2ctx, digest, sizeof(SHA2_CTX), (void *)&Ctx2.sha2ctx, > - (void (*)(void *))SHA256Init, > - (void (*)(void *, unsigned char *, unsigned int))SHA256Update, > - (void (*)(u_int8_t *, void *))SHA256Final, > + sha256_init, > + sha256_update, > + sha256_final, > hmac_init, > hmac_final > }, { > HASH_SHA2_384, 8, SHA2_384_SIZE, SHA384_BLOCK_LENGTH, > (void *)&Ctx.sha2ctx, digest, sizeof(SHA2_CTX), (void *)&Ctx2.sha2ctx, > - (void (*)(void *))SHA384Init, > - (void (*)(void *, unsigned char *, unsigned int))SHA384Update, > - (void (*)(u_int8_t *, void *))SHA384Final, > + sha384_init, > + sha384_update, > + sha384_final, > hmac_init, > hmac_final > }, { > HASH_SHA2_512, 9, SHA2_512_SIZE, SHA512_BLOCK_LENGTH, > (void *)&Ctx.sha2ctx, digest, sizeof(SHA2_CTX), (void *)&Ctx2.sha2ctx, > - (void (*)(void *))SHA512Init, > - (void (*)(void *, unsigned char *, unsigned int))SHA512Update, > - (void (*)(u_int8_t *, void *))SHA512Final, > + sha512_init, > + sha512_update, > + sha512_final, > hmac_init, > hmac_final > } > Index: hash.h > =================================================================== > RCS file: /cvs/src/sbin/isakmpd/hash.h,v > diff -u -p -r1.9 hash.h > --- hash.h 21 Nov 2024 10:07:30 -0000 1.9 > +++ hash.h 17 Jul 2025 06:34:50 -0000 > @@ -49,6 +49,8 @@ enum hashes { > HASH_SHA2_512 > }; > > +union ANY_CTX; > + > struct hash { > enum hashes type; > int id; /* ISAKMP/Oakley ID */ > @@ -58,9 +60,9 @@ struct hash { > unsigned char *digest; /* Pointer to a digest */ > int ctxsize; > void *ctx2; /* Pointer to a 2nd context, for HMAC octx */ > - void (*Init) (void *); > - void (*Update) (void *, unsigned char *, unsigned int); > - void (*Final) (unsigned char *, void *); > + void (*Init) (union ANY_CTX *); > + void (*Update) (union ANY_CTX *, const unsigned char *, size_t); > + void (*Final) (unsigned char *, union ANY_CTX *); > void (*HMACInit) (struct hash *, unsigned char *, unsigned int); > void (*HMACFinal) (unsigned char *, struct hash *); > }; >