New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/Makefile.m32: add CURL_{LD,C}FLAGS_EXTRAS support #670

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@vszakats
Contributor

vszakats commented Feb 20, 2016

  • in sync with src/Makefile.m32
  • fix a comment typo
  • sync some whitespace with src/Makefile.m32
lib/Makefile.m32: add CURL_{LD,C}FLAGS_EXTRAS support
* in sync with src/Makefile.m32
* fix a comment typo
* sync some whitespace with src/Makefile.m32
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Feb 20, 2016

By analyzing the blame information on this pull request, we identified @gknauf, @bagder and @captain-caveman2k to be potential reviewers

By analyzing the blame information on this pull request, we identified @gknauf, @bagder and @captain-caveman2k to be potential reviewers

@vszakats

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Feb 20, 2016

Contributor

Extra options are useful to pass hardening options when building libcurl.dll. After this, those options will also apply to curl.exe.

Contributor

vszakats commented Feb 20, 2016

Extra options are useful to pass hardening options when building libcurl.dll. After this, those options will also apply to curl.exe.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 20, 2016

Member

CURL_CFLAG_EXTRAS seems fine. How are you using CURL_LDFLAG_EXTRAS? I notice you added it to lib/Makefile.m32 a while back.

Member

jay commented Feb 20, 2016

CURL_CFLAG_EXTRAS seems fine. How are you using CURL_LDFLAG_EXTRAS? I notice you added it to lib/Makefile.m32 a while back.

@vszakats

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Feb 20, 2016

Contributor

This is the actual value I'm using it with:
-static-libgcc -Wl,--nxcompat -Wl,--dynamicbase'

Contributor

vszakats commented Feb 20, 2016

This is the actual value I'm using it with:
-static-libgcc -Wl,--nxcompat -Wl,--dynamicbase'

@vszakats

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Feb 20, 2016

Contributor

It's useful for both the dll and exe.

Contributor

vszakats commented Feb 20, 2016

It's useful for both the dll and exe.

jay added a commit that referenced this pull request Feb 20, 2016

src/Makefile.m32: add CURL_{LD,C}FLAGS_EXTRAS support
Sync with lib/Makefile.m32 which already uses those variables.

Bug: #670
@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 20, 2016

Member

Ok. Thanks, landed in 91cfcc5.

Member

jay commented Feb 20, 2016

Ok. Thanks, landed in 91cfcc5.

@jay jay closed this Feb 20, 2016

@vszakats

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Feb 20, 2016

Contributor

Thank you Jay!

Contributor

vszakats commented Feb 20, 2016

Thank you Jay!

@vszakats vszakats deleted the vszakats:srcmingw branch Feb 20, 2016

@vszakats

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Mar 1, 2016

Contributor

After digging deeper into it, this patch turned out to be not enough. It seems inevitable to pass different LDFLAGS when linking libcurl.dll vs curl.exe, to properly enable ASLR.

For some reason, the linker ASLR option doesn't actually enable it for .exes at all, and needs a further, .exe specific hack, and it needs a further option for .dlls to unlock it fully in 64-bit mode.

Contributor

vszakats commented Mar 1, 2016

After digging deeper into it, this patch turned out to be not enough. It seems inevitable to pass different LDFLAGS when linking libcurl.dll vs curl.exe, to properly enable ASLR.

For some reason, the linker ASLR option doesn't actually enable it for .exes at all, and needs a further, .exe specific hack, and it needs a further option for .dlls to unlock it fully in 64-bit mode.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Mar 1, 2016

Member

Well nothing's gone out yet there's still a few weeks to change it. The thing is in the m32 lib makefile there's been $(CURL_LDFLAG_EXTRAS) and $(CURL_CFLAG_EXTRAS) for a while, I don't know if anyone's using that. In the regular makefiles CURL_CFLAG_EXTRAS is used for both lib and src and there's no CURL_LDFLAG_EXTRAS

Member

jay commented Mar 1, 2016

