Compile native extensions with '-O2 -g' flags #2100

ilyazub · 2020-10-16T15:48:20Z

Resolves #2022.

In my case parsing of a big HTML file decreased from 1.1 seconds to 210 ms.

@flavorjones Regarding the final challenge, I haven't cleaned up process_recipe and configure_options, but I can try if you add some guidance or vision of the result.

And thanks for the detailed issue description. It was pretty straightforward to make this change.

codeclimate · 2020-10-16T15:52:09Z

Code Climate has analyzed commit abd5669 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 94.3% (0.0% change).

View more on Code Climate.

AppVeyorBot · 2020-10-16T16:17:24Z

✅ Build nokogiri 1.0.712 completed (commit 6f7a4458b0 by @ilyazub)

flavorjones · 2020-10-16T16:37:46Z

Thank you for submitting this! Looks good, will likely merge this weekend and ship in the next v1.11.0 release candidate.

flavorjones · 2020-10-16T19:07:07Z

Fascinating! When compiled with -O2 the precompiled native gem crashes on musl-based systems (like Alpine).

I'll try to take a look over the weekend.

flavorjones · 2020-10-16T19:30:55Z

Threw it into a debugger and here's a partial stack walkback:

#0  0x000000000003a9a0 in ?? ()
#1  0x00007f08ac7b7d19 in vsnprintf (__ap=0x7ffc529b51b0, 
    __fmt=0x7f08ac8b3f70 "Opening and ending tag mismatch: %s line %d and %s\n", __n=150, __s=0x556dfb894e00 "\350\253l\260\b\177")
    at /usr/include/x86_64-linux-gnu/bits/stdio2.h:80
#2  __xmlRaiseError (schannel=0x7f08ac7a4530 <Nokogiri_error_array_pusher>, schannel@entry=0x0, channel=channel@entry=0x0, 
    data=0x556dfb8756e0, data@entry=0x0, ctx=ctx@entry=0x556dfb88fd20, nod=nod@entry=0x0, domain=domain@entry=1, code=76, 
    level=XML_ERR_FATAL, file=0x0, line=0, str1=0x556dfb89407f "track", str2=0x556dfb894077 "test", str3=0x0, int1=2, col=0, 
    msg=0x7f08ac8b3f70 "Opening and ending tag mismatch: %s line %d and %s\n") at error.c:501
#3  0x00007f08ac7bc915 in xmlFatalErrMsgStrIntStr (ctxt=0x556dfb88fd20, error=<optimized out>, msg=<optimized out>, 
    str1=<optimized out>, val=<optimized out>, str2=<optimized out>) at parser.c:720
#4  0x00007f08ac7ca57b in xmlParseEndTag2 (ctxt=ctxt@entry=0x556dfb88fd20, prefix=0x0, URI=0x0, nsNr=0, tlen=0, line=<optimized out>)
    at parser.c:9679
#5  0x00007f08ac7caa07 in xmlParseElementEnd (ctxt=ctxt@entry=0x556dfb88fd20) at parser.c:10074
#6  0x00007f08ac7cef33 in xmlParseContent (ctxt=0x556dfb88fd20) at parser.c:9860
#7  0x00007f08ac7cf6c0 in xmlParseElement (ctxt=0x556dfb88fd20) at parser.c:9912
#8  xmlParseElement (ctxt=0x556dfb88fd20) at parser.c:9909
#9  0x00007f08ac7d2ec2 in xmlParseDocument (ctxt=ctxt@entry=0x556dfb88fd20) at parser.c:10748

So it looks like this is segfaulting on a vsnprintf call, and I'm wondering if it's got the same root cause as another bug we had to work around to get native compilation working on musl: see @larskanis's work at a3d9438.

because this makes the precompiled (glibc) library segfault on musl. Related to a3d9438 and sparklemotion#2022

flavorjones · 2020-10-16T20:40:08Z

This seems to be related to:

-O2 in gcc turns on -D_FORTIFY_SOURCE=2
and musl libc doesn't support _FORTIFY_SOURCE yet

So I've pushed an additional commit that turns this macro off, and musl seems to now work. Let's watch what CI says ...

EDIT: CI went green

For context, here's a thing that explains _FORTIFY_SOURCE pretty well: https://p0lycarp.github.io/2019/fortify/

And here's some context around musl support for it:

AppVeyorBot · 2020-10-16T21:03:58Z

✅ Build nokogiri 1.0.713 completed (commit 7faad5c213 by @flavorjones)

ilyazub · 2020-10-18T14:04:23Z

This seems to be related to:

-O2 in gcc turns on -D_FORTIFY_SOURCE=2

and musl libc doesn't support _FORTIFY_SOURCE yet

libiconv in master compiles successfully with -O2 -g flags.

