[1/1] package/domoticz: fix build with RELRO
diff mbox series

Message ID 20191031102306.15433-1-fontaine.fabrice@gmail.com
State Superseded
Headers show
Series
  • [1/1] package/domoticz: fix build with RELRO
Related show

Commit Message

Fabrice Fontaine Oct. 31, 2019, 10:23 a.m. UTC
Fixes:
 - http://autobuild.buildroot.org/results/5c1ca3083ad672401d1e050c6c3a07b8c33b851d

Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
---
 ...xt-Add-USE_PRECOMPILED_HEADER-option.patch | 35 +++++++++++++++++++
 package/domoticz/domoticz.mk                  |  3 ++
 2 files changed, 38 insertions(+)
 create mode 100644 package/domoticz/0003-CMakeLists.txt-Add-USE_PRECOMPILED_HEADER-option.patch

Comments

Thomas Petazzoni Oct. 31, 2019, 12:43 p.m. UTC | #1
Hello Fabrice,

As usual, thanks for working on build issues.

On Thu, 31 Oct 2019 11:23:06 +0100
Fabrice Fontaine <fontaine.fabrice@gmail.com> wrote:

> diff --git a/package/domoticz/0003-CMakeLists.txt-Add-USE_PRECOMPILED_HEADER-option.patch b/package/domoticz/0003-CMakeLists.txt-Add-USE_PRECOMPILED_HEADER-option.patch
> new file mode 100644
> index 0000000000..831e4816e1
> --- /dev/null
> +++ b/package/domoticz/0003-CMakeLists.txt-Add-USE_PRECOMPILED_HEADER-option.patch
> @@ -0,0 +1,35 @@
> +From e2dfb2ece19748ba99ec8199fc902c0c9daff325 Mon Sep 17 00:00:00 2001
> +From: Fabrice Fontaine <fontaine.fabrice@gmail.com>
> +Date: Wed, 30 Oct 2019 18:55:48 +0100
> +Subject: [PATCH] CMakeLists.txt: Add USE_PRECOMPILED_HEADER option
> +
> +Add USE_PRECOMPILED_HEADER to allow the user to disable precompiled
> +header feature. Thanks to this, domoticz will be able to be built with
> +RELRO on buildroot

What is the relationship between using precompiled headers and relro
build problems ?

Thomas
Fabrice Fontaine Oct. 31, 2019, 1:01 p.m. UTC | #2
Dear Thomas,

Le jeu. 31 oct. 2019 à 13:43, Thomas Petazzoni
<thomas.petazzoni@bootlin.com> a écrit :
>
> Hello Fabrice,
>
> As usual, thanks for working on build issues.
>
> On Thu, 31 Oct 2019 11:23:06 +0100
> Fabrice Fontaine <fontaine.fabrice@gmail.com> wrote:
>
> > diff --git a/package/domoticz/0003-CMakeLists.txt-Add-USE_PRECOMPILED_HEADER-option.patch b/package/domoticz/0003-CMakeLists.txt-Add-USE_PRECOMPILED_HEADER-option.patch
> > new file mode 100644
> > index 0000000000..831e4816e1
> > --- /dev/null
> > +++ b/package/domoticz/0003-CMakeLists.txt-Add-USE_PRECOMPILED_HEADER-option.patch
> > @@ -0,0 +1,35 @@
> > +From e2dfb2ece19748ba99ec8199fc902c0c9daff325 Mon Sep 17 00:00:00 2001
> > +From: Fabrice Fontaine <fontaine.fabrice@gmail.com>
> > +Date: Wed, 30 Oct 2019 18:55:48 +0100
> > +Subject: [PATCH] CMakeLists.txt: Add USE_PRECOMPILED_HEADER option
> > +
> > +Add USE_PRECOMPILED_HEADER to allow the user to disable precompiled
> > +header feature. Thanks to this, domoticz will be able to be built with
> > +RELRO on buildroot
>
> What is the relationship between using precompiled headers and relro
> build problems ?
I took me some time to understand the issue.
It seems that linker flag such as -Wl,-z,relro should not be used when
building a precompiled header.
I discovered this through this domoticz commit message:
https://github.com/domoticz/domoticz/commit/68698e7a5dce80dea5a9b997d7e171b80bf566ac