Well nothing's gone out yet there's still a few weeks to change it. The thing is in the m32 lib makefile there's been $(CURL_LDFLAG_EXTRAS) and $(CURL_CFLAG_EXTRAS) for a while, I don't know if anyone's using that. In the regular makefiles CURL_CFLAG_EXTRAS is used for both lib and src and there's no CURL_LDFLAG_EXTRAS

@vszakats

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Mar 1, 2016

Contributor

I see two possible solutions. First is not a beauty, but compatible and covers all the bases:

diff --git a/lib/Makefile.m32 b/lib/Makefile.m32
index fbc38de..eeb4311 100644
--- a/lib/Makefile.m32
+++ b/lib/Makefile.m32
@@ -80,7 +80,7 @@ CC    = $(CROSSPREFIX)gcc
 CFLAGS = $(CURL_CFLAG_EXTRAS) -g -O2 -Wall
 CFLAGS += -fno-strict-aliasing
 # comment LDFLAGS below to keep debug info
-LDFLAGS        = $(CURL_LDFLAG_EXTRAS) -s
+LDFLAGS        = $(CURL_LDFLAG_EXTRAS) $(CURL_LDFLAG_EXTRAS_DLL) -s
 AR     = $(CROSSPREFIX)ar
 RANLIB = $(CROSSPREFIX)ranlib
 RC     = $(CROSSPREFIX)windres
diff --git a/src/Makefile.m32 b/src/Makefile.m32
index 64c65a6..a6a5b77 100644
--- a/src/Makefile.m32
+++ b/src/Makefile.m32
@@ -92,7 +92,7 @@ CC    = $(CROSSPREFIX)gcc
 CFLAGS = $(CURL_CFLAG_EXTRAS) -g -O2 -Wall
 CFLAGS += -fno-strict-aliasing
 # comment LDFLAGS below to keep debug info
-LDFLAGS        = $(CURL_LDFLAG_EXTRAS) -s
+LDFLAGS        = $(CURL_LDFLAG_EXTRAS) $(CURL_LDFLAG_EXTRAS_EXE) -s
 AR     = $(CROSSPREFIX)ar
 RC     = $(CROSSPREFIX)windres
 RCFLAGS        = --include-dir=$(PROOT)/include -O COFF

Another, slightly more elegant~~, but breaking~~ option is to keep CURL_LDFLAG_EXTRAS for the .exe and add CURL_SHFLAG_EXTRAS for .dll:

diff --git a/lib/Makefile.m32 b/lib/Makefile.m32
index fbc38de..60b05c4 100644
--- a/lib/Makefile.m32
+++ b/lib/Makefile.m32
@@ -80,7 +80,7 @@ CC    = $(CROSSPREFIX)gcc
 CFLAGS = $(CURL_CFLAG_EXTRAS) -g -O2 -Wall
 CFLAGS += -fno-strict-aliasing
 # comment LDFLAGS below to keep debug info
-LDFLAGS    = $(CURL_LDFLAG_EXTRAS) -s
+LDFLAGS    = $(CURL_SHFLAG_EXTRAS) -s
 AR = $(CROSSPREFIX)ar
 RANLIB = $(CROSSPREFIX)ranlib
 RC = $(CROSSPREFIX)windres
Contributor

vszakats commented Mar 1, 2016

I see two possible solutions. First is not a beauty, but compatible and covers all the bases:

diff --git a/lib/Makefile.m32 b/lib/Makefile.m32
index fbc38de..eeb4311 100644
--- a/lib/Makefile.m32
+++ b/lib/Makefile.m32
@@ -80,7 +80,7 @@ CC    = $(CROSSPREFIX)gcc
 CFLAGS = $(CURL_CFLAG_EXTRAS) -g -O2 -Wall
 CFLAGS += -fno-strict-aliasing
 # comment LDFLAGS below to keep debug info
-LDFLAGS        = $(CURL_LDFLAG_EXTRAS) -s
+LDFLAGS        = $(CURL_LDFLAG_EXTRAS) $(CURL_LDFLAG_EXTRAS_DLL) -s
 AR     = $(CROSSPREFIX)ar
 RANLIB = $(CROSSPREFIX)ranlib
 RC     = $(CROSSPREFIX)windres