Where is it defined that -O2 in gcc turns on -D_FORTIFY_SOURCE=2? It seems that -D_FORTIFY_SOURCE is available with -O1 or higher but -O2 itself doesn't turn it on.

I've modified example code from blog post on _FORTIFY_SOURCE to print console message when that macro is defined.

#include <stdio.h>
#include <string.h>

int main(int argc, char** argv) {
#ifdef _FORTIFY_SOURCE
    printf("_FORTIFY_SOURCE defined\n");
#endif

    char buf[5];
    strcpy(buf, argv[1]);
    puts(buf);
}

The output shows that -O2 doesn't turn on _FORTIFY_SOURCE (from my very limited understanding of C).

$ gcc ./fmt.c -o fmt

$ ./fmt nokogiri_with_o
nokogiri_with_o
Segmentation fault (core dumped)

$ gcc ./fmt.c -o fmt -O2

$ ./fmt nokogiri_with_o
nokogiri_with_o
Segmentation fault (core dumped)

$ gcc ./fmt.c -o fmt -O2 -D_FORTIFY_SOURCE=1

$ ./fmt nokogiri_with_o
_FORTIFY_SOURCE defined
*** buffer overflow detected ***: terminated
Aborted (core dumped)

$ gcc ./fmt.c -o fmt -O2 -D_FORTIFY_SOURCE=2

$ ./fmt nokogiri_with_o
_FORTIFY_SOURCE defined
*** buffer overflow detected ***: terminated

Checked with gcc 10.2.1

$ gcc -v
Using built-in specs.
COLLECT_GCC=/usr/bin/gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/10/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-redhat-linux
Configured with: ../configure --enable-bootstrap --enable-languages=c,c++,fortran,objc,obj-c++,ada,go,d,lto --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --enable-checking=release --enable-multilib --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-gcc-major-version-only --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --with-isl --enable-offload-targets=nvptx-none --without-cuda-driver --enable-gnu-indirect-function --enable-cet --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 10.2.1 20200723 (Red Hat 10.2.1-1) (GCC)

flavorjones · 2020-10-18T16:31:20Z

From man gcc on my ubuntu-based system:

       -O2 Optimize even more.  GCC performs nearly all supported optimizations that do not involve a
           space-speed tradeoff.  As compared to -O, this option increases both compilation time and the
           performance of the generated code.

           -O2 turns on all optimization flags specified by -O.  It also turns on the following
           optimization flags:

           -falign-functions  -falign-jumps -falign-labels  -falign-loops -fcaller-saves -fcode-hoisting
           -fcrossjumping -fcse-follow-jumps  -fcse-skip-blocks -fdelete-null-pointer-checks
           -fdevirtualize  -fdevirtualize-speculatively -fexpensive-optimizations -fgcse  -fgcse-lm
           -fhoist-adjacent-loads -finline-small-functions -findirect-inlining -fipa-bit-cp  -fipa-cp
           -fipa-icf -fipa-ra  -fipa-sra  -fipa-vrp -fisolate-erroneous-paths-dereference -flra-remat
           -foptimize-sibling-calls -foptimize-strlen -fpartial-inlining -fpeephole2
           -freorder-blocks-algorithm=stc -freorder-blocks-and-partition  -freorder-functions
           -frerun-cse-after-loop -fschedule-insns  -fschedule-insns2 -fsched-interblock  -fsched-spec
           -fstore-merging -fstrict-aliasing -fthread-jumps -ftree-builtin-call-dce -ftree-pre
           -ftree-switch-conversion  -ftree-tail-merge -ftree-vrp

           Please note the warning under -fgcse about invoking -O2 on programs that use computed gotos.

           NOTE: In Ubuntu 8.10 and later versions, -D_FORTIFY_SOURCE=2 is set by default, and is
           activated when -O is set to 2 or higher.  This enables additional compile-time and run-time
           checks for several libc functions.  To disable, specify either -U_FORTIFY_SOURCE or
           -D_FORTIFY_SOURCE=0.

ilyazub · 2020-10-18T19:41:32Z

Oh, I see. -O2 turns on -D_FORTIFY_SOURCE=2 on Alpine and Ubuntu versions of gcc. At least in flavorjones/nokogiri-test:alpine and larskanis/rake-compiler-dock-mri-x86_64-linux:1.0.0 docker images.

Output from larskanis/rake-compiler-dock-mri-x86_64-linux:1.0.0 docker image.

# gcc ./fmt.c -o fmt

# ./fmt nokogiri_with_o
nokogiri_with_o
*** stack smashing detected ***: ./fmt terminated
Aborted (core dumped)

# gcc ./fmt.c -o fmt -O2