However, this commit does not fix the issue on buildroot as
-Wl,-z,relro is not passed through CXXFLAGS but through our
toolchain-wrapper.
Updating toolchain/toolchain-wrapper.c to add the logic to manage
precompiled header seems more complicated than adding an option to
disable this feature from domoticz.
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Best Regards,

Fabrice
Thomas Petazzoni Nov. 4, 2019, 10:12 p.m. UTC | #3
Hello,

I'm adding a few more people in Cc: Matt, Yann, Arnout, Peter.

On Thu, 31 Oct 2019 14:01:33 +0100
Fabrice Fontaine <fontaine.fabrice@gmail.com> wrote:

> > What is the relationship between using precompiled headers and relro
> > build problems ?  
> I took me some time to understand the issue.
> It seems that linker flag such as -Wl,-z,relro should not be used when
> building a precompiled header.
> I discovered this through this domoticz commit message:
> https://github.com/domoticz/domoticz/commit/68698e7a5dce80dea5a9b997d7e171b80bf566ac
> 
> However, this commit does not fix the issue on buildroot as
> -Wl,-z,relro is not passed through CXXFLAGS but through our
> toolchain-wrapper.
> Updating toolchain/toolchain-wrapper.c to add the logic to manage
> precompiled header seems more complicated than adding an option to
> disable this feature from domoticz.

So, I looked into this, found
https://lists.fedoraproject.org/pipermail/devel/2011-July/153664.html
and https://bugzilla.redhat.com/show_bug.cgi?id=718719.

So it really seems like -Wl,-z,relzo should not be passed when
compiling. But our compiler wrapper passes it unconditionally when
RELRO is enabled.

Do we have a way to detect that the wrapper is called to compile or to
link ? I don't think we do.

So is Fabrice's patch an acceptable work-around, or do we want to fix
this more globally ?

Thanks for your feedback,

Thomas
Matthew Weber Nov. 5, 2019, 8:19 p.m. UTC | #4
Thomas,

On Mon, Nov 4, 2019 at 4:12 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> I'm adding a few more people in Cc: Matt, Yann, Arnout, Peter.
>
> On Thu, 31 Oct 2019 14:01:33 +0100
> Fabrice Fontaine <fontaine.fabrice@gmail.com> wrote:
>
> > > What is the relationship between using precompiled headers and relro
> > > build problems ?
> > I took me some time to understand the issue.
> > It seems that linker flag such as -Wl,-z,relro should not be used when
> > building a precompiled header.
> > I discovered this through this domoticz commit message:
> > https://github.com/domoticz/domoticz/commit/68698e7a5dce80dea5a9b997d7e171b80bf566ac
> >
> > However, this commit does not fix the issue on buildroot as
> > -Wl,-z,relro is not passed through CXXFLAGS but through our
> > toolchain-wrapper.
> > Updating toolchain/toolchain-wrapper.c to add the logic to manage
> > precompiled header seems more complicated than adding an option to
> > disable this feature from domoticz.
>
> So, I looked into this, found
> https://lists.fedoraproject.org/pipermail/devel/2011-July/153664.html
> and https://bugzilla.redhat.com/show_bug.cgi?id=718719.
>
> So it really seems like -Wl,-z,relzo should not be passed when
> compiling. But our compiler wrapper passes it unconditionally when
> RELRO is enabled.
>
> Do we have a way to detect that the wrapper is called to compile or to
> link ? I don't think we do.

Looks like we can just key on -xc-header or -xc++-header and
selectively enable the RELRO flags.  I'll do some builds and then send
a patch if that looks ok.

https://pastebin.com/XGc8bFxh

