Cristian Rodríguez
2012-May-05 21:34 UTC
[flac-dev] [PATCH] Optionally, allow distros to use openssl for MD5 verification
This has the advantage of being more efficient than the included
routines and allows distros to centralize crypto mainteniance on
a few libraries.
---
configure.ac | 4 +-
m4/ax_check_openssl.m4 | 124 +++++++++++++++++++++++++++++++++++++
src/libFLAC/Makefile.am | 2 +-
src/libFLAC/include/private/md5.h | 8 ++-
src/libFLAC/md5.c | 38 +++++++++++
src/libFLAC/stream_decoder.c | 30 +++++++--
src/libFLAC/stream_encoder.c | 30 +++++++--
7 files changed, 220 insertions(+), 16 deletions(-)
create mode 100644 m4/ax_check_openssl.m4
diff --git a/configure.ac b/configure.ac
index e794ca2..3b627e5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -56,7 +56,7 @@ AM_PROG_CC_C_O
AC_C_INLINE
AC_C_VARARRAYS
AC_C_TYPEOF
-
+AC_FUNC_ALLOCA
AC_CHECK_HEADERS(stdint.h)
AC_SUBST(HAVE_STDINT_H)
AC_CHECK_HEADERS(inttypes.h)
@@ -83,6 +83,8 @@ dnl check for getopt in standard library
dnl AC_CHECK_FUNCS(getopt_long , , [LIBOBJS="$LIBOBJS getopt.o
getopt1.o"] )
AC_CHECK_FUNCS(getopt_long, [], [])
+AX_CHECK_OPENSSL([AC_DEFINE([HAVE_OPENSSL], [1], [We have openSSL])])
+
case "$host_cpu" in
i*86)
cpu_ia32=true
diff --git a/m4/ax_check_openssl.m4 b/m4/ax_check_openssl.m4
new file mode 100644
index 0000000..a87c5a6
--- /dev/null
+++ b/m4/ax_check_openssl.m4
@@ -0,0 +1,124 @@
+# ==========================================================================+#
http://www.gnu.org/software/autoconf-archive/ax_check_openssl.html
+# ==========================================================================+#
+# SYNOPSIS
+#
+# AX_CHECK_OPENSSL([action-if-found[, action-if-not-found]])
+#
+# DESCRIPTION
+#
+# Look for OpenSSL in a number of default spots, or in a user-selected
+# spot (via --with-openssl). Sets
+#
+# OPENSSL_INCLUDES to the include directives required
+# OPENSSL_LIBS to the -l directives required
+# OPENSSL_LDFLAGS to the -L or -R flags required
+#
+# and calls ACTION-IF-FOUND or ACTION-IF-NOT-FOUND appropriately
+#
+# This macro sets OPENSSL_INCLUDES such that source files should use the
+# openssl/ directory in include directives:
+#
+# #include <openssl/hmac.h>
+#
+# LICENSE
+#
+# Copyright (c) 2009,2010 Zmanda Inc. <http://www.zmanda.com/>
+# Copyright (c) 2009,2010 Dustin J. Mitchell <dustin at zmanda.com>
+#
+# Copying and distribution of this file, with or without modification, are
+# permitted in any medium without royalty provided the copyright notice
+# and this notice are preserved. This file is offered as-is, without any
+# warranty.
+
+#serial 8
+
+AU_ALIAS([CHECK_SSL], [AX_CHECK_OPENSSL])
+AC_DEFUN([AX_CHECK_OPENSSL], [
+ found=false
+ AC_ARG_WITH([openssl],
+ [AS_HELP_STRING([--with-openssl=DIR],
+ [root of the OpenSSL directory])],
+ [
+ case "$withval" in
+ "" | y | ye | yes | n | no)
+ AC_MSG_ERROR([Invalid --with-openssl value])
+ ;;
+ *) ssldirs="$withval"
+ ;;
+ esac
+ ], [
+ # if pkg-config is installed and openssl has installed a .pc file,
+ # then use that information and don't search ssldirs
+ AC_PATH_PROG([PKG_CONFIG], [pkg-config])
+ if test x"$PKG_CONFIG" != x""; then
+ OPENSSL_LDFLAGS=`$PKG_CONFIG openssl --libs-only-L
2>/dev/null`
+ if test $? = 0; then
+ OPENSSL_LIBS=`$PKG_CONFIG openssl --libs-only-l
2>/dev/null`
+ OPENSSL_INCLUDES=`$PKG_CONFIG openssl --cflags-only-I
2>/dev/null`
+ found=true
+ fi
+ fi
+
+ # no such luck; use some default ssldirs
+ if ! $found; then
+ ssldirs="/usr/local/ssl /usr/lib/ssl /usr/ssl /usr/pkg
/usr/local /usr"
+ fi
+ ]
+ )
+
+
+ # note that we #include <openssl/foo.h>, so the OpenSSL headers have
to be in
+ # an 'openssl' subdirectory
+
+ if ! $found; then
+ OPENSSL_INCLUDES+ for ssldir in $ssldirs; do
+ AC_MSG_CHECKING([for openssl/ssl.h in $ssldir])
+ if test -f "$ssldir/include/openssl/ssl.h"; then
+ OPENSSL_INCLUDES="-I$ssldir/include"
+ OPENSSL_LDFLAGS="-L$ssldir/lib"
+ OPENSSL_LIBS="-lssl -lcrypto"
+ found=true
+ AC_MSG_RESULT([yes])
+ break
+ else
+ AC_MSG_RESULT([no])
+ fi
+ done
+
+ # if the file wasn't found, well, go ahead and try the link anyway
-- maybe
+ # it will just work!
+ fi
+
+ # try the preprocessor and linker with our new flags,
+ # being careful not to pollute the global LIBS, LDFLAGS, and CPPFLAGS
+
+ AC_MSG_CHECKING([whether compiling and linking against OpenSSL works])
+ echo "Trying link with OPENSSL_LDFLAGS=$OPENSSL_LDFLAGS;" \
+ "OPENSSL_LIBS=$OPENSSL_LIBS;
OPENSSL_INCLUDES=$OPENSSL_INCLUDES" >&AS_MESSAGE_LOG_FD
+
+ save_LIBS="$LIBS"
+ save_LDFLAGS="$LDFLAGS"
+ save_CPPFLAGS="$CPPFLAGS"
+ LDFLAGS="$LDFLAGS $OPENSSL_LDFLAGS"
+ LIBS="$OPENSSL_LIBS $LIBS"
+ CPPFLAGS="$OPENSSL_INCLUDES $CPPFLAGS"
+ AC_LINK_IFELSE(
+ [AC_LANG_PROGRAM([#include <openssl/ssl.h>], [SSL_new(NULL)])],
+ [
+ AC_MSG_RESULT([yes])
+ $1
+ ], [
+ AC_MSG_RESULT([no])
+ $2
+ ])
+ CPPFLAGS="$save_CPPFLAGS"
+ LDFLAGS="$save_LDFLAGS"
+ LIBS="$save_LIBS"
+
+ AC_SUBST([OPENSSL_INCLUDES])
+ AC_SUBST([OPENSSL_LIBS])
+ AC_SUBST([OPENSSL_LDFLAGS])
+])
diff --git a/src/libFLAC/Makefile.am b/src/libFLAC/Makefile.am
index 134168e..2143ad2 100644
--- a/src/libFLAC/Makefile.am
+++ b/src/libFLAC/Makefile.am
@@ -78,7 +78,7 @@ endif
endif
endif
-libFLAC_la_LIBADD = $(LOCAL_EXTRA_LIBADD) @OGG_LIBS@ -lm
+libFLAC_la_LIBADD = $(LOCAL_EXTRA_LIBADD) @OPENSSL_LIBS@ @OGG_LIBS@ -lm
SUBDIRS = $(ARCH_SUBDIRS) include .
diff --git a/src/libFLAC/include/private/md5.h
b/src/libFLAC/include/private/md5.h
index e5f675a..5b42a27 100644
--- a/src/libFLAC/include/private/md5.h
+++ b/src/libFLAC/include/private/md5.h
@@ -28,6 +28,11 @@
#include "FLAC/ordinals.h"
+#if defined(HAVE_OPENSSL)
+#include <openssl/evp.h>
+#define FLAC__MD5Context EVP_MD_CTX
+#else
+#define EVP_MAX_MD_SIZE 16
typedef struct {
FLAC__uint32 in[16];
FLAC__uint32 buf[4];
@@ -37,7 +42,8 @@ typedef struct {
} FLAC__MD5Context;
void FLAC__MD5Init(FLAC__MD5Context *context);
-void FLAC__MD5Final(FLAC__byte digest[16], FLAC__MD5Context *context);
+void FLAC__MD5Final(FLAC__byte digest[EVP_MAX_MD_SIZE], FLAC__MD5Context
*context);
+#endif
FLAC__bool FLAC__MD5Accumulate(FLAC__MD5Context *ctx, const FLAC__int32 * const
signal[], unsigned channels, unsigned samples, unsigned bytes_per_sample);
diff --git a/src/libFLAC/md5.c b/src/libFLAC/md5.c
index e251d50..81439d5 100644
--- a/src/libFLAC/md5.c
+++ b/src/libFLAC/md5.c
@@ -5,6 +5,19 @@
#include <stdlib.h> /* for malloc() */
#include <string.h> /* for memcpy() */
+#ifdef HAVE_ALLOCA_H
+# include <alloca.h>
+#elif !defined alloca
+# ifdef __GNUC__
+# define alloca __builtin_alloca
+# elif defined _AIX
+# define alloca __alloca
+# elif defined _MSC_VER
+# include <malloc.h>
+# define alloca _alloca
+# endif
+#endif
+
#include "private/md5.h"
#include "share/alloc.h"
@@ -35,6 +48,7 @@
/* The four core functions - F1 is optimized somewhat */
+#if !defined(HAVE_OPENSSL)
/* #define F1(x, y, z) (x & y | ~x & z) */
#define F1(x, y, z) (z ^ (x & (y ^ z)))
#define F2(x, y, z) F1(z, x, y)
@@ -267,6 +281,8 @@ void FLAC__MD5Final(FLAC__byte digest[16], FLAC__MD5Context
*ctx)
memset(ctx, 0, sizeof(*ctx)); /* In case it's sensitive */
}
+#endif /* !defined(HAVE_OPENSSL) */
+
/*
* Convert the incoming audio signal to a byte stream
*/
@@ -401,6 +417,26 @@ FLAC__bool FLAC__MD5Accumulate(FLAC__MD5Context *ctx, const
FLAC__int32 * const
if((size_t)channels * (size_t)bytes_per_sample > SIZE_MAX /
(size_t)samples)
return false;
+#if defined(HAVE_OPENSSL)
+ /* Use stack for the most common cases, heap when bytes_needed is larger
than 4032 (unlikely)
+ * Note that this is a _very_ conservative estimation.
+ */
+#if defined(_MSC_VER)
+/* see http://msdn.microsoft.com/en-us/library/5471dc8s(v=vs.80).aspx for the
rationale */
+ FLAC__byte *tmp = _malloca(bytes_needed);
+#else
+ const FLAC__bool usealloca = (bytes_needed < 4032);
+ FLAC__byte *tmp = usealloca ? alloca(bytes_needed) :
safe_malloc_(bytes_needed);
+#endif
+ format_input_(tmp, signal, channels, samples, bytes_per_sample);
+ const FLAC__bool retval = (EVP_DigestUpdate(ctx, tmp , bytes_needed) == 1);
+#if defined(_MSC_VER)
+ _freea(tmp)
+#else
+ if(!usealloca) free(tmp);
+#endif
+ return retval;
+#else
if(ctx->capacity < bytes_needed) {
FLAC__byte *tmp = realloc(ctx->internal_buf, bytes_needed);
if(0 == tmp) {
@@ -418,4 +454,6 @@ FLAC__bool FLAC__MD5Accumulate(FLAC__MD5Context *ctx, const
FLAC__int32 * const
FLAC__MD5Update(ctx, ctx->internal_buf, bytes_needed);
return true;
+#endif
+
}
diff --git a/src/libFLAC/stream_decoder.c b/src/libFLAC/stream_decoder.c
index 5b3c3cd..f8029dd 100644
--- a/src/libFLAC/stream_decoder.c
+++ b/src/libFLAC/stream_decoder.c
@@ -185,7 +185,7 @@ typedef struct FLAC__StreamDecoderPrivate {
FLAC__bool internal_reset_hack; /* used only during init() so we can call
reset to set up the decoder without rewinding the input */
FLAC__bool is_seeking;
FLAC__MD5Context md5context;
- FLAC__byte computed_md5sum[16]; /* this is the sum we computed from the
decoded data */
+ FLAC__byte computed_md5sum[EVP_MAX_MD_SIZE]; /* this is the sum we computed
from the decoded data */
/* (the rest of these are only used for seeking) */
FLAC__Frame last_frame; /* holds the info of the last frame we seeked to */
FLAC__uint64 first_frame_offset; /* hint to the seek routine of where in the
stream the first audio frame starts */
@@ -322,7 +322,9 @@ FLAC_API FLAC__StreamDecoder *FLAC__stream_decoder_new(void)
decoder->private_->file = 0;
set_defaults_(decoder);
-
+#if defined(HAVE_OPENSSL)
+ EVP_MD_CTX_init(&decoder->private_->md5context);
+#endif
decoder->protected_->state = FLAC__STREAM_DECODER_UNINITIALIZED;
return decoder;
@@ -346,7 +348,9 @@ FLAC_API void
FLAC__stream_decoder_delete(FLAC__StreamDecoder *decoder)
for(i = 0; i < FLAC__MAX_CHANNELS; i++)
FLAC__format_entropy_coding_method_partitioned_rice_contents_clear(&decoder->private_->partitioned_rice_contents[i]);
-
+#if defined(HAVE_OPENSSL)
+ EVP_MD_CTX_cleanup(&decoder->private_->md5context);
+#endif
free(decoder->private_);
free(decoder->protected_);
free(decoder);
@@ -666,8 +670,16 @@ FLAC_API FLAC__bool
FLAC__stream_decoder_finish(FLAC__StreamDecoder *decoder)
/* see the comment in FLAC__seekable_stream_decoder_reset() as to why we
* always call FLAC__MD5Final()
*/
+#if defined(HAVE_OPENSSL)
+ /* decoder->private_->computed_md5sum is NULL when
decoder->private_->do_md5_checking == false
+ * that causes assertion failure crash in openSSL.
+ */
+ if(decoder->private_->do_md5_checking) {
+ md5_failed =
(EVP_DigestFinal_ex(&decoder->private_->md5context,
decoder->private_->computed_md5sum, NULL) == 0);
+ }
+#else
FLAC__MD5Final(decoder->private_->computed_md5sum,
&decoder->private_->md5context);
-
+#endif
if(decoder->private_->has_seek_table && 0 !=
decoder->private_->seek_table.data.seek_table.points) {
free(decoder->private_->seek_table.data.seek_table.points);
decoder->private_->seek_table.data.seek_table.points = 0;
@@ -1018,11 +1030,15 @@ FLAC_API FLAC__bool
FLAC__stream_decoder_reset(FLAC__StreamDecoder *decoder)
* FLAC__stream_decoder_finish() to make sure things are always cleaned up
* properly.
*/
- FLAC__MD5Init(&decoder->private_->md5context);
+ decoder->private_->first_frame_offset = 0;
- decoder->private_->first_frame_offset = 0;
- decoder->private_->unparseable_frame_count = 0;
+ decoder->private_->unparseable_frame_count = 0;
+#if defined(HAVE_OPENSSL)
+ return (EVP_DigestInit_ex(&decoder->private_->md5context,
EVP_md5(), NULL) == 1);
+#else
+????FLAC__MD5Init(&decoder->private_->md5context);
+#endif
return true;
}
diff --git a/src/libFLAC/stream_encoder.c b/src/libFLAC/stream_encoder.c
index 7e102a5..787366e 100644
--- a/src/libFLAC/stream_encoder.c
+++ b/src/libFLAC/stream_encoder.c
@@ -570,7 +570,9 @@ FLAC_API FLAC__StreamEncoder *FLAC__stream_encoder_new(void)
FLAC__format_entropy_coding_method_partitioned_rice_contents_init(&encoder->private_->partitioned_rice_contents_extra[i]);
encoder->protected_->state = FLAC__STREAM_ENCODER_UNINITIALIZED;
-
+#if defined(HAVE_OPENSSL)
+ if(encoder->protected_->do_md5)
EVP_MD_CTX_init(&encoder->private_->md5context);
+#endif
return encoder;
}
@@ -602,6 +604,11 @@ FLAC_API void
FLAC__stream_encoder_delete(FLAC__StreamEncoder *encoder)
FLAC__format_entropy_coding_method_partitioned_rice_contents_clear(&encoder->private_->partitioned_rice_contents_extra[i]);
FLAC__bitwriter_delete(encoder->private_->frame);
+#if defined(HAVE_OPENSSL)
+ if(encoder->protected_->do_md5) {
+ EVP_MD_CTX_cleanup(&encoder->private_->md5context);
+ }
+#endif
free(encoder->private_);
free(encoder->protected_);
free(encoder);
@@ -1035,8 +1042,15 @@ static FLAC__StreamEncoderInitStatus
init_stream_internal_(
encoder->private_->streaminfo.data.stream_info.bits_per_sample =
encoder->protected_->bits_per_sample;
encoder->private_->streaminfo.data.stream_info.total_samples =
encoder->protected_->total_samples_estimate; /* we will replace this later
with the real total */
memset(encoder->private_->streaminfo.data.stream_info.md5sum, 0, 16); /*
we don't know this yet; have to fill it in later */
- if(encoder->protected_->do_md5)
- FLAC__MD5Init(&encoder->private_->md5context);
+ if(encoder->protected_->do_md5) {
+#if defined(HAVE_OPENSSL)
+ if(EVP_DigestInit_ex(&encoder->private_->md5context,
EVP_md5(), NULL) == 0) {
+ return FLAC__STREAM_ENCODER_INIT_STATUS_ENCODER_ERROR;
+ }
+#else
+ FLAC__MD5Init(&encoder->private_->md5context);
+#endif
+ }
if(!FLAC__add_metadata_block(&encoder->private_->streaminfo,
encoder->private_->frame)) {
encoder->protected_->state = FLAC__STREAM_ENCODER_FRAMING_ERROR;
return FLAC__STREAM_ENCODER_INIT_STATUS_ENCODER_ERROR;
@@ -1305,9 +1319,13 @@ FLAC_API FLAC__bool
FLAC__stream_encoder_finish(FLAC__StreamEncoder *encoder)
}
}
- if(encoder->protected_->do_md5)
- FLAC__MD5Final(encoder->private_->streaminfo.data.stream_info.md5sum,
&encoder->private_->md5context);
-
+ if(encoder->protected_->do_md5) {
+#if defined(HAVE_OPENSSL)
+ error = (EVP_DigestFinal_ex(&encoder->private_->md5context,
encoder->private_->streaminfo.data.stream_info.md5sum, NULL) == 0);
+#else
+
FLAC__MD5Final(encoder->private_->streaminfo.data.stream_info.md5sum,
&encoder->private_->md5context);
+#endif
+ }
if(!encoder->private_->is_being_deleted) {
if(encoder->protected_->state == FLAC__STREAM_ENCODER_OK) {
if(encoder->private_->seek_callback) {
--
1.7.7
Eric Wong
2012-May-06 01:04 UTC
[flac-dev] [PATCH] Optionally, allow distros to use openssl for MD5 verification
Cristian Rodr?guez <crrodriguez at opensuse.org> wrote:> +#if defined(HAVE_OPENSSL) > + /* decoder->private_->computed_md5sum is NULL when decoder->private_->do_md5_checking == false > + * that causes assertion failure crash in openSSL. > + */ > + if(decoder->private_->do_md5_checking) { > + md5_failed = (EVP_DigestFinal_ex(&decoder->private_->md5context, decoder->private_->computed_md5sum, NULL) == 0); > + } > +#else > FLAC__MD5Final(decoder->private_->computed_md5sum, &decoder->private_->md5context); > - > +#endifCan you do this without sprinkling #ifdefs all over the place? Mixing #ifdefs and normal C control structures make code hard to read/maintain. This is *especially* true for folks who aren't regular contributors to flac, myself included. I would define workalike macros/no-op functions instead and hide OpenSSL-related functionality behind them. Thanks.
Erik de Castro Lopo
2012-May-06 01:20 UTC
[flac-dev] [PATCH] Optionally, allow distros to use openssl for MD5 verification
Eric Wong wrote:> Can you do this without sprinkling #ifdefs all over the place?+1> Mixing #ifdefs and normal C control structures make code hard to > read/maintain. This is *especially* true for folks who aren't regular > contributors to flac, myself included.Very much agree. Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/
Cristian Rodríguez
2012-May-06 02:37 UTC
[flac-dev] [PATCH] Optionally, allow distros to use openssl for MD5 verification
El 05/05/12 21:04, Eric Wong escribi?:> Cristian Rodr?guez<crrodriguez at opensuse.org> wrote: >> +#if defined(HAVE_OPENSSL) >> + /* decoder->private_->computed_md5sum is NULL when decoder->private_->do_md5_checking == false >> + * that causes assertion failure crash in openSSL. >> + */ >> + if(decoder->private_->do_md5_checking) { >> + md5_failed = (EVP_DigestFinal_ex(&decoder->private_->md5context, decoder->private_->computed_md5sum, NULL) == 0); >> + } >> +#else >> FLAC__MD5Final(decoder->private_->computed_md5sum,&decoder->private_->md5context); >> - >> +#endif > > Can you do this without sprinkling #ifdefs all over the place? > > Mixing #ifdefs and normal C control structures make code hard to > read/maintain. This is *especially* true for folks who aren't regular > contributors to flac, myself included. > > I would define workalike macros/no-op functions instead and hide > OpenSSL-related functionality behind them.Well, there are reasons why I did it this way.. Initially I though about your suggested approach... With this new code - The format can be easily extended to support other digests algorithms, (i.e shaXXX that nowdays is implemented with hardware support) openssl is everywhere including windows. - The original code follows the traditional old-style convention of init() update() final() for every operation, this new version init() cleanup() only *once* per encoder/decoder "instance" reducing the number of function calls and openssl reuses the allocated resources...
Miroslav Lichvar
2012-May-07 10:23 UTC
[flac-dev] [PATCH] Optionally, allow distros to use openssl for MD5 verification
On Sat, May 05, 2012 at 05:34:31PM -0400, Cristian Rodr?guez wrote:> This has the advantage of being more efficient than the included > routines and allows distros to centralize crypto mainteniance on > a few libraries.Isn't the OpenSSL license incompatible with GPL? IANAL, but I think the flac and metaflac utilities can't be linked with OpenSSL unless there is an exception in the flac license which would require getting the permission from all contributors. I'd suggest to use the NSS library instead. It has an API just for hashing (NSSLOWHASH) which can be used with just one small library (freebl3) instead of the whole NSS stack. -- Miroslav Lichvar
Cristian Rodríguez
2012-May-07 18:01 UTC
[flac-dev] [PATCH] Optionally, allow distros to use openssl for MD5 verification
El 07/05/12 06:23, Miroslav Lichvar escribi?:> On Sat, May 05, 2012 at 05:34:31PM -0400, Cristian Rodr?guez wrote: >> This has the advantage of being more efficient than the included >> routines and allows distros to centralize crypto mainteniance on >> a few libraries. > > Isn't the OpenSSL license incompatible with GPL? IANAL, but I think > the flac and metaflac utilities can't be linked with OpenSSL unless > there is an exception in the flac license which would require getting > the permission from all contributors.flac and metaflac do not use openSSL, only libFLAC does.> I'd suggest to use the NSS library instead. It has an API just for > hashing (NSSLOWHASH) which can be used with just one small library > (freebl3) instead of the whole NSS stack.And hence loosing all benefits from my patch, NSS does not have : - Hardware assisted hashing - NSSLOWHASH does not have documentation - freedbl is not linked to any application in my full blow desktop, but openssl libcrypto is used widely, hence at least its codepaths are better excersized. Personally I don't see any technical reason to use a different library other than politics and obscure potential license incompatibilities. PS: I know NSS is the choice for this "design by comittee" thing "Crypto consolidation" in distributions that some suits are trying to push on, but Im glad to see it has failed miserably ;-)