Обсуждение: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails on Windows with Visual Studio 2017
BUG #15789: libpq compilation with OpenSSL 1.1.1b fails on Windows with Visual Studio 2017
От
PG Bug reporting form
Дата:
The following bug has been logged on the website: Bug reference: 15789 Logged by: Sergey Pashkov Email address: serpashk@gmail.com PostgreSQL version: 11.2 Operating system: Windows 10 Description: libssh2 and libpq are used in the same product. It is necessary to use OpenSSL 1.1.1b as it includes EdDSA support required for libssh2. Here are parts of our build scripts. 1. Building OpenSSL: perl Configure VC-WIN64A no-shared no-asm enable-ssl3 enable-ssl3-method nmake 2. Building libpq: XCOPY /s /i "%OPENSSL_SRCS%" "%OPENSSL_PATH%" MKDIR "%OPENSSL_PATH%\lib" COPY "%THIRD_LIBS%\libssl_64.lib" "%OPENSSL_PATH%\lib\ssleay32.lib" COPY "%THIRD_LIBS%\libcrypto_64.lib" "%OPENSSL_PATH%\lib\libeay32.lib" perl mkvcbuild.pl COPY config_default.pl config.pl sed -i "s/openssl => undef/openssl => \"%OPENSSL_PATH_ESC%\"/g" config.pl perl mkvcbuild.pl msbuild pgsql.sln /t:interfaces\libpq /p:Configuration="Release" /m The following errors are encountered: "C:\Users\User\AppData\Local\Temp\postgres_64\libpq.vcxproj" (default target) (6) -> (ClCompile target) -> c:\users\user\appdata\local\temp\postgres_64\src\interfaces\libpq\fe-secure-openssl.c(1467): error C2037: left of 'ptr' specifies undefined struct/union 'bio_st' [C:\Users\User\AppData\Local\Temp\postgres_64\libpq.vcxproj] c:\users\user\appdata\local\temp\postgres_64\src\interfaces\libpq\fe-secure-openssl.c(1467): error C2198: 'pqs ecure_raw_read': too few arguments for call [C:\Users\User\AppData\Local\Temp\postgres_64\libpq.vcxproj] c:\users\user\appdata\local\temp\postgres_64\src\interfaces\libpq\fe-secure-openssl.c(1497): error C2037: left of 'ptr' specifies undefined struct/union 'bio_st' [C:\Users\User\AppData\Local\Temp\postgres_64\libpq.vcxproj] c:\users\user\appdata\local\temp\postgres_64\src\interfaces\libpq\fe-secure-openssl.c(1497): error C2198: 'pqs ecure_raw_write': too few arguments for call [C:\Users\User\AppData\Local\Temp\postgres_64\libpq.vcxproj] c:\users\user\appdata\local\temp\postgres_64\src\interfaces\libpq\fe-secure-openssl.c(1556): error C2027: use of undefined type 'bio_method_st' [C:\Users\User\AppData\Local\Temp\postgres_64\libpq.vcxproj] c:\users\user\appdata\local\temp\postgres_64\src\interfaces\libpq\fe-secure-openssl.c(1559): error C2027: use of undefined type 'bio_method_st' [C:\Users\User\AppData\Local\Temp\postgres_64\libpq.vcxproj] c:\users\user\appdata\local\temp\postgres_64\src\interfaces\libpq\fe-secure-openssl.c(1560): error C2037: left of 'bread' specifies undefined struct/union 'bio_method_st' [C:\Users\User\AppData\Local\Temp\postgres_64\libpq .vcxproj] c:\users\user\appdata\local\temp\postgres_64\src\interfaces\libpq\fe-secure-openssl.c(1561): error C2037: left of 'bwrite' specifies undefined struct/union 'bio_method_st' [C:\Users\User\AppData\Local\Temp\postgres_64\libp q.vcxproj] c:\users\user\appdata\local\temp\postgres_64\src\interfaces\libpq\fe-secure-openssl.c(1587): error C2037: left of 'ptr' specifies undefined struct/union 'bio_st' [C:\Users\User\AppData\Local\Temp\postgres_64\libpq.vcxproj] Headers of the recent OpenSSL versions don't include bio_st definition, as it was in 1.0.2. If I manually define the following macros in pg_config.h: HAVE_BIO_GET_DATA HAVE_BIO_METH_NEW - compilation passes but SSL connection cannot be established anyway. This question was raised in the following thread: https://www.postgresql.org/message-id/flat/CAAw-Mseg9JYpp%3DA%3D51HR3rKiQtbvT0MWw%2BaYFwNeJEbdNr%3DCDA%40mail.gmail.com No solution was proposed. Thank you!
Re: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails onWindows with Visual Studio 2017
От
Juan José Santamaría Flecha
Дата:
On Thu, May 2, 2019 at 3:58 PM PG Bug reporting form <noreply@postgresql.org> wrote:
This question was raised in the following thread:
https://www.postgresql.org/message-id/flat/CAAw-Mseg9JYpp%3DA%3D51HR3rKiQtbvT0MWw%2BaYFwNeJEbdNr%3DCDA%40mail.gmail.com
No solution was proposed.
Can you check if you get a sane compilation with the attached patch?
It is just a compilation issue, once the software is built with the 1.1.x libraries you should not have any further problems to use SSL.
Regards,
Juan José Santamaría Flecha
Вложения
Re: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails onWindows with Visual Studio 2017
От
Michael Paquier
Дата:
On Tue, May 28, 2019 at 07:31:55AM +0200, Juan José Santamaría Flecha wrote: > Can you check if you get a sane compilation with the attached patch? > > It is just a compilation issue, once the software is built with the 1.1.x > libraries you should not have any further problems to use SSL. Could you add your patch to the upcoming commit fest please? Here it is: https://commitfest.postgresql.org/23/ The scripts in src/tools/msvc/ make efforts for being able to compile with OpenSSL 1.0.2 which is the latest LTS version of upstream, but we lack facility to make them more dynamic depending on the version of OpenSSL so as the compile flags of pg_config.h can be enforced correctly. So what you are doing in GetOpenSSLVersion() is something that we are going to need badly, and OpenSSL has broken many interfaces between 1.0.2 and 1.1.0. + # Startint at version 1.1.0 OpenSSL have changed their library names from: + # libeay to libcrypto + # ssleay to libssl s/startint/starting/ Are these from the installers we recommend in the docs? I mean these ones: https://slproweb.com/products/Win32OpenSSL.html + if (-e "$self->{options}->{openssl}/lib/VC/libssl32MD.lib") Why not using a version-specific logic here? + my ($major, $minor) = $self->GetOpenSSLVersion(); + if ($major == 1 && $minor == 1) + { + print $o "#define HAVE_BIO_GET_DATA 1\n"; + print $o "#define HAVE_BIO_METH_NEW 1\n"; + } I think that you are missing HAVE_OPENSSL_INIT_SSL and HAVE_ASN1_STRING_GET0_DATA here. Please see commit message of bde64eb. -- Michael
Вложения
Re: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails onWindows with Visual Studio 2017
От
Juan José Santamaría Flecha
Дата:
On Tue, May 28, 2019 at 12:58 PM Michael Paquier <michael@paquier.xyz> wrote:
Could you add your patch to the upcoming commit fest please? Here it
is:
https://commitfest.postgresql.org/23/
Sure, done as [1]. The attached patch is still for 11, will it be back patched?
+ # Startint at version 1.1.0 OpenSSL have changed their library names from:
+ # libeay to libcrypto
+ # ssleay to libssl
s/startint/starting/
Are these from the installers we recommend in the docs? I mean these
ones:
https://slproweb.com/products/Win32OpenSSL.html
The change in the library names comes directly from OpenSSL [2].
+ if (-e "$self->{options}->{openssl}/lib/VC/libssl32MD.lib")
Why not using a version-specific logic here?
The version logic is just before that:
+ my ($major, $minor) = $self->GetOpenSSLVersion();
+ if ($major == 1 && $minor == 1)
+ if ($major == 1 && $minor == 1)
I guess that what you mean is, why testing the 32/64 bits using the libraries instead of using the 'platform'? I try to make it clearer in this version.
+ my ($major, $minor) = $self->GetOpenSSLVersion();
+ if ($major == 1 && $minor == 1)
+ {
+ print $o "#define HAVE_BIO_GET_DATA 1\n";
+ print $o "#define HAVE_BIO_METH_NEW 1\n";
+ }
I think that you are missing HAVE_OPENSSL_INIT_SSL and
HAVE_ASN1_STRING_GET0_DATA here. Please see commit message of
bde64eb.
Yes, you are right. Since those do not break the compilation between 1.0.2 and 1.1.0 I did not notice them.
Regards,
Juan José Santamaría Flecha
Вложения
Re: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails onWindows with Visual Studio 2017
От
Michael Paquier
Дата:
On Fri, May 31, 2019 at 04:11:37PM +0200, Juan José Santamaría Flecha wrote: > Sure, done as [1]. The attached patch is still for 11, will it be back > patched? I don't see a reason why we should not back-patch something like what you propose where build with OpenSSL 1.1.0 is supported (aka down to 9.5) if that proves to be reliable enough. -- Michael
Вложения
Re: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails onWindows with Visual Studio 2017
От
Sandeep Thakkar
Дата:
On Fri, May 31, 2019 at 10:47 PM Michael Paquier <michael@paquier.xyz> wrote:
On Fri, May 31, 2019 at 04:11:37PM +0200, Juan José Santamaría Flecha wrote:
> Sure, done as [1]. The attached patch is still for 11, will it be back
> patched?
I don't see a reason why we should not back-patch something like what
you propose where build with OpenSSL 1.1.0 is supported (aka down to
9.5) if that proves to be reliable enough.
OpenSSL 1.1.0 and 1.0.2 both are going out of Support in 2019 and PG 9.4 is supported till Feb2020.
Ideally, 1.1.1x support should be backpatched until 9.4
--
Michael
Sandeep Thakkar
Re: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails onWindows with Visual Studio 2017
От
Michael Paquier
Дата:
On Mon, Jun 03, 2019 at 03:30:49PM +0530, Sandeep Thakkar wrote: > OpenSSL 1.1.0 and 1.0.2 both are going out of Support in 2019 and PG 9.4 is > supported till Feb2020. > Ideally, 1.1.1x support should be backpatched until 9.4 There is no point to patch a stable branch if it has no support for the OpenSSL version we target. Now, bb132cdd has added support for OpenSSL 1.1.0 in 9.4, so there is actually no reason to not patch 9.4 as well and you are right. Missed that in my initial lookup of the commit logs as this was applied 7 months after the 9.5~ portions. -- Michael
Вложения
Re: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails onWindows with Visual Studio 2017
От
Juan José Santamaría Flecha
Дата:
On Tue, Jun 4, 2019 at 1:51 AM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Jun 03, 2019 at 03:30:49PM +0530, Sandeep Thakkar wrote:
> OpenSSL 1.1.0 and 1.0.2 both are going out of Support in 2019 and PG 9.4 is
> supported till Feb2020.
> Ideally, 1.1.1x support should be backpatched until 9.4
There is no point to patch a stable branch if it has no support for
the OpenSSL version we target. Now, bb132cdd has added support for
OpenSSL 1.1.0 in 9.4, so there is actually no reason to not patch 9.4
as well and you are right.
The patches are intended to support OpenSSL 1.1.x, including 1.1.1x.
The attached patches are meant for all supported versions and HEAD.
Regards,
Juan José Santamaría Flecha
Вложения
Re: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails onWindows with Visual Studio 2017
От
Michael Paquier
Дата:
On Wed, Jun 05, 2019 at 09:29:21AM +0200, Juan José Santamaría Flecha wrote: > The patches are intended to support OpenSSL 1.1.x, including 1.1.1x. > > The attached patches are meant for all supported versions and HEAD. I have been looking at this patch, and here are some comments. First, I have looked at the differences in what gets installed between 1.0.2 and 1.1.0 and things are rather messy: - On the Win64 and Win32 installers for 1.0.2, we have both libssl32.lib and libeay.lib, with the same naming of libraries in lib/VC/. This keeps the Postgres scripts more simple, but it causes conflicts with the installers when trying to manage both Win64 and Win32, so I cannot blame them for the changes done.. - In 1.1.0~, the situation gets fancy: -- For Win64 and Win32, we have both now libssl.lib and libcrypto.lib, which is still consistent. -- lib/VC/ has been heavily reworked so as we have now for example libssl64MD.lib & co for Win64 and libssl32MD.lib & co for Win32. Your patch catches the differences nicely. sub AddLibrary { - my ($self, $lib, $dbgsuffix) = @_; + my ($self, $lib, $slib) = @_; The only reason why we have the debugging option in the original interface is for OpenSSL, where Solution.pm checks for the presence of lib/VC/ssleay32MD.lib before deciding if we should use the debug libs or not. I am confused by what you are doing though: what do $slib and $xlib mean? For simplicity and compatibility, I am not sure that we should change the current interface, and just check for the debug libs that we expect, as done previously. If the new interface is more advantageous, the patch needs an effort of documentation, but I would keep that as a separate improvement. +sub GetOpenSSLVersion [...] + if ($sslout =~ /(\d+)\.(\d+)\.(\d+)/m) + { + return ($1, $2) I really like this interface, but I think that we should return the three digits instead to help with the decision making so as we don't need to rework it later on. + my ($major, $minor) = $self->GetOpenSSLVersion(); + if ($major == 1 && $minor == 1) If one day OpenSSL bumps to 1.2.X, this code would fail. I think that we should check that the major and minor digits are at least what we expect them to be. The same applies to the third digit. -- Michael
Вложения
Re: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails onWindows with Visual Studio 2017
От
Juan José Santamaría Flecha
Дата:
On Thu, Jun 20, 2019 at 4:50 AM Michael Paquier <michael@paquier.xyz> wrote: > > - In 1.1.0~, the situation gets fancy: > -- For Win64 and Win32, we have both now libssl.lib and > libcrypto.lib, which is still consistent. > -- lib/VC/ has been heavily reworked so as we have now for example > libssl64MD.lib & co for Win64 and libssl32MD.lib & co for Win32. > > Your patch catches the differences nicely. > Great. > sub AddLibrary > { > - my ($self, $lib, $dbgsuffix) = @_; > + my ($self, $lib, $slib) = @_; > The only reason why we have the debugging option in the original > interface is for OpenSSL, where Solution.pm checks for the presence of > lib/VC/ssleay32MD.lib before deciding if we should use the debug libs > or not. I am confused by what you are doing though: what do $slib and > $xlib mean? For simplicity and compatibility, I am not sure that we > should change the current interface, and just check for the debug libs > that we expect, as done previously. If the new interface is more > advantageous, the patch needs an effort of documentation, but I would > keep that as a separate improvement. > Since the third parameter is only currently used with OpenSSL libraries, I thought it would be ok to touch it in this patch. The reasson behind doing so was to keep the logic for the new fancy library naming tidy. Documenting better is not a thing I can argue against, xlib stands for expanded library name as used in GetAdditionalLinkerDependencies and maybe a comment like this can make things clearer: # The parameter $slib is for sufixed run-time library. # When available it will be added, if not it silently defaults to $lib. sub AddLibrary > +sub GetOpenSSLVersion > [...] > If one day OpenSSL bumps to 1.2.X, this code would fail. I think that > we should check that the major and minor digits are at least what we > expect them to be. The same applies to the third digit. No problem with that. Even more so, it can return 4 values: the 3 digits and the letter. For example, 1.1.1b would be (1, 1, 1, b). Regards, Juan José Santamaría Flecha
Re: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails onWindows with Visual Studio 2017
От
Michael Paquier
Дата:
On Fri, Jun 21, 2019 at 12:23:21PM +0200, Juan José Santamaría Flecha wrote: > Since the third parameter is only currently used with OpenSSL > libraries, I thought it would be ok to touch it in this patch. The > reasson behind doing so was to keep the logic for the new fancy > library naming tidy. Documenting better is not a thing I can argue > against, xlib stands for expanded library name as used in > GetAdditionalLinkerDependencies and maybe a comment like this can make > things clearer: > > # The parameter $slib is for sufixed run-time library. > # When available it will be added, if not it silently defaults to $lib. > sub AddLibrary That's a sensible argument. Perhaps your way of doing is much better at the end. I still found the patch hard to follow in the changes it did though, so documenting things for the changes you are doing would help anybody looking at this code in the future. > No problem with that. Even more so, it can return 4 values: the 3 > digits and the letter. For example, 1.1.1b would be (1, 1, 1, b). There are two reasons why we need to know the version of OpenSSL dynamically: 1) Enforce the correct compilation flags in pg_config.h 2) Select the correct library paths. OpenSSL keeps API compatibility in their stable branches, so 1. would not be an issue. I suspect as well that the installers we use for Postgres compilation are not going to change their layer in a minor version update. So having only 3 digits is enough. 2 likely not. I would just keep it at three for now. Could you send an updated patch? I would like to look at this stuff rather sooner than later as OpenSSL 1.0.2 goes EOL in a couple of months, so there is going to be demand for it. -- Michael
Вложения
Re: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails onWindows with Visual Studio 2017
От
Juan José Santamaría Flecha
Дата:
On Sat, Jun 22, 2019 at 4:44 AM Michael Paquier <michael@paquier.xyz> wrote: > > Could you send an updated patch? I would like to look at this stuff > rather sooner than later as OpenSSL 1.0.2 goes EOL in a couple of > months, so there is going to be demand for it. The updated patches include: 1. Comments for AddLibrary. 2. The function GetOpenSSLVersion returns: major, minor and fix numbers. 3. The version check will default to 1.1.X behaviour for 1.Y.X versions if Y > 0. Regards, Juan José Santamaría Flecha
Вложения
Re: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails onWindows with Visual Studio 2017
От
Michael Paquier
Дата:
On Mon, Jun 24, 2019 at 01:44:18PM +0200, Juan José Santamaría Flecha wrote: > The updated patches include: > > 1. Comments for AddLibrary. > 2. The function GetOpenSSLVersion returns: major, minor and fix numbers. > 3. The version check will default to 1.1.X behaviour for 1.Y.X > versions if Y > 0. Thanks for the new patch set! I have been looking at that in depths and I have adjusted the whole logic a bit here and there. At the end I found the logic changed in AddLibrary more confusing because the debugging suffix may change depending on if we use a release-quality build or not, so I have kept the original interface, and completed it with a logic allowing the scripts to grab all the libraries it expects. This has a small gain when using a non-debug installation as the library names are the same for Win32 and Win64 for >= 1.1.0. Also, your patch was not working with other versions of MSVC as the new routine got stuck into the 2017 class, so I had to move it, and I found that it was cleaner to just make it return a string made of the 3 first digits and to do direct string comparisons. Please note that it is not necessary to create versions for the back-branches yet. If necessary, I'll do that myself, but first let's make sure that we agree about the shape of the patch for HEAD. Attached is an updated version which I would be fine to commit. I have tested it with compilation linking to OpenSSL 1.0.2 and 1.1.0 on Win32 and the build is able to complete. This applies on HEAD only, where I have run all my tests. The patch is properly indented. What do you think? -- Michael
Вложения
Re: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails onWindows with Visual Studio 2017
От
Juan José Santamaría Flecha
Дата:
I will not have much available time for this list in the next few weeks, so as quick eye review: On Tue, Jun 25, 2019 at 10:08 AM Michael Paquier <michael@paquier.xyz> wrote: > > Thanks for the new patch set! I have been looking at that in depths > and I have adjusted the whole logic a bit here and there. At the end > I found the logic changed in AddLibrary more confusing because the > debugging suffix may change depending on if we use a release-quality > build or not, so I have kept the original interface, and completed it > with a logic allowing the scripts to grab all the libraries it > expects. This has a small gain when using a non-debug installation as > the library names are the same for Win32 and Win64 for >= 1.1.0. > It actually looks clearer and less intrusive (better) to me too. > Also, your patch was not working with other versions of MSVC as the > new routine got stuck into the 2017 class, so I had to move it, and I > found that it was cleaner to just make it return a string made of the > 3 first digits and to do direct string comparisons. > If you are using a string you will need padding, maybe mimic OPENSSL_VERSION_NUMBER [1]. > Please note that it is not necessary to create versions for the > back-branches yet. If necessary, I'll do that myself, but first let's > make sure that we agree about the shape of the patch for HEAD. > Attached is an updated version which I would be fine to commit. I > have tested it with compilation linking to OpenSSL 1.0.2 and 1.1.0 on > Win32 and the build is able to complete. This applies on HEAD only, > where I have run all my tests. The patch is properly indented. > There is a typo: s/versoin/version/ +# made of the three first digits of the OpenSSL versoin, which is enough Regards, Juan José Santamaría Flecha [1] https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_VERSION_NUMBER.html
Re: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails onWindows with Visual Studio 2017
От
Alvaro Herrera
Дата:
On 2019-Jun-25, Michael Paquier wrote: > Attached is an updated version which I would be fine to commit. I > have tested it with compilation linking to OpenSSL 1.0.2 and 1.1.0 on > Win32 and the build is able to complete. This applies on HEAD only, > where I have run all my tests. The patch is properly indented. In sub GenerateFiles, I would move the stanza for ->{openssl} next to the one for ->{gss} and keep the simpler lines all together (your patch leaves ->{nls} as a separate item, which is aesthetically unpleasing). -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails onWindows with Visual Studio 2017
От
Michael Paquier
Дата:
On Tue, Jun 25, 2019 at 09:52:43AM -0400, Alvaro Herrera wrote: > In sub GenerateFiles, I would move the stanza for ->{openssl} next to > the one for ->{gss} and keep the simpler lines all together (your patch > leaves ->{nls} as a separate item, which is aesthetically unpleasing). Right, thanks. This makes the flow cleaner. -- Michael
Вложения
Re: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails onWindows with Visual Studio 2017
От
Michael Paquier
Дата:
On Tue, Jun 25, 2019 at 11:43:29AM +0200, Juan José Santamaría Flecha wrote: > If you are using a string you will need padding, maybe mimic > OPENSSL_VERSION_NUMBER [1]. For now, I have taken the more simple approach to have three separate fields for the digit numbers, which is more than enough. So, after doing more tweaks and tests on it with the installers for 1.0.2, 1.1.0 and 1.1.1, I have committed the patch to master for now. The result is rather cool, as we can now adapt with future versions easily. I am planning to back-patch the thing down to 9.4 where OpenSSL 1.1.0 is supported, but first let's see if the buildfarm has anything to say. I don't expect any issues as we basically don't change the logic build for 1.0.2, but nobody is never careful enough with this stuff. -- Michael
Вложения
Re: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails onWindows with Visual Studio 2017
От
Michael Paquier
Дата:
On Wed, Jun 26, 2019 at 10:48:03AM +0900, Michael Paquier wrote: > I am planning to back-patch the thing down to 9.4 where OpenSSL 1.1.0 > is supported, but first let's see if the buildfarm has anything to > say. I don't expect any issues as we basically don't change the logic > build for 1.0.2, but nobody is never careful enough with this stuff. The buildfarm did not complain, so I have backpatched the thing down to 9.4. There were some conflicts, mainly: - On 9.4, we need to use USE_SSL to enable SSL in the builds. - In ~9.6, we also need to set HAVE_RAND_OPENSSL for RAND_OpenSSL() which is a function used by pgcrypto available in OpenSSL 1.1.0 and newer. This was removed in v10 as of fe0a0b59, so we can clean up a bit things here. -- Michael