Matt
Arnout Vandecappelle Nov. 6, 2019, 1:07 p.m. UTC | #5
On 05/11/2019 21:19, Matthew Weber wrote:
> Thomas,
> 
> On Mon, Nov 4, 2019 at 4:12 PM Thomas Petazzoni
> <thomas.petazzoni@bootlin.com> wrote:
>>
>> Hello,
>>
>> I'm adding a few more people in Cc: Matt, Yann, Arnout, Peter.
>>
>> On Thu, 31 Oct 2019 14:01:33 +0100
>> Fabrice Fontaine <fontaine.fabrice@gmail.com> wrote:
>>
>>>> What is the relationship between using precompiled headers and relro
>>>> build problems ?
>>> I took me some time to understand the issue.
>>> It seems that linker flag such as -Wl,-z,relro should not be used when
>>> building a precompiled header.
>>> I discovered this through this domoticz commit message:
>>> https://github.com/domoticz/domoticz/commit/68698e7a5dce80dea5a9b997d7e171b80bf566ac
>>>
>>> However, this commit does not fix the issue on buildroot as
>>> -Wl,-z,relro is not passed through CXXFLAGS but through our
>>> toolchain-wrapper.
>>> Updating toolchain/toolchain-wrapper.c to add the logic to manage
>>> precompiled header seems more complicated than adding an option to
>>> disable this feature from domoticz.
>>
>> So, I looked into this, found
>> https://lists.fedoraproject.org/pipermail/devel/2011-July/153664.html
>> and https://bugzilla.redhat.com/show_bug.cgi?id=718719.
>>
>> So it really seems like -Wl,-z,relzo should not be passed when
>> compiling. But our compiler wrapper passes it unconditionally when
>> RELRO is enabled.
>>
>> Do we have a way to detect that the wrapper is called to compile or to
>> link ? I don't think we do.
> 
> Looks like we can just key on -xc-header or -xc++-header and
> selectively enable the RELRO flags.  I'll do some builds and then send
> a patch if that looks ok.
> 
> https://pastebin.com/XGc8bFxh

 I'm not so sure if that is a generic solution... As explained in [1]:

$ gcc -o /tmp/stdio.gch /usr/include/stdio.h
$ gcc -c -o /tmp/stdio.gch /usr/include/stdio.h -Wl,-z,relro
$ gcc -o /tmp/stdio.gch /usr/include/stdio.h -Wl,-z,relro
/usr/bin/ld: /usr/lib/gcc/x86_64-redhat-linux/9/../../../../lib64/crt1.o: in
function `_start':
(.text+0x24): undefined reference to `main'
collect2: error: ld returned 1 exit status

 The problem AFAICS is that if no -c or similar option is given, GCC decides
what needs to be done based on the rest of the arguments. If the rest of the
arguments include a -Wl,... option, it decides that linking needs to be done. If
the rest of the arguments are just header files, it decides to create a
precompiled header.

 I think a more appropriate solution is to patch domoticz to add the -c option
for building precompiled headers, which AFAICS should fix the issue.

 If we want to fix it at the toolchain level, I think it would be more
appropriate to handle it the way most distros do: by patching the spec file.

 Regards,
 Arnout


[1] https://lists.fedoraproject.org/pipermail/devel/2011-July/153864.html
Matthew Weber Nov. 6, 2019, 1:46 p.m. UTC | #6
Arnout,