diff --git a/src/Makefile.m32 b/src/Makefile.m32
index 64c65a6..a6a5b77 100644
--- a/src/Makefile.m32
+++ b/src/Makefile.m32
@@ -92,7 +92,7 @@ CC    = $(CROSSPREFIX)gcc
 CFLAGS = $(CURL_CFLAG_EXTRAS) -g -O2 -Wall
 CFLAGS += -fno-strict-aliasing
 # comment LDFLAGS below to keep debug info
-LDFLAGS        = $(CURL_LDFLAG_EXTRAS) -s
+LDFLAGS        = $(CURL_LDFLAG_EXTRAS) $(CURL_LDFLAG_EXTRAS_EXE) -s
 AR     = $(CROSSPREFIX)ar
 RC     = $(CROSSPREFIX)windres
 RCFLAGS        = --include-dir=$(PROOT)/include -O COFF

Another, slightly more elegant~~, but breaking~~ option is to keep CURL_LDFLAG_EXTRAS for the .exe and add CURL_SHFLAG_EXTRAS for .dll:

diff --git a/lib/Makefile.m32 b/lib/Makefile.m32
index fbc38de..60b05c4 100644
--- a/lib/Makefile.m32
+++ b/lib/Makefile.m32
@@ -80,7 +80,7 @@ CC    = $(CROSSPREFIX)gcc
 CFLAGS = $(CURL_CFLAG_EXTRAS) -g -O2 -Wall
 CFLAGS += -fno-strict-aliasing
 # comment LDFLAGS below to keep debug info
-LDFLAGS    = $(CURL_LDFLAG_EXTRAS) -s
+LDFLAGS    = $(CURL_SHFLAG_EXTRAS) -s
 AR = $(CROSSPREFIX)ar
 RANLIB = $(CROSSPREFIX)ranlib
 RC = $(CROSSPREFIX)windres
@vszakats

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Mar 1, 2016

Contributor

Updated the previous. Neither option is breaking.

Contributor

vszakats commented Mar 1, 2016

Updated the previous. Neither option is breaking.

@vszakats

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Mar 1, 2016

Contributor

Taking libssh2 into account *_LDFLAG_EXTRAS is used for libs there, so while the second option looks cleaner and non-breaking, it might still be confusing. Leaning towards the less-elegant, first solution.

Contributor

vszakats commented Mar 1, 2016

Taking libssh2 into account *_LDFLAG_EXTRAS is used for libs there, so while the second option looks cleaner and non-breaking, it might still be confusing. Leaning towards the less-elegant, first solution.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Mar 1, 2016

Member

The first one is easier to understand but I think the symbols look better as CURL__LDFLAG_EXTRAS.

Side note, the mingw makefile.m32 build process is not documented very well, I think. I checked in INSTALL and many of the "chained" style options that can be used are not listed. For example right now all of this is accepted:
-dyn, -ares, -sync, -rtmp, -ssh2, -ssl, -srp, -zlib, -idn, -winidn, -sspi, -ldaps, -ipv6, -winssl, -nghttp2
if I was trying to compile using that build process but also use say, nghttp2 it would take some time to figure out. So I will add some note about that once we address this issue.

I think we could document these like:
Extra C flags can be passed by using CURL_CFLAG_EXTRAS.
Extra linker flags can be passed by using CURL_EXE_LDFLAG_EXTRAS for the EXE, CURL_DLL_LDFLAG_EXTRAS for the DLL and CURL_LDFLAG_EXTRAS for both.

Member

jay commented Mar 1, 2016

The first one is easier to understand but I think the symbols look better as CURL__LDFLAG_EXTRAS.

Side note, the mingw makefile.m32 build process is not documented very well, I think. I checked in INSTALL and many of the "chained" style options that can be used are not listed. For example right now all of this is accepted:
-dyn, -ares, -sync, -rtmp, -ssh2, -ssl, -srp, -zlib, -idn, -winidn, -sspi, -ldaps, -ipv6, -winssl, -nghttp2
if I was trying to compile using that build process but also use say, nghttp2 it would take some time to figure out. So I will add some note about that once we address this issue.

