Re: Review Request 52695: Harden libprocess

[Benjamin Mahler <bm...@apache.org>]

> On Nov. 2, 2016, 9:32 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/Makefile.am
> > Lines 30 (patched)
> > <https://reviews.apache.org/r/52695/diff/4/?file=1551006#file1551006line30>
> >
> >     I am not a big fan of unconditionally omitting frame pointers as this gives the optimizer one less register to work with. Unfortunately one cannot easily tell the actual impact of this from the info here. Is this strictly needed here or just nice to have?
> 
> James Peach wrote:
>     The performance benefit of omitting frame pointers is likely to be marginal on x64_64, if it is a win at all. The rationale for adding this is that it makes stack walking reliable in all cases, so debugability is improved and you can get reasonable results when uting `perf`. Since most users will build with default options I suggested to Aaron that we should make it the default.
> 
> Benjamin Bannier wrote:
>     Thanks James, that makes sense.
>     
>     Since this seems all related to debugability what about enabling it _only for builds with `--enable-debug`_ (e.g., perf results already now also don't necessarily give full info w/o debug symbols)? Tangentially related, tcmalloc can fail in debug builds with omitted frame pointers, so disabling `omit-frame-pointer` in debug builds might safe us from some future headaches, https://bugs.chromium.org/p/chromium/issues/detail?id=636489.
>     
>     `stack-protector-strong` can significantly increase the binary size, and we should either only enable it for e.g., debug builds, or give users a `configure` knob to disable it.
>     
>     For using `FORTIFY_SOURCE` I think we also need be a little more careful. Support for it is somewhat broken in clang (https://llvm.org/bugs/show_bug.cgi?id=16821), it only has useful effects in builds with some level of optimization, and can e.g., mess up reports from sanitizers injected by users. I can see good uses for a `configure` flag to disable this compiler flag, but I am not sure what the default should be.
> 
> Aaron Wood wrote:
>     Going to drop this since we've all agreed on Slack to have the frame pointer modification done in a separate patch.

Looks like there wasn't a patch for `-fno-omit-frame-pointer`?

Filed: https://issues.apache.org/jira/browse/MESOS-8908


- Benjamin


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


On Nov. 30, 2016, 8:52 p.m., Aaron Wood wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52695/
> -----------------------------------------------------------
> 
> (Updated Nov. 30, 2016, 8:52 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6229
>     https://issues.apache.org/jira/browse/MESOS-6229
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add hardened flags for libprocess.
> Take compile flag macro at 391cb680171d3889965b1ead43d3a326c913bc25.
> The macro at 1a869696e4129279f7b99c3f9052717354b79a86 requires autoconf 2.64 which breaks on CentOS 6.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 9d496b8 
>   3rdparty/libprocess/configure.ac e65e5ca 
>   3rdparty/libprocess/m4/ax_check_compile_flag.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/52695/diff/9/
> 
> 
> Testing
> -------
> 
> Compared the benchmarks with and without the flags being used. Also did a comparsion with the flags being used with and without optimizations and without the flags being used with and without optimizations. Overall the performance hit was very small with a 3-8% overhead (optimizations brings this down slightly). Most benchmarks were about 5% (or less) slower.
> 
> 
> File Attachments
> ----------------
> 
> --enable-optimized with hardening applied
>   https://reviews.apache.org/media/uploaded/files/2016/11/02/875c9e6e-c73b-4e3c-8265-0f7c6dc00351__hardened-optimized.txt
> Hardening applied but no --enable-optimized
>   https://reviews.apache.org/media/uploaded/files/2016/11/02/932d28a7-2d31-471a-b438-647841a6853c__hardened-unoptimized.txt
> --enable-optimized with no hardening applied
>   https://reviews.apache.org/media/uploaded/files/2016/11/02/896944ea-9b31-4d62-b1b9-97fb4700a882__optimized.txt
> No hardening applied and no --enable-optimized
>   https://reviews.apache.org/media/uploaded/files/2016/11/02/b32667ce-3e3b-4d2b-b4f8-4c2404a0fc1c__unoptimized.txt
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>