On Wed, Nov 6, 2019 at 7:07 AM Arnout Vandecappelle <arnout@mind.be> wrote:
>
>
>
> On 05/11/2019 21:19, Matthew Weber wrote:
> > Thomas,
> >
> > On Mon, Nov 4, 2019 at 4:12 PM Thomas Petazzoni
> > <thomas.petazzoni@bootlin.com> wrote:
> >>
> >> Hello,
> >>
> >> I'm adding a few more people in Cc: Matt, Yann, Arnout, Peter.
> >>
> >> On Thu, 31 Oct 2019 14:01:33 +0100
> >> Fabrice Fontaine <fontaine.fabrice@gmail.com> wrote:
> >>
> >>>> What is the relationship between using precompiled headers and relro
> >>>> build problems ?
> >>> I took me some time to understand the issue.
> >>> It seems that linker flag such as -Wl,-z,relro should not be used when
> >>> building a precompiled header.
> >>> I discovered this through this domoticz commit message:
> >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_domoticz_domoticz_commit_68698e7a5dce80dea5a9b997d7e171b80bf566ac&d=DwICaQ&c=ilBQI1lupc9Y65XwNblLtw&r=y1sOV0GV8NZUkffv7oCRxs2Sd3nOBS-NxDM3NY8lOgs&m=DjIUeN2SXon2vr5dX4sl6rmhJvz5cmbRv5u_qS4doBw&s=ZPd1FlLwup8D1uVOQHfI-t-Whn2V55bYtrI9ZDYtrR4&e=
> >>>
> >>> However, this commit does not fix the issue on buildroot as
> >>> -Wl,-z,relro is not passed through CXXFLAGS but through our
> >>> toolchain-wrapper.
> >>> Updating toolchain/toolchain-wrapper.c to add the logic to manage
> >>> precompiled header seems more complicated than adding an option to
> >>> disable this feature from domoticz.
> >>
> >> So, I looked into this, found
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.fedoraproject.org_pipermail_devel_2011-2DJuly_153664.html&d=DwICaQ&c=ilBQI1lupc9Y65XwNblLtw&r=y1sOV0GV8NZUkffv7oCRxs2Sd3nOBS-NxDM3NY8lOgs&m=DjIUeN2SXon2vr5dX4sl6rmhJvz5cmbRv5u_qS4doBw&s=6ZTbPXMho6JGTlraCYmMM2nc_HlF9NsMkXyNc5GCgho&e=
> >> and https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.redhat.com_show-5Fbug.cgi-3Fid-3D718719&d=DwICaQ&c=ilBQI1lupc9Y65XwNblLtw&r=y1sOV0GV8NZUkffv7oCRxs2Sd3nOBS-NxDM3NY8lOgs&m=DjIUeN2SXon2vr5dX4sl6rmhJvz5cmbRv5u_qS4doBw&s=e_knYSAlNZ7WMHm6gZnbJn7Kj5mhCVIe5zWhgxkb23o&e= .
> >>
> >> So it really seems like -Wl,-z,relzo should not be passed when
> >> compiling. But our compiler wrapper passes it unconditionally when
> >> RELRO is enabled.
> >>
> >> Do we have a way to detect that the wrapper is called to compile or to
> >> link ? I don't think we do.
> >
> > Looks like we can just key on -xc-header or -xc++-header and
> > selectively enable the RELRO flags.  I'll do some builds and then send
> > a patch if that looks ok.
> >
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__pastebin.com_XGc8bFxh&d=DwICaQ&c=ilBQI1lupc9Y65XwNblLtw&r=y1sOV0GV8NZUkffv7oCRxs2Sd3nOBS-NxDM3NY8lOgs&m=DjIUeN2SXon2vr5dX4sl6rmhJvz5cmbRv5u_qS4doBw&s=5sBtnwkv13zwuv2FZm1hYBGap0wC14KeSXkljxL8JtQ&e=
>
>  I'm not so sure if that is a generic solution... As explained in [1]:
>
> $ gcc -o /tmp/stdio.gch /usr/include/stdio.h
> $ gcc -c -o /tmp/stdio.gch /usr/include/stdio.h -Wl,-z,relro
> $ gcc -o /tmp/stdio.gch /usr/include/stdio.h -Wl,-z,relro
> /usr/bin/ld: /usr/lib/gcc/x86_64-redhat-linux/9/../../../../lib64/crt1.o: in
> function `_start':
> (.text+0x24): undefined reference to `main'
> collect2: error: ld returned 1 exit status
>
>  The problem AFAICS is that if no -c or similar option is given, GCC decides
> what needs to be done based on the rest of the arguments. If the rest of the
> arguments include a -Wl,... option, it decides that linking needs to be done. If
> the rest of the arguments are just header files, it decides to create a
> precompiled header.
>
>  I think a more appropriate solution is to patch domoticz to add the -c option
> for building precompiled headers, which AFAICS should fix the issue.
>
>  If we want to fix it at the toolchain level, I think it would be more
> appropriate to handle it the way most distros do: by patching the spec file.

