Allow to run the DMD testsuite on hardened systems #7420

wilzbach · 2017-12-11T05:05:08Z

Because most Linux OSes now use hardening (e.g. Debian, Ubuntu, Arch Linux) and the DMD test suite should work out of the box.
-fPIC has been the default on Linux x64 since 2.072.2.

Bugzilla references

Auto-close	Bugzilla	Description
✓	18014	DMD test suite fails to link on Linux distros where PIC/PIE is enforced

wilzbach · 2017-12-11T08:13:12Z

Okay so regarding:

 ... runnable/test_cdvecfill.d      -O (-mcpu=avx -mcpu=avx2)
Test failed.  The logged output:
../src/dmd -conf= -m64 -Irunnable -O -fPIC  -odgenerated/runnable -ofgenerated/runnable/test_cdvecfill_0 runnable/test_cdvecfill.d
generated/runnable/test_cdvecfill_0
Expected code sequence for load!(ubyte, 16) not found.
  Expected: 0x50 0x66 0x0f 0x6e 0xc7 0x66 0x0f 0x60 0xc0 0x66 0x0f 0x61 0xc0 0x66 0x0f 0x70 0xc0 0x00 0x59 0xc3
  Actual: 0x55 0x48 0x8b 0xec 0x66 0x0f 0x6e 0xc7 0x66 0x0f 0x60 0xc0 0x66 0x0f 0x61 0xc0 0x66 0x0f 0x70 0xc0 0x00 0x5d 0xc3 0x00 0x55 0x48 0x8b 0xec 0x0f 0xb6 0x07 0x66 0x0f 0x6e 0xc0 0x66 0x0f 0x60 0xc0 0x66 0x0f 0x61 0xc0 0x66 0x0f 0x70 0xc0 0x00 0x5d 0xc3 0x00 0x00 0x55 0x48 0x8b 0xec 0x66 0x0f 0x6e 0xc7 0x66 0x0f 0x60 0xc0
core.exception.AssertError@runnable/test_cdvecfill.d(895): Assertion failure

-> I updated the cdvec_fill.d file (this script is apparently only run on Linux x64)

wilzbach · 2017-12-11T08:15:00Z

test/runnable/test_cdvecfill.d

        foreach (arch; [EnumMembers!Arch])
        {
-            auto args = [dmd, "-c", "-O", "-mcpu=" ~ arch.to!string, "test/runnable/test_cdvecfill.d"];
+            auto args = [dmd, "-c", "-fPIC", "-O", "-mcpu=" ~ arch.to!string, "test/runnable/test_cdvecfill.d"];


This line in the update script isn't necessary itself because the default dmd.conf for Linux x86_64 already adds -fPIC as default flag, but imho explicitness helps.

JinShil · 2017-12-12T05:10:26Z

test/runnable/test_cdvecfill.d

-        /* pop    rcx                     */ 0x59,
+        /* pop    rbp                     */ 0x5d,
        /* ret                            */ 0xc3,
    ]),


Why was this code change necessary?

Apparently when emitting position-independent code, different registers are used. There's an update script included in this file which I used to update the test cases.

Would it be prudent to keep the original test for Non-PIC and simply keep this PR as a simple addition? My motivation being to test both scenarios.

Yes I could use version(D_PIC), but this would make the update script more complex. And I'm not sure it's necessary, because this script is only run under Linux x86_64 atm and since 2.072.2 dmd uses -fPIC for compiled programs on Linux as default (for Darwin -fPIC is even fixed). Anyhow, if I add the old test cases, this would only make sense, if they are run as part of the test suite and then if not in version(PIE) we would have to detect if the environment is hardened and then avoid running the test cases.
And in case this will ever be needed, the original code is still saved in git and alternatively it can be easily regenerated with Martin's fancy update script.
So I would only do this extra work if @MartinNowak or @WalterBright think it's essential.

JinShil · 2017-12-12T05:12:19Z

Looks like this may address this issue: https://issues.dlang.org/show_bug.cgi?id=18013

wilzbach · 2017-12-12T05:40:42Z

Looks like this may address this issue: https://issues.dlang.org/show_bug.cgi?id=18013

Yes it will. I'm a Arch Linux user myself, so I constantly bump into the -fPIC issues with D, but luckily with a rising number of merged PRs the problems have been drastically reduced. This is actually (hopefully) the last bit of a long series (dlang/phobos#5586, dlang/phobos#5750, dlang/druntime#1880, #7002, dlang/tools#264 ...) in my quest to build and test the main D repos on Arch Linux without needing to patch anything ;-)

PetarKirov · 2017-12-12T06:12:53Z

Can you add this to the list of bugs fixed:
https://issues.dlang.org/show_bug.cgi?id=18014

My fix is a bit different, I'll push it soon so we can compare our approaches.

PetarKirov · 2017-12-12T08:20:53Z

I don't think your PR fixes all of the issues from 18014. When running:

docker run -it zombinedev/dmd-test-suite-docker --repos:dmd:pr/7420

I get these errors:

/usr/bin/ld: test_results/compilable/main.o: relocation R_X86_64_32 against `.rodata.str1.1' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: test_results/compilable/a.a(a_1_149.o): relocation R_X86_64_32 against symbol `_D28TypeInfo_S4tmpl__T4TmplTiZQi6__initZ' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: test_results/compilable/b.a(b_1_149.o): relocation R_X86_64_32 against symbol `_D28TypeInfo_S4tmpl__T4TmplTlZQi6__initZ' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: final link failed: Nonrepresentable section on output
collect2: error: ld returned 1 exit status
Error: linker exited with status 1
Makefile:164: recipe for target 'test_results/compilable/test6461.sh.out' failed
make[1]: *** [test_results/compilable/test6461.sh.out] Error 1
make[1]: *** Waiting for unfinished jobs....
 ... fail_compilation/diag6707.d    ()
 ... fail_compilation/fail14416.d   ()
 ... fail_compilation/ice8309.d     ()
 ... fail_compilation/ice10076.d    ()
 ... fail_compilation/fail233.d     -o- ()