I think we could document these like:
Extra C flags can be passed by using CURL_CFLAG_EXTRAS.
Extra linker flags can be passed by using CURL_EXE_LDFLAG_EXTRAS for the EXE, CURL_DLL_LDFLAG_EXTRAS for the DLL and CURL_LDFLAG_EXTRAS for both.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Mar 1, 2016

Member

nevermind I took another look CURL_LDFLAG_EXTRAS_DLL looks fine, just took a second to adjust :)

Member

jay commented Mar 1, 2016

nevermind I took another look CURL_LDFLAG_EXTRAS_DLL looks fine, just took a second to adjust :)

@vszakats

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Mar 1, 2016

Contributor

Cool, I was thinking about CURL_LDFLAG_DLL_EXTRAS, but settled on the above :) Easier to grep.

I've tested almost all of these options (except -dyn and -sync) and can confirm they work fine, nice to have them documented, thanks!

I'll prepare a PR with the first option.

Contributor

vszakats commented Mar 1, 2016

Cool, I was thinking about CURL_LDFLAG_DLL_EXTRAS, but settled on the above :) Easier to grep.

I've tested almost all of these options (except -dyn and -sync) and can confirm they work fine, nice to have them documented, thanks!

I'll prepare a PR with the first option.

jay added a commit that referenced this pull request Mar 1, 2016

makefile.m32: allow to pass .dll/.exe-specific LDFLAGS
using envvars `CURL_LDFLAG_EXTRAS_DLL` and
`CURL_LDFLAG_EXTRAS_EXE` respectively. This
is useful f.e. to pass ASLR-related extra
options, that are required to make this
feature work when using the mingw toolchain.

Ref: #670 (comment)

Closes #689
@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Mar 1, 2016

Member

Thanks, just landed. Can you take a look at docs/INSTALL, and tell me if the directions look right to you, because I just tried them and they don't work for me.

mingw32-make mingw32
mingw32-make: *** No rule to make target 'mingw32'. Stop.

How are you building using these makefiles? I may have already asked you this but I forgot.

Member

jay commented Mar 1, 2016

Thanks, just landed. Can you take a look at docs/INSTALL, and tell me if the directions look right to you, because I just tried them and they don't work for me.

mingw32-make mingw32
mingw32-make: *** No rule to make target 'mingw32'. Stop.

How are you building using these makefiles? I may have already asked you this but I forgot.

@vszakats

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Mar 1, 2016

Contributor

Thank you for the quick merge!

Here's the complete formula I use to build curl: https://github.com/vszakats/harbour-deps/blob/master/curl.sh

(Note that MSYS2/sh is not required, this is the umpteenth revision of this script which originally started as a plain batch file, with equally good results.)

The error you're getting happens when trying to build a bare (non-release) repo (Git clone or GitHub source archive). These come without a Makefile. To make it working, buildconf.bat needs to be started first. Or maybe buildconf.sh, though I haven't tried that option yet.

INSTALL looks correct, but of course there are more options available. I haven't tried the non-native LDAP options yet, so can't verify those. (Windows LDAP worked fine for me so far)

Contributor

vszakats commented Mar 1, 2016

Thank you for the quick merge!

Here's the complete formula I use to build curl: https://github.com/vszakats/harbour-deps/blob/master/curl.sh

(Note that MSYS2/sh is not required, this is the umpteenth revision of this script which originally started as a plain batch file, with equally good results.)

The error you're getting happens when trying to build a bare (non-release) repo (Git clone or GitHub source archive). These come without a Makefile. To make it working, buildconf.bat needs to be started first. Or maybe buildconf.sh, though I haven't tried that option yet.

INSTALL looks correct, but of course there are more options available. I haven't tried the non-native LDAP options yet, so can't verify those. (Windows LDAP worked fine for me so far)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment