On Thu, Nov 19, 2020 at 02:05:35PM +0100, Daniel Gustafsson wrote: > IIUC, this patchset moves the allocation of the context inside the API rather > than having the caller be responsible for providing the memory (and thus be > able to use the stack). This in turn changes the API to returning int rather > than being void. > > As an effect of this we get the below types of statements that aren't easy on > the eyes: > > [... code ...]
We use this style in other places of the code. Slitting each part can also be done, if that's easier for the eye. Personal taste likely ;) > This seems like a complication which brings little benefit since the only real > errorcase is OOM in creating the context? The built-in crypto support is > designed to never fail, and reading the OpenSSL code the only failure cases > are > ENGINE initialization (which we don't do) and memory allocation. Did you > consider using EVP_MD_CTX_init instead which place the memory allocation > responsibility with the caller? Keeping this a void API and leaving the > caller > to decide on memory allocation would make the API a lot simpler IMHO. Yes. That's actually the main take and why EVP callers *have* to let OpenSSL do the allocation: we cannot know the size of EVP_MD_CTX in advance in newer versions, and OpenSSL visibly want to keep the right to do extra allocations if necessary within what's set in the context. This is based on my reads of the upstream code as of crypto/evp/digest.c and crypto/evp/evp.h. evp.h includes env_md_ctx_st for OpenSSL <= 1.0.2, but this has been moved to evp_local.h, header not installed, in newer versions which refers only to the internals of the thing, with ossl_typ.h still making EVP_MD_CTX point to env_md_ctx_st, but it becomes an incomplete type. On top of that, knowing that we still need to deal with the OOM failure handling and pass down the error to the callers playing with SHA2, it feels like the most consistent API to me for the frontend and the backend. If you want, it is easy to test that with stuff like that in cryptohashes.c: + EVP_MD_CTX *ctx; [...] + ctx = palloc(sizeof(EVP_MD_CTX)); + + EVP_MD_CTX_init(ctx); + + EVP_DigestInit_ex(ctx, EVP_sha256(), NULL); + EVP_DigestUpdate(ctx, data, len); + EVP_DigestFinal_ex(ctx, buf, 0); FWIW, this thread has dealt with the problem for pgcrypto: https://www.postgresql.org/message-id/20160701094159.ga17...@msg.df7cb.de > +#ifndef _PG_CRYPTOHASH_H_ > +#define _PG_CRYPTOHASH_H_ > This should probably be CRYPTOHASH_H to be consistent? cryptohash.h sounds like a header file we could find somewhere else, hence the extra PG_. > + status = EVP_DigestInit_ex((EVP_MD_CTX *) ctx->data, > + EVP_sha224(), NULL); > Since we always use the default implementation and never select an ENGINE, why > not use EVP_DigestInit instead? Indeed, let's do that. > +/* Include internals of each implementation here */ > +#include "sha2.c" > Do we really want to implement this by including a .c file? Are there no > other > options you see? That was the least intrusive option I could figure out. Two other options I have thought about: - Compile those fallback implementations independently and publish the internals in a separate header, but nobody is going to need that if we have a generic entry point. - Include the internals of each implementation in cryptohash.c, but this bloats the file with more implementations added (HMAC and MD5 still need to be added on top of SHA2), and it messes up with the existing copyright entries. So splitting and just including them sounds like the cleanest option of the set. -- Michael
signature.asc
Description: PGP signature