# ./fmt nokogiri_with_o
_FORTIFY_SOURCE defined
*** buffer overflow detected ***: ./fmt terminated
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x777e5)[0x7f1a353677e5]
/lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x5c)[0x7f1a3540915c]
/lib/x86_64-linux-gnu/libc.so.6(+0x117160)[0x7f1a35407160]
/lib/x86_64-linux-gnu/libc.so.6(+0x1164b2)[0x7f1a354064b2]
./fmt[0x400533]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f1a35310830]
./fmt[0x400589]
======= Memory map: ========
00400000-00401000 r-xp 00000000 fd:01 1207836                            /tmp/build/fmt
00600000-00601000 r--p 00000000 fd:01 1207836                            /tmp/build/fmt
00601000-00602000 rw-p 00001000 fd:01 1207836                            /tmp/build/fmt
00d1d000-00d3e000 rw-p 00000000 00:00 0                                  [heap]
7f1a350da000-7f1a350f0000 r-xp 00000000 fd:01 133625                     /lib/x86_64-linux-gnu/libgcc_s.so.1
7f1a350f0000-7f1a352ef000 ---p 00016000 fd:01 133625                     /lib/x86_64-linux-gnu/libgcc_s.so.1
7f1a352ef000-7f1a352f0000 rw-p 00015000 fd:01 133625                     /lib/x86_64-linux-gnu/libgcc_s.so.1
7f1a352f0000-7f1a354b0000 r-xp 00000000 fd:01 133604                     /lib/x86_64-linux-gnu/libc-2.23.so
7f1a354b0000-7f1a356b0000 ---p 001c0000 fd:01 133604                     /lib/x86_64-linux-gnu/libc-2.23.so
7f1a356b0000-7f1a356b4000 r--p 001c0000 fd:01 133604                     /lib/x86_64-linux-gnu/libc-2.23.so
7f1a356b4000-7f1a356b6000 rw-p 001c4000 fd:01 133604                     /lib/x86_64-linux-gnu/libc-2.23.so
7f1a356b6000-7f1a356ba000 rw-p 00000000 00:00 0
7f1a356ba000-7f1a356e0000 r-xp 00000000 fd:01 133584                     /lib/x86_64-linux-gnu/ld-2.23.so
7f1a358d7000-7f1a358da000 rw-p 00000000 00:00 0
7f1a358de000-7f1a358df000 rw-p 00000000 00:00 0
7f1a358df000-7f1a358e0000 r--p 00025000 fd:01 133584                     /lib/x86_64-linux-gnu/ld-2.23.so
7f1a358e0000-7f1a358e1000 rw-p 00026000 fd:01 133584                     /lib/x86_64-linux-gnu/ld-2.23.so
7f1a358e1000-7f1a358e2000 rw-p 00000000 00:00 0
7ffdb56b9000-7ffdb56da000 rw-p 00000000 00:00 0                          [stack]
7ffdb5784000-7ffdb5788000 r--p 00000000 00:00 0                          [vvar]
7ffdb5788000-7ffdb578a000 r-xp 00000000 00:00 0                          [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
Aborted (core dumped)

Output from flavorjones/nokogiri-test:alpine docker image.

# gcc ./fmt.c -o fmt

# ./fmt nokogiri_with_o
nokogiri_with_o
Segmentation fault (core dumped)

# gcc ./fmt.c -o fmt -O2

# ./fmt nokogiri_with_o
_FORTIFY_SOURCE defined
Illegal instruction (core dumped)

ilyazub · 2020-10-18T19:41:38Z

Is it okay to turn off _FORTIFY_SOURCE for the compilation of all native extensions in terms of security? Only libxml2 crashes with -D_FORTIFY_SOURCE=2. libiconv is compiled with -O2 -g in master.

flavorjones · 2020-10-19T13:25:47Z

@ilyazub I'm not sure what you're asking when you say "all native extensions." In the particular case of libxml2 and #2022 I think it's safe:

because we've been shipping precompiled libxml2 for Windows for a decade -- with _FORTIFY_SOURCE set to 0 -- and have not had any security reports
because there are other safety mitigations in place -- we run the Nokogiri test suite through Valgrind; libxml2 runs tests through ASan; and libxml2 regularly tests with fuzzed input.

If you have reason to think this is unsafe, please let me know.

flavorjones · 2020-10-19T13:27:19Z

Oh, if your question was about using _FORTIFY_SOURCE=0 for all the libraries, you're correct that this PR is adding the CFLAGS in the wrong place; but it's difficult to add them in the right place because of the way process_recipe is designed. I plan to fix this before merging.

ilyazub · 2020-10-19T13:32:35Z

Yes, I mean CFLAGS can be added to configure_options of libxml2_recipe instead of all recipes.

diff --git a/ext/nokogiri/extconf.rb b/ext/nokogiri/extconf.rb
index 89c6ec56..030cfb92 100644
--- a/ext/nokogiri/extconf.rb
+++ b/ext/nokogiri/extconf.rb
@@ -582,7 +582,8 @@ EOM
       "--with-c14n",
       "--with-debug",
       "--with-threads",
-      *(darwin? ? ["RANLIB=/usr/bin/ranlib", "AR=/usr/bin/ar"] : "")
+      *(darwin? ? ["RANLIB=/usr/bin/ranlib", "AR=/usr/bin/ar"] : ""),
+      "CFLAGS=-O2 -U_FORTIFY_SOURCE -g"
     ]
   end

