| Message ID | 1477640828-2738-1-git-send-email-mforney@mforney.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, Oct 28, 2016 at 12:47:08AM -0700, Michael Forney wrote: > Signed-off-by: Michael Forney <mforney@mforney.org> > --- > libavformat/tls_openssl.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/libavformat/tls_openssl.c b/libavformat/tls_openssl.c > index c551ac7..9712856 100644 > --- a/libavformat/tls_openssl.c > +++ b/libavformat/tls_openssl.c > @@ -43,7 +43,7 @@ typedef struct TLSContext { > TLSShared tls_shared; > SSL_CTX *ctx; > SSL *ssl; > -#if OPENSSL_VERSION_NUMBER >= 0x1010000fL > +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_NUMBER) > BIO_METHOD* url_bio_method; > #endif > } TLSContext; > @@ -68,7 +68,7 @@ static unsigned long openssl_thread_id(void) > > static int url_bio_create(BIO *b) > { > -#if OPENSSL_VERSION_NUMBER >= 0x1010000fL > +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_NUMBER) > BIO_set_init(b, 1); > BIO_set_data(b, NULL); > BIO_set_flags(b, 0); > @@ -85,7 +85,7 @@ static int url_bio_destroy(BIO *b) > return 1; > } > > -#if OPENSSL_VERSION_NUMBER >= 0x1010000fL > +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_NUMBER) > #define GET_BIO_DATA(x) BIO_get_data(x); > #else > #define GET_BIO_DATA(x) (x)->ptr; > @@ -133,7 +133,7 @@ static int url_bio_bputs(BIO *b, const char *str) > return url_bio_bwrite(b, str, strlen(str)); > } > > -#if OPENSSL_VERSION_NUMBER < 0x1010000fL > +#if OPENSSL_VERSION_NUMBER < 0x1010000fL || defined(LIBRESSL_VERSION_NUMBER) > static BIO_METHOD url_bio_method = { > .type = BIO_TYPE_SOURCE_SINK, > .name = "urlprotocol bio", > @@ -212,7 +212,7 @@ static int tls_close(URLContext *h) > SSL_CTX_free(c->ctx); > if (c->tls_shared.tcp) > ffurl_close(c->tls_shared.tcp); > -#if OPENSSL_VERSION_NUMBER >= 0x1010000fL > +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_NUMBER) > if (c->url_bio_method) > BIO_meth_free(c->url_bio_method); > #endif > @@ -265,7 +265,7 @@ static int tls_open(URLContext *h, const char *uri, int flags, AVDictionary **op > ret = AVERROR(EIO); > goto fail; > } > -#if OPENSSL_VERSION_NUMBER >= 0x1010000fL > +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_NUMBER) > p->url_bio_method = BIO_meth_new(BIO_TYPE_SOURCE_SINK, "urlprotocol bio"); > BIO_meth_set_write(p->url_bio_method, url_bio_bwrite); > BIO_meth_set_read(p->url_bio_method, url_bio_bread); The same condition is repeated instead whatever the underlaying thing is that is intended to be checkd for should be set by configure and used in something like that #if HAVE_BIO_METHODS_WHATEVER ... #endif [...]
On 30 October 2016 at 09:39, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Oct 28, 2016 at 12:47:08AM -0700, Michael Forney wrote: > > Signed-off-by: Michael Forney <mforney@mforney.org> > > --- > > libavformat/tls_openssl.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/libavformat/tls_openssl.c b/libavformat/tls_openssl.c > > index c551ac7..9712856 100644 > > --- a/libavformat/tls_openssl.c > > +++ b/libavformat/tls_openssl.c > > @@ -43,7 +43,7 @@ typedef struct TLSContext { > > TLSShared tls_shared; > > SSL_CTX *ctx; > > SSL *ssl; > > -#if OPENSSL_VERSION_NUMBER >= 0x1010000fL > > +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_ > NUMBER) > > BIO_METHOD* url_bio_method; > > #endif > > } TLSContext; > > @@ -68,7 +68,7 @@ static unsigned long openssl_thread_id(void) > > > > static int url_bio_create(BIO *b) > > { > > -#if OPENSSL_VERSION_NUMBER >= 0x1010000fL > > +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_ > NUMBER) > > BIO_set_init(b, 1); > > BIO_set_data(b, NULL); > > BIO_set_flags(b, 0); > > @@ -85,7 +85,7 @@ static int url_bio_destroy(BIO *b) > > return 1; > > } > > > > -#if OPENSSL_VERSION_NUMBER >= 0x1010000fL > > +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_ > NUMBER) > > #define GET_BIO_DATA(x) BIO_get_data(x); > > #else > > #define GET_BIO_DATA(x) (x)->ptr; > > @@ -133,7 +133,7 @@ static int url_bio_bputs(BIO *b, const char *str) > > return url_bio_bwrite(b, str, strlen(str)); > > } > > > > -#if OPENSSL_VERSION_NUMBER < 0x1010000fL > > +#if OPENSSL_VERSION_NUMBER < 0x1010000fL || defined(LIBRESSL_VERSION_ > NUMBER) > > static BIO_METHOD url_bio_method = { > > .type = BIO_TYPE_SOURCE_SINK, > > .name = "urlprotocol bio", > > @@ -212,7 +212,7 @@ static int tls_close(URLContext *h) > > SSL_CTX_free(c->ctx); > > if (c->tls_shared.tcp) > > ffurl_close(c->tls_shared.tcp); > > -#if OPENSSL_VERSION_NUMBER >= 0x1010000fL > > +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_ > NUMBER) > > if (c->url_bio_method) > > BIO_meth_free(c->url_bio_method); > > #endif > > @@ -265,7 +265,7 @@ static int tls_open(URLContext *h, const char *uri, > int flags, AVDictionary **op > > ret = AVERROR(EIO); > > goto fail; > > } > > -#if OPENSSL_VERSION_NUMBER >= 0x1010000fL > > +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_ > NUMBER) > > p->url_bio_method = BIO_meth_new(BIO_TYPE_SOURCE_SINK, > "urlprotocol bio"); > > BIO_meth_set_write(p->url_bio_method, url_bio_bwrite); > > BIO_meth_set_read(p->url_bio_method, url_bio_bread); > This also seems a bit odd, why is libreSSL setting an openssl version number of 1.1.0 or higher when it doesnt actually conform to the corresponding openssl version. LibreSSl setting the openssl version number to 2.0.0 seems to be problematic and causes problems not just for ffmpeg. Of course theres not much we can do about that but perhaps an alternative to Michaels suggestion would be to just add a single check near the top of the file that checks for libressl and then redefines the openssl version to the api libressl actually supports. i.e. #ifdef LIBRESSL_VERSION_NUMBER #undef OPENSSL_VERSION_NUMBER #define OPENSSL_VERSION_NUMBER 0x1000107fL #endif This I believe is what some other projects have done to avoid this issue and is rather simple to add and prevents further clutter in our configure. I dont have any preference between this and the other suggestion, im just providing some alternatives. The above is pretty simple so if it works for you i can write up a patch for that pretty easily.
On 10/29/16, Matt Oliver <protogonoi@gmail.com> wrote: > This also seems a bit odd, why is libreSSL setting an openssl version > number of 1.1.0 or higher when it doesnt actually conform to the > corresponding openssl version. LibreSSl setting the openssl version number > to 2.0.0 seems to be problematic and causes problems not just for ffmpeg. > Of course theres not much we can do about that but perhaps an alternative > to Michaels suggestion would be to just add a single check near the top of > the file that checks for libressl and then redefines the openssl version to > the api libressl actually supports. i.e. > > #ifdef LIBRESSL_VERSION_NUMBER > #undef OPENSSL_VERSION_NUMBER > #define OPENSSL_VERSION_NUMBER 0x1000107fL > #endif > > This I believe is what some other projects have done to avoid this issue > and is rather simple to add and prevents further clutter in our configure. > I dont have any preference between this and the other suggestion, im just > providing some alternatives. The above is pretty simple so if it works for > you i can write up a patch for that pretty easily. I'm not sure why libressl sets OPENSSL_VERSION_NUMBER to 0x20000000L. Maybe so that they can pick and choose which openssl APIs to implement (rather than everything up through some version)? I'm fine with any of the suggested approaches. I just went with !defined(LIBRESSL_VERSION_NUMBER) in my original patch because that's what I've seen projects like curl, wpa_supplicant, and openvpn do. It also seems to be what openbsd is doing itself with patches in its ports tree (xca, stunnel, postfix).
On 30 October 2016 at 17:57, Michael Forney <mforney@mforney.org> wrote: > On 10/29/16, Matt Oliver <protogonoi@gmail.com> wrote: > > This also seems a bit odd, why is libreSSL setting an openssl version > > number of 1.1.0 or higher when it doesnt actually conform to the > > corresponding openssl version. LibreSSl setting the openssl version > number > > to 2.0.0 seems to be problematic and causes problems not just for ffmpeg. > > Of course theres not much we can do about that but perhaps an alternative > > to Michaels suggestion would be to just add a single check near the top > of > > the file that checks for libressl and then redefines the openssl version > to > > the api libressl actually supports. i.e. > > > > #ifdef LIBRESSL_VERSION_NUMBER > > #undef OPENSSL_VERSION_NUMBER > > #define OPENSSL_VERSION_NUMBER 0x1000107fL > > #endif > > > > This I believe is what some other projects have done to avoid this issue > > and is rather simple to add and prevents further clutter in our > configure. > > I dont have any preference between this and the other suggestion, im just > > providing some alternatives. The above is pretty simple so if it works > for > > you i can write up a patch for that pretty easily. > > I'm not sure why libressl sets OPENSSL_VERSION_NUMBER to 0x20000000L. > Maybe so that they can pick and choose which openssl APIs to implement > (rather than everything up through some version)? > > I'm fine with any of the suggested approaches. I just went with > !defined(LIBRESSL_VERSION_NUMBER) in my original patch because that's > what I've seen projects like curl, wpa_supplicant, and openvpn do. It > also seems to be what openbsd is doing itself with patches in its > ports tree (xca, stunnel, postfix). > Something should actually be done about this. Im fine with the original patch but the blocker seems to have been Michael. Im not such a huge fan of cluttering configure and slowing it down more for a small issue (as there could get progressively more and more difference between the 2 projects) so as an inbetween what about creating HAVE_BIO_METHODS but do so only locally in tls_openssl using something like: #if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_ NUMBER) #define HAVE_BIO_METHODS_WHATEVER 1 #else #define HAVE_BIO_METHODS_WHATEVER 0 #endif
On Wed, Dec 7, 2016 at 7:13 AM, Matt Oliver <protogonoi@gmail.com> wrote: > On 30 October 2016 at 17:57, Michael Forney <mforney@mforney.org> wrote: > >> On 10/29/16, Matt Oliver <protogonoi@gmail.com> wrote: >> > This also seems a bit odd, why is libreSSL setting an openssl version >> > number of 1.1.0 or higher when it doesnt actually conform to the >> > corresponding openssl version. LibreSSl setting the openssl version >> number >> > to 2.0.0 seems to be problematic and causes problems not just for ffmpeg. >> > Of course theres not much we can do about that but perhaps an alternative >> > to Michaels suggestion would be to just add a single check near the top >> of >> > the file that checks for libressl and then redefines the openssl version >> to >> > the api libressl actually supports. i.e. >> > >> > #ifdef LIBRESSL_VERSION_NUMBER >> > #undef OPENSSL_VERSION_NUMBER >> > #define OPENSSL_VERSION_NUMBER 0x1000107fL >> > #endif >> > >> > This I believe is what some other projects have done to avoid this issue >> > and is rather simple to add and prevents further clutter in our >> configure. >> > I dont have any preference between this and the other suggestion, im just >> > providing some alternatives. The above is pretty simple so if it works >> for >> > you i can write up a patch for that pretty easily. >> >> I'm not sure why libressl sets OPENSSL_VERSION_NUMBER to 0x20000000L. >> Maybe so that they can pick and choose which openssl APIs to implement >> (rather than everything up through some version)? >> >> I'm fine with any of the suggested approaches. I just went with >> !defined(LIBRESSL_VERSION_NUMBER) in my original patch because that's >> what I've seen projects like curl, wpa_supplicant, and openvpn do. It >> also seems to be what openbsd is doing itself with patches in its >> ports tree (xca, stunnel, postfix). >> > > Something should actually be done about this. Im fine with the original > patch but the blocker seems to have been Michael. Im not such a huge fan of > cluttering configure and slowing it down more for a small issue (as there > could get progressively more and more difference between the 2 projects) so > as an inbetween what about creating HAVE_BIO_METHODS but do so only locally > in tls_openssl using something like: The way I see it, if LibreSSL pretends to be OpenSSL and expects to run with the same code in downstream projects, it should also match its API and/or version checks. If it doesn't, we cannot realistically support it. Otherwise, we should drop any illusions of treating them the same, especially if as you say they diverge more and more in the future. In any case, configure is the place to check these things, heuristics in the code is what configure is designed to avoid. - Hendrik
diff --git a/libavformat/tls_openssl.c b/libavformat/tls_openssl.c index c551ac7..9712856 100644 --- a/libavformat/tls_openssl.c +++ b/libavformat/tls_openssl.c @@ -43,7 +43,7 @@ typedef struct TLSContext { TLSShared tls_shared; SSL_CTX *ctx; SSL *ssl; -#if OPENSSL_VERSION_NUMBER >= 0x1010000fL +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_NUMBER) BIO_METHOD* url_bio_method; #endif } TLSContext; @@ -68,7 +68,7 @@ static unsigned long openssl_thread_id(void) static int url_bio_create(BIO *b) { -#if OPENSSL_VERSION_NUMBER >= 0x1010000fL +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_NUMBER) BIO_set_init(b, 1); BIO_set_data(b, NULL); BIO_set_flags(b, 0); @@ -85,7 +85,7 @@ static int url_bio_destroy(BIO *b) return 1; } -#if OPENSSL_VERSION_NUMBER >= 0x1010000fL +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_NUMBER) #define GET_BIO_DATA(x) BIO_get_data(x); #else #define GET_BIO_DATA(x) (x)->ptr; @@ -133,7 +133,7 @@ static int url_bio_bputs(BIO *b, const char *str) return url_bio_bwrite(b, str, strlen(str)); } -#if OPENSSL_VERSION_NUMBER < 0x1010000fL +#if OPENSSL_VERSION_NUMBER < 0x1010000fL || defined(LIBRESSL_VERSION_NUMBER) static BIO_METHOD url_bio_method = { .type = BIO_TYPE_SOURCE_SINK, .name = "urlprotocol bio", @@ -212,7 +212,7 @@ static int tls_close(URLContext *h) SSL_CTX_free(c->ctx); if (c->tls_shared.tcp) ffurl_close(c->tls_shared.tcp); -#if OPENSSL_VERSION_NUMBER >= 0x1010000fL +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_NUMBER) if (c->url_bio_method) BIO_meth_free(c->url_bio_method); #endif @@ -265,7 +265,7 @@ static int tls_open(URLContext *h, const char *uri, int flags, AVDictionary **op ret = AVERROR(EIO); goto fail; } -#if OPENSSL_VERSION_NUMBER >= 0x1010000fL +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_NUMBER) p->url_bio_method = BIO_meth_new(BIO_TYPE_SOURCE_SINK, "urlprotocol bio"); BIO_meth_set_write(p->url_bio_method, url_bio_bwrite); BIO_meth_set_read(p->url_bio_method, url_bio_bread);
Signed-off-by: Michael Forney <mforney@mforney.org> --- libavformat/tls_openssl.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)