Agree, the little bit of testing I did ended up at the conclusion of
[1] where GCC infers a direction to take.

I believe the last time we looked at doing a custom spec file we
instead choose the wrapper path.    However I'd be curious how often
this will show up as it would be easier to patch domoticz and the
couple other packages for now.

>
>  Regards,
>  Arnout
>
>
> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.fedoraproject.org_pipermail_devel_2011-2DJuly_153864.html&d=DwICaQ&c=ilBQI1lupc9Y65XwNblLtw&r=y1sOV0GV8NZUkffv7oCRxs2Sd3nOBS-NxDM3NY8lOgs&m=DjIUeN2SXon2vr5dX4sl6rmhJvz5cmbRv5u_qS4doBw&s=h1AFIQjg3Krsi6JqWc3h0L83OT887ET8xVrRdcoBNqg&e=

Patch
diff mbox series

diff --git a/package/domoticz/0003-CMakeLists.txt-Add-USE_PRECOMPILED_HEADER-option.patch b/package/domoticz/0003-CMakeLists.txt-Add-USE_PRECOMPILED_HEADER-option.patch
new file mode 100644
index 0000000000..831e4816e1
--- /dev/null
+++ b/package/domoticz/0003-CMakeLists.txt-Add-USE_PRECOMPILED_HEADER-option.patch
@@ -0,0 +1,35 @@ 
+From e2dfb2ece19748ba99ec8199fc902c0c9daff325 Mon Sep 17 00:00:00 2001
+From: Fabrice Fontaine <fontaine.fabrice@gmail.com>
+Date: Wed, 30 Oct 2019 18:55:48 +0100
+Subject: [PATCH] CMakeLists.txt: Add USE_PRECOMPILED_HEADER option
+
+Add USE_PRECOMPILED_HEADER to allow the user to disable precompiled
+header feature. Thanks to this, domoticz will be able to be built with
+RELRO on buildroot
+
+Fixes:
+ - http://autobuild.buildroot.org/results/5c1ca3083ad672401d1e050c6c3a07b8c33b851d
+
+Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
+[Retrieved from:
+https://github.com/domoticz/domoticz/commit/e2dfb2ece19748ba99ec8199fc902c0c9daff325]
+---
+ CMakeLists.txt | 5 ++++-
+ 1 file changed, 4 insertions(+), 1 deletion(-)
+
+diff --git a/CMakeLists.txt b/CMakeLists.txt
+index cb6150ce4c..bd48872214 100644
+--- a/CMakeLists.txt
++++ b/CMakeLists.txt
+@@ -744,7 +744,10 @@ else()
+   target_link_libraries(domoticz -lrt -lresolv ${EXECINFO_LIBRARIES})
+ ENDIF()
+ 
+-ADD_PRECOMPILED_HEADER(domoticz "main/stdafx.h")
++option(USE_PRECOMPILED_HEADER "Use precompiled header feature to speed up build time " YES)
++if(USE_PRECOMPILED_HEADER)
++  ADD_PRECOMPILED_HEADER(domoticz "main/stdafx.h")
++ENDIF(USE_PRECOMPILED_HEADER)
+ 
+ IF(CMAKE_COMPILER_IS_GNUCXX)
+   option(USE_STATIC_LIBSTDCXX "Build with static libgcc/libstdc++ libraries" YES)
diff --git a/package/domoticz/domoticz.mk b/package/domoticz/domoticz.mk
index c0568c61c6..d8ccfeee5a 100644
--- a/package/domoticz/domoticz.mk
+++ b/package/domoticz/domoticz.mk
@@ -31,6 +31,9 @@  DOMOTICZ_CONF_OPTS += \
 	-DUSE_BUILTIN_SQLITE=OFF \
 	-DUSE_BUILTIN_MQTT=OFF
 
+# Disable precompiled header feature to fix build with RELRO
+DOMOTICZ_CONF_OPTS += -DUSE_PRECOMPILED_HEADER=OFF
+
 ifeq ($(BR2_PACKAGE_LIBUSB),y)
 DOMOTICZ_DEPENDENCIES += libusb
 endif