Re: Review Request 60546: Harden Mesos when building with cmake.

[Joseph Wu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60546/#review184580
---


Ship it!




I'm going to apply this review in spirit (because the CMake build was rewritten 
underneath it).

I'll make the below changes before committing.


cmake/CompilationConfigure.cmake
Lines 202-212 (patched)


Going to replace this:
`set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} `

With this:
`string(APPEND CMAKE_CXX_FLAGS " `



cmake/CompilationConfigure.cmake
Lines 219-223 (patched)


Going to replace this with the cross-platform equivalent:
```
set(CMAKE_POSITION_INDEPENDENT_CODE TRUE)
```

See: 
https://cmake.org/cmake/help/v3.0/prop_tgt/POSITION_INDEPENDENT_CODE.html



cmake/MesosConfigure.cmake
Lines 65-70 (patched)


Ditto with above.


- Joseph Wu


On July 5, 2017, 9:06 a.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60546/
> ---
> 
> (Updated July 5, 2017, 9:06 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-7737
> https://issues.apache.org/jira/browse/MESOS-7737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Apply stack protectors (stronger stack protectors with newer compilers), 
> position independent code suitable for linking into executables, security 
> warnings, and generate position independent executables.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3fa2e2f3b 
>   cmake/MesosConfigure.cmake 5ecad2c0f 
> 
> 
> Diff: https://reviews.apache.org/r/60546/diff/2/
> 
> 
> Testing
> ---
> 
> `mkdir build && cd build && cmake .. && make -j$(nproc) && make check 
> -j$(nproc)`
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60546: Harden Mesos when building with cmake.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60546/#review179946
---



Patch looks great!

Reviews applied: [60546]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On July 5, 2017, 4:06 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60546/
> ---
> 
> (Updated July 5, 2017, 4:06 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-7737
> https://issues.apache.org/jira/browse/MESOS-7737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Apply stack protectors (stronger stack protectors with newer compilers), 
> position independent code suitable for linking into executables, security 
> warnings, and generate position independent executables.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3fa2e2f3b 
>   cmake/MesosConfigure.cmake 5ecad2c0f 
> 
> 
> Diff: https://reviews.apache.org/r/60546/diff/2/
> 
> 
> Testing
> ---
> 
> `mkdir build && cd build && cmake .. && make -j$(nproc) && make check 
> -j$(nproc)`
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60546: Harden Mesos when building with cmake.

[Mesos Reviewbot Windows]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60546/#review179738
---



Bad patch!

Reviews applied: [60546]

Failed command: python support/apply-reviews.py -n -r 60546

Error:
Traceback (most recent call last):
  File "support/apply-reviews.py", line 417, in 
main()
  File "support/apply-reviews.py", line 412, in main
reviewboard(options)
  File "support/apply-reviews.py", line 402, in reviewboard
apply_review(options)
  File "support/apply-reviews.py", line 158, in apply_review
fetch_patch(options)
  File "support/apply-reviews.py", line 215, in fetch_patch
shell(cmd, options['dry_run'])
UnboundLocalError: local variable 'cmd' referenced before assignment

Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/125/console

- Mesos Reviewbot Windows


On July 5, 2017, 9:06 a.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60546/
> ---
> 
> (Updated July 5, 2017, 9:06 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-7737
> https://issues.apache.org/jira/browse/MESOS-7737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Apply stack protectors (stronger stack protectors with newer compilers), 
> position independent code suitable for linking into executables, security 
> warnings, and generate position independent executables.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3fa2e2f3b 
>   cmake/MesosConfigure.cmake 5ecad2c0f 
> 
> 
> Diff: https://reviews.apache.org/r/60546/diff/2/
> 
> 
> Testing
> ---
> 
> `mkdir build && cd build && cmake .. && make -j$(nproc) && make check 
> -j$(nproc)`
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60546: Harden Mesos when building with cmake.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60546/#review179721
---


Ship it!




Ship It!

- Andrew Schwartzmeyer


On July 5, 2017, 4:06 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60546/
> ---
> 
> (Updated July 5, 2017, 4:06 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-7737
> https://issues.apache.org/jira/browse/MESOS-7737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Apply stack protectors (stronger stack protectors with newer compilers), 
> position independent code suitable for linking into executables, security 
> warnings, and generate position independent executables.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3fa2e2f3b 
>   cmake/MesosConfigure.cmake 5ecad2c0f 
> 
> 
> Diff: https://reviews.apache.org/r/60546/diff/2/
> 
> 
> Testing
> ---
> 
> `mkdir build && cd build && cmake .. && make -j$(nproc) && make check 
> -j$(nproc)`
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60546: Harden Mesos when building with cmake.

[Aaron Wood via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60546/
---

(Updated July 5, 2017, 4:06 p.m.)


Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Joseph Wu.


Changes
---

Use CHECK_CXX_COMPILER_FLAG to detect support for stack protectors and add more 
documentation surrounding why we want position independent code.


Bugs: MESOS-7737
https://issues.apache.org/jira/browse/MESOS-7737


Repository: mesos


Description
---

Apply stack protectors (stronger stack protectors with newer compilers), 
position independent code suitable for linking into executables, security 
warnings, and generate position independent executables.


Diffs (updated)
-

  cmake/CompilationConfigure.cmake 3fa2e2f3b 
  cmake/MesosConfigure.cmake 5ecad2c0f 


Diff: https://reviews.apache.org/r/60546/diff/2/

Changes: https://reviews.apache.org/r/60546/diff/1-2/


Testing
---

`mkdir build && cd build && cmake .. && make -j$(nproc) && make check 
-j$(nproc)`


Thanks,

Aaron Wood

Re: Review Request 60546: Harden Mesos when building with cmake.

[Aaron Wood via Review Board]

> On July 4, 2017, 3:06 p.m., Benjamin Bannier wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 213-214 (patched)
> > 
> >
> > Not sure this is needed, see below.

Relating to my comment below, I saw that `-pie` was not used to generate a 
fully position independent executable. Only `-fPIC` was used to generate 
position independent code for dynamic linking.


> On July 4, 2017, 3:06 p.m., Benjamin Bannier wrote:
> > cmake/MesosConfigure.cmake
> > Lines 65 (patched)
> > 
> >
> > It appears as if just setting
> > 
> > set(CMAKE_POSITION_INDEPENDENT_CODE TRUE)
> > 
> > here as well would take care of both the compiler and linker flags; 
> > could you check if it does and potentially streamline this part. It would 
> > also be great if we could have a comment explaining why we enforce position 
> > independent code.

>From my testing I found that `-fPIC` was always set when 
>`set(CMAKE_POSITION_INDEPENDENT_CODE TRUE)` is used but `-fPIE` is never set. 
>I figured I'd add it here since it's very relevant when building static libs. 
>I'll definitely add more comments surrounding this stuff.

Also, I'm assuming that the static libs built here are only for linking into 
executables. Do you think this will hold true going forward?


- Aaron


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60546/#review179581
---


On June 29, 2017, 6:18 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60546/
> ---
> 
> (Updated June 29, 2017, 6:18 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-7737
> https://issues.apache.org/jira/browse/MESOS-7737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Apply stack protectors (stronger stack protectors with newer compilers), 
> position independent code suitable for linking into executables, security 
> warnings, and generate position independent executables.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3fa2e2f3b 
>   cmake/MesosConfigure.cmake 5ecad2c0f 
> 
> 
> Diff: https://reviews.apache.org/r/60546/diff/1/
> 
> 
> Testing
> ---
> 
> `mkdir build && cd build && cmake .. && make -j$(nproc) && make check 
> -j$(nproc)`
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60546: Harden Mesos when building with cmake.

[Benjamin Bannier]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60546/#review179581
---




cmake/CompilationConfigure.cmake
Lines 207-210 (patched)


Let's not tie support to arbitrary versions (e.g., this condition rejects 
clang-4.0 just like gcc-4.3 which doesn't seem like the intention).

Instead let's use `CHECK_CXX_COMPILER_FLAG` to check compiler support for 
the flags you are passing, and potentially output a warning if we cannot enable 
any stack protection.



cmake/CompilationConfigure.cmake
Lines 213-214 (patched)


Not sure this is needed, see below.



cmake/MesosConfigure.cmake
Lines 65 (patched)


It appears as if just setting

set(CMAKE_POSITION_INDEPENDENT_CODE TRUE)

here as well would take care of both the compiler and linker flags; could 
you check if it does and potentially streamline this part. It would also be 
great if we could have a comment explaining why we enforce position independent 
code.


- Benjamin Bannier


On June 29, 2017, 8:18 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60546/
> ---
> 
> (Updated June 29, 2017, 8:18 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-7737
> https://issues.apache.org/jira/browse/MESOS-7737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Apply stack protectors (stronger stack protectors with newer compilers), 
> position independent code suitable for linking into executables, security 
> warnings, and generate position independent executables.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3fa2e2f3b 
>   cmake/MesosConfigure.cmake 5ecad2c0f 
> 
> 
> Diff: https://reviews.apache.org/r/60546/diff/1/
> 
> 
> Testing
> ---
> 
> `mkdir build && cd build && cmake .. && make -j$(nproc) && make check 
> -j$(nproc)`
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Review Request 60546: Harden Mesos when building with cmake.

[Aaron Wood via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60546/
---

Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.


Bugs: MESOS-7737
https://issues.apache.org/jira/browse/MESOS-7737


Repository: mesos


Description
---

Apply stack protectors (stronger stack protectors with newer compilers), 
position independent code suitable for linking into executables, security 
warnings, and generate position independent executables.


Diffs
-

  cmake/CompilationConfigure.cmake 3fa2e2f3b 
  cmake/MesosConfigure.cmake 5ecad2c0f 


Diff: https://reviews.apache.org/r/60546/diff/1/


Testing
---

`mkdir build && cd build && cmake .. && make -j$(nproc) && make check 
-j$(nproc)`


Thanks,

Aaron Wood