Hello!
This could be good as an optional feature, disabled by default so that
extra dependency doesn't get added accidentally. It's too late for
5.4.0 but perhaps in 5.4.1 or .2.
The biggest problem with the patch is that it lacks error checking:
- EVP_MD_CTX_new() can return NULL if memory allocation fails. Man
page doesn't document this but source code makes it clear.
- EVP_get_digestbyname() can return NULL on failure. Perhaps this
could be replaced with EVP_sha256()? It seems to return a pointer
to a statically-allocated structure and man page implies that it
cannot fail.
- EVP_DigestInit_ex(), EVP_DigestUpdate(), and EVP_DigestFinal_ex()
can in theory fail, perhaps not in practice, I don't know.
Currently it is assumed in liblzma that initiazation cannot fail so
that would need to be changed. It could be good to check the return
values from EVP_DigestUpdate() and EVP_DigestFinal_ex() too. Since it
is unlikely that EVP_DigestUpdate() fails it could perhaps be OK to
store the failure code and only return it for lzma_check_finish() but
I'm not sure if that is acceptable.
The configure options perhaps should be --with instead of --enable since
it adds a dependency on another package, if one wants to stick to
Autoconf's guidlines. (It's less clear if --enable-external-sha256
should be --with since it only affects what to use from the OS base
libraries. In any case it won't be changed as it would affect
compatibility with build scripts.)
Are there other good library options? For example, Nettle's SHA-256
functions don't need any error checking but I haven't checked the
performance.
Is it a mess for distributions if a dependency of liblzma gets its
soname bumped and then liblzma needs to be rebuilt without changing its
soname? I suppose such things happen all the time but when a library is
needed by a package manager it might perhaps have extra worries.
--
Lasse Collin
