Re: [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
On 09/16/20 16:17, Yao, Jiewen wrote:
Thank you Laszlo.I don't like "B" because in the loop body where you call EvpMdUpdate(),
you may encounter an error from a different source than EvpMdUpdate()
itself. For example, you may be reading data from the network or a disk
file. If fetching the next chunk fails, we'd still want to clean up the
EVP MD context. Therefore, if EvpMdUpdate() would itself invalidate the
context, if it failed, then that would *complicate* the error path. (=
Clean up the context after the loop body *only* if something different
from EvpMdUpdate() failed.)
Yes, this was the first pattern, the one that caused me to say "please
don't do this". In this pattern, the context may exist between "New" and
"Init", and also between "Final" and "Free". Those two states are
useless and only good for causing confusion.
For example, are you allowed to Duplicate a context in those states?
It's best to prevent even the *asking* of that question.
- I cannot comment on mbedtls (2).
- I think the current BaseCryptLib HMAC APIs (3) are not great, and we
should use this opportunity to improve upon them.
- Regarding openssl (1), as I understand it, the situation is different
from edk2. In OpenSSL, EVP_MD_CTX is *not* an opaque type (expressed in
C language terms, it is not an "incomplete" type, but a "complete"
type). Therefore, an expression such as sizeof(EVP_MD_CTX) works.
The consequence is that OpenSSL code itself can use the following style:
In other words, OpenSSL-native code is permitted to know the internals
of EVP_MD_CTX, therefore the OpenSSL-internal code may allocate
EVP_MD_CTX with means that are *different* from EVP_MD_CTX_new() and
The caller of (Init, Final) may use (New, Free) for memory management,
or may use something completely different (for example, local variables).
Therefore, offering the (Init, Final) APIs separately from (New, Free)
But the situation in edk2 -- or any other OpenSSL *consumer* -- is
different. In edk2, EVP_MD_CTX is an opaque type -- in C language terms:
an "incomplete" type. OpenSSL deliberately hides the internals of
typedef struct evp_md_ctx_st EVP_MD_CTX;and the "evp_md_ctx_st" structure type is only defined in
contents -- I think! -- OpenSSL client code cannot, or *should not*,
This means that OpenSSL consumer code can *only* rely on
EVP_MD_CTX_new() and EVP_MD_CTX_free(), for allocating and releasing the
context, respectively. "sizeof(EVP_MD_CTX)" is not permitted, and
defining a local or global variable of type EVP_MD_CTX is also not valid.
This means that the only reason for separating (Init, Final) from (New,
Free) falls away, in OpenSSL consumer code. In OpenSSL consumer code,
there is no reason to keep *both* pairs of APIs.
Please note that, this (very prudent) information hiding / encapsulation
in OpenSSL used to be violated by edk2 in the past, intentionally.
That's what the HmacXxxGetContextSize() APIs were about -- those APIs
forcefully leaked the context sizes to client code, so that client code
in edk2 could perform its own allocation.
But in <https://bugzilla.tianocore.org/show_bug.cgi?id=1792>, we finally
eliminated HmacXxxGetContextSize(). As a part of that, we also
simplified the HMAC API -- note that Init was replaced by SetKey. And
because we were reworking an existent API, I didn't propose very
intrusive changes -- I didn't propose fusing Final and Free, for example.
Now, the EVP MD APIs are just being introduced to edk2. So we have a
chance at getting them right, and making them minimal.
Second, I believe EVP is just something in openssl. It does not mean that we *have to* design API to follow openssl.I have no comment on this.
We can do similar thing with Hmac, to deprecate Sha256GetContextSize() API,Yes, good idea.
and replace caller with Sha256New() / Sha384Free()Indeed.
But then -- there's no reason left for keeping (New, Free) separate from