/usr/bin/ld: test_results/compilable/test14894main.o: relocation R_X86_64_32 against symbol `_Dmain' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: final link failed: Nonrepresentable section on output
collect2: error: ld returned 1 exit status
Error: linker exited with status 1
Makefile:164: recipe for target 'test_results/compilable/test14894.sh.out' failed
make[1]: *** [test_results/compilable/test14894.sh.out] Error 1

Though it gets much further than:

docker run -it zombinedev/dmd-test-suite-docker --repos:dmd:master

Which can't even build the test runner (d_do_test.d).

PetarKirov · 2017-12-12T08:35:19Z

I'll see if with your fix for 18013 my branch finally passes the test suite, and if that's the case I'll open an alternative PR.

PetarKirov · 2017-12-12T08:36:45Z

https://github.com/ZombineDev/dmd/commits/fpic-for-testsuite

wilzbach · 2018-01-16T19:53:05Z

79.152% (-1.841%) compared to cd6b40f

Seems like we have to go with three containers :/

wilzbach · 2018-01-17T04:40:24Z

Looks like we are good to go here :)
The Docker image, see e.g. #7579 (comment) for an example of the relocation errors you get.

The docker image is based on this: https://github.com/wilzbach/dlang-docker/blob/master/circleci/dlang.docker

Why 3 jobs and not 2?

Code coverage:

79.152% (-1.841%) compared to cd6b40f

Why is there now `circleci: pic` and `circleci: no_pic` appearing?

It's a feature from CodeCov to send different status notifications for different stages.

https://circleci.com/docs/2.0/workflows/

Having the ci/circleci checkbox enabled will prevent the status from showing as completed in GitHub when using a workflow because CircleCI posts statuses to Github with a key that includes the job by name.

wilzbach · 2018-01-17T04:45:24Z

Ah I forgot to mention - the pic job is even faster (probably due to all dependencies being part of the Docker image):

https://circleci.com/workflow-run/cd281b04-466e-4db5-b44d-c6281a3b31db

wilzbach · 2018-01-17T04:47:53Z

.circleci/config.yml

+  no_pic:
    working_directory: ~/dmd
    docker:
      - image: circleci/node:4.8.2


We could replace this with a D Docker image that doesn't require -fPIC, e.g. https://github.com/wilzbach/dlang-docker/blob/master/circleci/dlang.docker built for Ubuntu 14.04, but this should be more than fine for now (it's the existing status quo after all).

wilzbach · 2018-01-17T04:48:21Z

.circleci/config.yml

    parallelism: 2
-    branches:
-      ignore:
-        - dmd-1.x


This didn't work with the new workflow setting and AFAICT is only required for the respective branch anyways.

wilzbach · 2018-01-17T04:48:54Z

.circleci/config.yml

-          command: ./.circleci/run.sh coverage
-          name: Run testsuite with -cov
+          command: ./.circleci/run.sh all
+          name: Run all


I reduced the commands to "all", s.t. it's easier to maintain both jobs.

wilzbach · 2018-01-17T04:51:42Z

test/Makefile

    endif
    export DFLAGS
 endif
+REQUIRED_ARGS+=$(PIC_FLAG)


After the other PRs that's all that's needed to fix the test suite for -fPIC :)

wilzbach · 2018-01-17T04:52:31Z

.circleci/run.sh

+    check-clean-git) echo "removed" ;;
+    codecov)
+        echo "removed - use 'all'"
+        ;&


fall-through is required, s.t. existing PRs don't need to be rebased, but will call all automatically.

Probably a good idea to add that as a comment to the script.

JinShil · 2018-01-17T06:04:51Z

Does the test suite pass? Even test/runnable/test17559.d?

wilzbach · 2018-01-17T11:11:26Z

Does the test suite pass? Even test/runnable/test17559.d?

Yes:

JinShil · 2018-01-17T11:57:55Z

This is looking great. Can you give a final summary? What's remaining before we can merge this?

wilzbach · 2018-01-17T12:17:06Z

Can you give a final summary?

I try to do an honest summary of the disadvantages I see as you know the advantages of testing automatically for -fPIC yourself.

One more container run at CircleCi

Con:

This shouldn't be a huge problem. CircleCi is still one of our fastest CIs and we have four containers (=jobs) per organization available.
If this really becomes a bottleneck, we can always easily disable the pic stage

Dependency on the D docker image

Con:

it's configured with Travis CRON + auto-deploy for new releases, s.t. not much maintenance should be required
it uses the official CircleCi Dockerfile and just adds the D installer + a few packages on top
it's just 29 lines

One more status update by CircleCi (no_pic and pic) = more visual noise

I haven't found a good solution for this :/

The status check ci/circleci was still set to "enforced". As the newly deployed auto-merge should obey combined CI status dlang-bot#69 feature lets the bot respect the failing CIs properly, I set "ci/circleci" to non-enforced until we reach a decision here.

What's remaining before we can merge this?

This is ready from my sides. CIs are happy -> I'm happy

JinShil

Nice work! 👍

wilzbach · 2018-01-19T02:30:27Z

The only thing that is still unclear to me is why the stack traces work on the Docker image.
It's failing on my local system and clearly I'm not the only one:

http://forum.dlang.org/post/ufpwobqmisamazigcaav@forum.dlang.org
https://issues.dlang.org/show_bug.cgi?id=18068

Maybe it's not as assumed due to PIE, but sth. else. glibc maybe? Arch Linux currently ships 2.26-10

JinShil · 2018-01-19T02:40:30Z

It's failing on my local system and clearly I'm not the only one:

Same here. That's why I asked.

Maybe it's not as assumed due to PIE, but sth. else. glibc maybe?

Evidence certainly points to something else. I use Arch Linux too. I don't think it should hold up this PR.

wilzbach · 2018-01-24T10:48:05Z

Evidence certainly points to something else. I use Arch Linux too. I don't think it should hold up this PR.

I agree, so let's move forward here?

JinShil · 2018-01-24T11:01:09Z

I agree, so let's move forward here?

Yep, just wanted to give others a few days opportunity to chime it. I think they've had enough time. auto-merge toggling on...

WalterBright added the Merge:auto-merge label Dec 11, 2017

dlang-bot removed the Merge:auto-merge label Dec 11, 2017

wilzbach commented Dec 11, 2017

View reviewed changes

wilzbach force-pushed the fpic-dmd branch 2 times, most recently from bf89166 to 26fea93 Compare December 11, 2017 08:50

JinShil reviewed Dec 12, 2017

View reviewed changes

wilzbach force-pushed the fpic-dmd branch from 26fea93 to 3f16ee7 Compare December 12, 2017 05:34

JinShil previously approved these changes Dec 12, 2017

View reviewed changes

wilzbach force-pushed the fpic-dmd branch from 3f16ee7 to 0324061 Compare December 12, 2017 06:17

dlang-bot added the Severity:Bug Fix label Dec 12, 2017

PetarKirov mentioned this pull request Dec 12, 2017

[WIP] Alternative approach to allow to running the DMD testsuite on hardened systems #7427

Closed

wilzbach mentioned this pull request Dec 18, 2017

Add DragonFlyBSD support for dmd #7463

Merged

wilzbach force-pushed the fpic-dmd branch from 0324061 to a41eddb Compare December 21, 2017 16:06

dlang-bot added Review:Needs Rebase Review:Needs Work and removed Review:Needs Rebase labels Jan 1, 2018

wilzbach mentioned this pull request Jan 2, 2018

Use a special D docker image for CircleCi #7579

Closed

dlang-bot removed the Review:Needs Rebase label Jan 5, 2018

JinShil mentioned this pull request Jan 14, 2018

Issue 14474 - Internally use UTF-8 strings for Windows #4602

Closed

wilzbach force-pushed the fpic-dmd branch from a41eddb to efffe16 Compare January 15, 2018 10:05

dlang-bot removed the Review:Needs Work label Jan 15, 2018

wilzbach force-pushed the fpic-dmd branch 2 times, most recently from ac30592 to 8ece1fa Compare January 17, 2018 02:53

wilzbach force-pushed the fpic-dmd branch from 8ece1fa to cf0d8da Compare January 17, 2018 04:41

wilzbach force-pushed the fpic-dmd branch from cf0d8da to e713254 Compare January 17, 2018 04:51

wilzbach commented Jan 17, 2018

View reviewed changes

wilzbach force-pushed the fpic-dmd branch from e713254 to d2cd565 Compare January 17, 2018 05:00

wilzbach force-pushed the fpic-dmd branch from d2cd565 to e548934 Compare January 17, 2018 11:59

JinShil approved these changes Jan 17, 2018

View reviewed changes

wilzbach added 2 commits January 19, 2018 03:27

Fix Issue 18014 - Allow to run the DMD testsuite on hardened systems

b55eff6

Use a special D docker image for CircleCi

c836db0

wilzbach force-pushed the fpic-dmd branch from e548934 to c836db0 Compare January 19, 2018 02:27

wilzbach mentioned this pull request Jan 24, 2018

Use multiple different test compilers when invoking rdmd_test from Travis CI dlang/tools#297

Merged

JinShil added the Merge:auto-merge label Jan 24, 2018

JinShil approved these changes Jan 24, 2018

View reviewed changes

JinShil merged commit 252ab44 into dlang:master Jan 24, 2018

wilzbach deleted the fpic-dmd branch January 24, 2018 11:56

This was referenced Jan 24, 2018

Handle head sha changes correctly dlang/dlang-bot#77

Closed

Allow using updated CircleCi without rebasing PRs #7770

Merged

Experiment with using only two CircleCi images #7818

Merged

Uh oh!

Allow to run the DMD testsuite on hardened systems #7420

Allow to run the DMD testsuite on hardened systems #7420

Uh oh!

Conversation

wilzbach commented Dec 11, 2017

Uh oh!

dlang-bot commented Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Uh oh!

wilzbach commented Dec 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JinShil Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JinShil commented Dec 12, 2017

Uh oh!

wilzbach commented Dec 12, 2017

Uh oh!

PetarKirov commented Dec 12, 2017

Uh oh!

PetarKirov commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PetarKirov commented Dec 12, 2017

Uh oh!

PetarKirov commented Dec 12, 2017

Uh oh!

wilzbach commented Jan 16, 2018

Uh oh!

wilzbach commented Jan 17, 2018

Why 3 jobs and not 2?

Why is there now circleci: pic and circleci: no_pic appearing?

Uh oh!

wilzbach commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JinShil commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Jan 17, 2018

Uh oh!

JinShil commented Jan 17, 2018

Uh oh!

wilzbach commented Jan 17, 2018

Uh oh!

JinShil left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jan 19, 2018

Uh oh!

JinShil commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Jan 24, 2018

Uh oh!

JinShil commented Jan 24, 2018

dlang-bot commented Dec 11, 2017 •

edited

Loading

JinShil Dec 12, 2017 •

edited

Loading

PetarKirov commented Dec 12, 2017 •

edited

Loading

Why is there now `circleci: pic` and `circleci: no_pic` appearing?

JinShil commented Jan 17, 2018 •

edited

Loading

JinShil commented Jan 19, 2018 •

edited

Loading