ilyazub · 2020-10-19T13:40:39Z

@flavorjones Thanks for convincing me that it's safe. I'm asking about the safety of this change because I have almost no experience in C.

AppVeyorBot · 2020-10-19T14:06:50Z

✅ Build nokogiri 1.0.714 completed (commit 0dd73c4e8c by @flavorjones)

This is the default for those libraries, but we were overriding CFLAGS in some cases. Additionally, we need to compile with -U_FORTIFY_SOURCE to avoid Ubuntu's convention of setting -D_FORTIFY_SOURCE=2 when -O2 is set. This leads to problems when running precompiled libraries on musl (see #2100 for details). Closes #2022.

flavorjones · 2020-10-26T18:44:25Z

@ilyazub I heavily refactored extconf.rb, to address the process_recipe problems, and made this change at the library level in 0b49210. I've credited you in the CHANGELOG! Thank you!

ilyazub · 2020-10-27T13:15:48Z

@flavorjones Thank you!

Compile native extensions with '-O2 -g' compiler flags

3b7ae18

flavorjones added this to the v1.11.0 milestone Oct 16, 2020

flavorjones added the topic/performance label Oct 16, 2020

turn off _FORTIFY_SOURCE checking with -O2 compilation

abd5669

because this makes the precompiled (glibc) library segfault on musl. Related to a3d9438 and sparklemotion#2022

ilyazub closed this Oct 19, 2020

ilyazub reopened this Oct 19, 2020

flavorjones closed this Oct 26, 2020

ilyazub deleted the build-native-extensions-with-compiler-optimizations branch October 27, 2020 13:15

flavorjones added the packaging/native-gem label Dec 1, 2020

flavorjones mentioned this pull request May 19, 2024

feat: first-class musl support #3111

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compile native extensions with '-O2 -g' flags #2100

Compile native extensions with '-O2 -g' flags #2100

ilyazub commented Oct 16, 2020

codeclimate bot commented Oct 16, 2020 •

edited

Loading

AppVeyorBot commented Oct 16, 2020

flavorjones commented Oct 16, 2020

flavorjones commented Oct 16, 2020 •

edited

Loading

flavorjones commented Oct 16, 2020

flavorjones commented Oct 16, 2020 •

edited

Loading

AppVeyorBot commented Oct 16, 2020

ilyazub commented Oct 18, 2020 •

edited

Loading

flavorjones commented Oct 18, 2020

ilyazub commented Oct 18, 2020 •

edited

Loading

ilyazub commented Oct 18, 2020

flavorjones commented Oct 19, 2020

flavorjones commented Oct 19, 2020

ilyazub commented Oct 19, 2020 •

edited

Loading

ilyazub commented Oct 19, 2020

AppVeyorBot commented Oct 19, 2020

flavorjones commented Oct 26, 2020

ilyazub commented Oct 27, 2020

Compile native extensions with '-O2 -g' flags #2100

Compile native extensions with '-O2 -g' flags #2100

Conversation

ilyazub commented Oct 16, 2020

codeclimate bot commented Oct 16, 2020 • edited Loading

AppVeyorBot commented Oct 16, 2020

flavorjones commented Oct 16, 2020

flavorjones commented Oct 16, 2020 • edited Loading

flavorjones commented Oct 16, 2020

flavorjones commented Oct 16, 2020 • edited Loading

AppVeyorBot commented Oct 16, 2020

ilyazub commented Oct 18, 2020 • edited Loading

flavorjones commented Oct 18, 2020

ilyazub commented Oct 18, 2020 • edited Loading

ilyazub commented Oct 18, 2020

flavorjones commented Oct 19, 2020

flavorjones commented Oct 19, 2020

ilyazub commented Oct 19, 2020 • edited Loading

ilyazub commented Oct 19, 2020

AppVeyorBot commented Oct 19, 2020

flavorjones commented Oct 26, 2020

ilyazub commented Oct 27, 2020

codeclimate bot commented Oct 16, 2020 •

edited

Loading

flavorjones commented Oct 16, 2020 •

edited

Loading

flavorjones commented Oct 16, 2020 •

edited

Loading

ilyazub commented Oct 18, 2020 •

edited

Loading

ilyazub commented Oct 18, 2020 •

edited

Loading

ilyazub commented Oct 19, 2020 •

edited

Loading