diff mbox

[FFmpeg-devel] lavf/tls_openssl: Fix compilation with current libressl

Message ID CAB0OVGrKL1Vq8gqzbMVEnxmWFkg1Eu__jwXmAVK8-Vsbp2_=fg@mail.gmail.com
State New
Headers show

Commit Message

Carl Eugen Hoyos Nov. 7, 2017, 12:28 p.m. UTC 
Hi!

Attached patch fixes compilation with current libressl, related to ticket #6801.

Please comment, Carl Eugen

Comments

Timo Rothenpieler Nov. 7, 2017, 1:04 p.m. UTC | #1 
Am 07.11.2017 um 13:28 schrieb Carl Eugen Hoyos:
> Hi!
> 
> Attached patch fixes compilation with current libressl, related to ticket #6801.
> 
> Please comment, Carl Eugen

Wouldn't it be better to move that check to configure, so it does not 
build the openssl backend with libressl, and then merge the libtls 
backend that's been posted, so there is proper support for libressl 
instead of an ever growing collection of #ifdefs?
James Almer Nov. 7, 2017, 1:15 p.m. UTC | #2 
On 11/7/2017 10:04 AM, Timo Rothenpieler wrote:
> Am 07.11.2017 um 13:28 schrieb Carl Eugen Hoyos:
>> Hi!
>>
>> Attached patch fixes compilation with current libressl, related to
>> ticket #6801.
>>
>> Please comment, Carl Eugen
> 
> Wouldn't it be better to move that check to configure, so it does not
> build the openssl backend with libressl, and then merge the libtls
> backend that's been posted, so there is proper support for libressl
> instead of an ever growing collection of #ifdefs?

I would very much rather have a way to get libressl to compile with
tls_openssl.c, or just reject it altogether, than adding a duplicate
module just for a fork that pretends to be compatible with a version of
openssl but not providing the required API for it.

I refuse to give special treatment to such a sloppy fork, especially
when adding it would increase the complexity in configure (As only one
tls module can be compiled per build).
An "if not defined libressl" for every openssl >= 1.1 check is simpler
and easier to maintain than a whole separate duplicate module.
Timo Rothenpieler Nov. 7, 2017, 1:28 p.m. UTC | #3 
> I would very much rather have a way to get libressl to compile with
> tls_openssl.c, or just reject it altogether, than adding a duplicate
> module just for a fork that pretends to be compatible with a version of
> openssl but not providing the required API for it.
> 
> I refuse to give special treatment to such a sloppy fork, especially
> when adding it would increase the complexity in configure (As only one
> tls module can be compiled per build).
> An "if not defined libressl" for every openssl >= 1.1 check is simpler
> and easier to maintain than a whole separate duplicate module.

 From how I understand it, libtls is an entirely new API, so it would 
not be a duplicate of the openssl backend.
James Almer Nov. 7, 2017, 1:37 p.m. UTC | #4 
On 11/7/2017 10:28 AM, Timo Rothenpieler wrote:
>> I would very much rather have a way to get libressl to compile with
>> tls_openssl.c, or just reject it altogether, than adding a duplicate
>> module just for a fork that pretends to be compatible with a version of
>> openssl but not providing the required API for it.
>>
>> I refuse to give special treatment to such a sloppy fork, especially
>> when adding it would increase the complexity in configure (As only one
>> tls module can be compiled per build).
>> An "if not defined libressl" for every openssl >= 1.1 check is simpler
>> and easier to maintain than a whole separate duplicate module.
> 
> From how I understand it, libtls is an entirely new API, so it would not
> be a duplicate of the openssl backend.

Then how is Carl's patch supposed to work? Unless you mean new API on
top of the inherited openssl 1.0 API. If that's the case and such new
libressl specific API is worth using then a separate module could be
considered (Or just some more ifdeffery in tls_openssl.c, much like we
do for openssl 1.1), but if it works as is then as i said I want to
avoid new TLS modules as much as possible.
Timo Rothenpieler Nov. 7, 2017, 1:43 p.m. UTC | #5 
Am 07.11.2017 um 14:37 schrieb James Almer:
> On 11/7/2017 10:28 AM, Timo Rothenpieler wrote:
>>> I would very much rather have a way to get libressl to compile with
>>> tls_openssl.c, or just reject it altogether, than adding a duplicate
>>> module just for a fork that pretends to be compatible with a version of
>>> openssl but not providing the required API for it.
>>>
>>> I refuse to give special treatment to such a sloppy fork, especially
>>> when adding it would increase the complexity in configure (As only one
>>> tls module can be compiled per build).
>>> An "if not defined libressl" for every openssl >= 1.1 check is simpler
>>> and easier to maintain than a whole separate duplicate module.
>>
>>  From how I understand it, libtls is an entirely new API, so it would not
>> be a duplicate of the openssl backend.
> 
> Then how is Carl's patch supposed to work? Unless you mean new API on
> top of the inherited openssl 1.0 API. If that's the case and such new
> libressl specific API is worth using then a separate module could be
> considered (Or just some more ifdeffery in tls_openssl.c, much like we
> do for openssl 1.1), but if it works as is then as i said I want to
> avoid new TLS modules as much as possible.

LibreSSL offers an older version of the openssl API in libssl, and at 
the same time also a new somewhat cleaner one in libtls, which right now 
is just a wrapper, but who knows if it becomes the new default at some 
point.
Hendrik Leppkes Nov. 7, 2017, 1:48 p.m. UTC | #6 
On Tue, Nov 7, 2017 at 1:28 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> Hi!
>
> Attached patch fixes compilation with current libressl, related to ticket #6801.
>

I believe we basically had that exact same patch on the ML before, and
the consensus was to not have a jungle of #ifdefs in the code, and
instead have configure figure it out.

- Hendrik
James Almer Nov. 7, 2017, 1:57 p.m. UTC | #7 
On 11/7/2017 10:48 AM, Hendrik Leppkes wrote:
> On Tue, Nov 7, 2017 at 1:28 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> Hi!
>>
>> Attached patch fixes compilation with current libressl, related to ticket #6801.
>>
> 
> I believe we basically had that exact same patch on the ML before, and
> the consensus was to not have a jungle of #ifdefs in the code, and
> instead have configure figure it out.

configure could at most refuse to compile tls_openssl with libressl, but
nothing to make it build successfully without ifdeffery.

Is that preferred?
James Almer Nov. 7, 2017, 3:19 p.m. UTC | #8 
On 11/7/2017 12:15 PM, Hendrik Leppkes wrote:
> On Tue, Nov 7, 2017 at 2:57 PM, James Almer <jamrial@gmail.com> wrote:
>> On 11/7/2017 10:48 AM, Hendrik Leppkes wrote:
>>> On Tue, Nov 7, 2017 at 1:28 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>>> Hi!
>>>>
>>>> Attached patch fixes compilation with current libressl, related to ticket #6801.
>>>>
>>>
>>> I believe we basically had that exact same patch on the ML before, and
>>> the consensus was to not have a jungle of #ifdefs in the code, and
>>> instead have configure figure it out.
>>
>> configure could at most refuse to compile tls_openssl with libressl, but
>> nothing to make it build successfully without ifdeffery.
>>
>> Is that preferred?
> 
> I meant to have configure figure out properly if OpenSSL 1.1 API is
> available, and not repeat that check in the code several times, which
> is growing more complex now.
> 
> - Hendrik

So you mean replacing "#if OPENSSL_VERSION_NUMBER >= 0x1010000fL &&
!defined(LIBRESSL_VERSION_NUMBER)" with "#if CONFIG_OPENSSL_1_1" or
similar? Should be easy and agree it's slightly cleaner.
Michael Niedermayer Nov. 8, 2017, 10:29 a.m. UTC | #9 
On Tue, Nov 07, 2017 at 12:19:02PM -0300, James Almer wrote:
> On 11/7/2017 12:15 PM, Hendrik Leppkes wrote:
> > On Tue, Nov 7, 2017 at 2:57 PM, James Almer <jamrial@gmail.com> wrote:
> >> On 11/7/2017 10:48 AM, Hendrik Leppkes wrote:
> >>> On Tue, Nov 7, 2017 at 1:28 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >>>> Hi!
> >>>>
> >>>> Attached patch fixes compilation with current libressl, related to ticket #6801.
> >>>>
> >>>
> >>> I believe we basically had that exact same patch on the ML before, and
> >>> the consensus was to not have a jungle of #ifdefs in the code, and
> >>> instead have configure figure it out.
> >>
> >> configure could at most refuse to compile tls_openssl with libressl, but
> >> nothing to make it build successfully without ifdeffery.
> >>
> >> Is that preferred?
> > 
> > I meant to have configure figure out properly if OpenSSL 1.1 API is
> > available, and not repeat that check in the code several times, which
> > is growing more complex now.
> > 
> > - Hendrik
> 
> So you mean replacing "#if OPENSSL_VERSION_NUMBER >= 0x1010000fL &&
> !defined(LIBRESSL_VERSION_NUMBER)" with "#if CONFIG_OPENSSL_1_1" or
> similar? Should be easy and agree it's slightly cleaner.

+1
this seems the cleanest solution to me too

[...]
diff mbox

Patch

From 9e660d1bbe0842264971f1e4e8c2539bdb36cd4c Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
Date: Tue, 7 Nov 2017 13:25:55 +0100
Subject: [PATCH] lavf/tls_openssl: Fix compilation with current libressl.

Fixes a bug related to ticket #6801.
---
 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 1443e90..1e261d3 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;
@@ -137,7 +137,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
@@ -147,7 +147,7 @@  static int tls_close(URLContext *h)
 
 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);
@@ -164,7 +164,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
@@ -212,7 +212,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",
@@ -276,7 +276,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);
-- 
1.7.10.4