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

PetarKirov · 2017-12-12T08:41:15Z

Alternative to #7420.

To reproduce the original problem, simply run:

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

To verify the changes in this PR, run:

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

... on Linux x86_64 distros where PIC/PIE is enforced. For more details, see: * https://wiki.gentoo.org/wiki/Hardened/Position_Independent_Code_internals * https://fedoraproject.org/wiki/Packaging:Guidelines#PIE * https://wiki.debian.org/Hardening/PIEByDefaultTransition * https://wiki.ubuntu.com/SecurityTeam/PIE * non-PIE linker support removed in Android 5.0: https://source.android.com/security/enhancements/enhancements50 * https://eli.thegreenplace.net/2011/11/03/position-independent-code-pic-in-shared-libraries/ We addressed the issues in the `dmd.conf` generated when dmd is build, the `dmd.conf` files we ship in the release packages, and the makefiles of druntime and phobos, but we haven't (yet) fixed the test suite build system. Because of that, on Linux x86_64 distros where PIC is enforced, the dmd test suite (the d_do_test.d test runner, as well as all test cases that require linking) fails to link and as a result is completely unusable. The first commit of this pull-request does the following: * adds a `PIC` variable to `test/Makefile`, which defaults to `1`, on x86_64 * exports a `PIC_FLAG` environment variable, which is set to `-fPIC` when `PIC` is set to `1` * adds `$(PIC_FLAG)` to `d_do_test.d`'s build command-line * appends `$(PIC_FLAG)` to the `REQUIRED_ARGS` variable, which is expanded in each `*.d` test case command-line The second and final commit adds the `$PIC_FLAG` environment variable to the dmd command-line in the shell-driven test cases, where the object files were linked. This was done by trial and error, by amending each failing test case, until I got the whole test suite to pass on my Ubuntu 17.10 system, minus two unrelated test failures: * `runnable/test_cdvecfill.d` - see https://issues.dlang.org/show_bug.cgi?id=18013 * runnable/test17559.d - stack traces don't work properly on my system, but I have yet to investigate the root cause. See also: * dlang#7002 * dlang/druntime#1880 * dlang/druntime#1974 * dlang/phobos#5586 * dlang/phobos#5868

... on Linux x86_64 distros where PIC/PIE is enforced. See the previous commit for more details. This commit adds the `$PIC_FLAG` environment variable to the dmd command-line in the shell-driven test cases, where the object files were linked. This was done by trial and error, by amending each failing test case, until I got the whole test suite to pass (minus to unrelated test failures) on my Ubuntu 17.10 system.

Update test_cdvecfill test suite for -fPIC

dlang-bot · 2017-12-12T08:41:16Z

Thanks for your pull request, @ZombineDev!

Bugzilla references

Auto-close	Bugzilla	Description
✓	18013	DMD test suite assertion failure in test_cdvecfill.d
✓	18014	DMD test suite fails to link on Linux distros where PIC/PIE is enforced

PetarKirov · 2017-12-12T08:42:01Z

Alternative to #7420

JinShil · 2017-12-12T08:48:12Z

test/runnable/test_cdvecfill.d

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


If PIC=0 won't that cause this test to fail if we accept these changes?

I'm not sure. I just cherry-picked @wilzbach's fix to try to get the test suite to pass on my machine. It seems this will need more work.

(As I also hit 18013, though I hadn't had the time to look into it.)

I think you may need to keep both tests: This one for -fPIC and the test without these changes for Non-PIC. That is, if you intend to allow both PIC and non-PIC compiles, which appears to be the case, unless I'm misunderstanding.

How about copy/pasting this file and using version(D_PIC) {} else in the other? This has the advantage that the update script would still work.

PetarKirov · 2017-12-12T09:01:23Z

I'm not satisfied with my PR yet, as in my docker setup I get the following error:

Running runnable tests
 ... runnable/gdb1.d                -g -fPIC -fPIC ()
Test failed.  The logged output:
../src/dmd -conf= -m64 -Irunnable -g -fPIC -fPIC  -odtest_results/runnable -oftest_results/runnable/gdb1_0 runnable/gdb1.d
gdb test_results/runnable/gdb1_0 --batch -x test_results/runnable/gdb1_0.gdb
Breakpoint 1 at 0x842: file runnable/gdb1.d, line 15.
Could not trace the inferior process.
Error: ptrace: Operation not permittedtest_results/runnable/gdb1_0.gdb:3: Error in sourced command file:
During startup program exited with code 127.


==============================
Test failed: 
GDB regex: 'RESULT=.*ARG1.*ARG2' didn't match output:
----
Breakpoint 1 at 0x842: file runnable/gdb1.d, line 15.
Could not trace the inferior process.
Error: ptrace: Operation not permittedtest_results/runnable/gdb1_0.gdb:3: Error in sourced command file:
During startup program exited with code 127.

----


Makefile:167: recipe for target 'test_results/runnable/gdb1.d.out' failed
make[1]: *** [test_results/runnable/gdb1.d.out] Error 1

Also, on Windows I get the following error:

../src/dmd.exe -conf= -m64 -fPIC -unittest -run d_do_test.d -unittest
Error: unrecognized switch '-fPIC'

I'm running out of time now, so I'll try to investigate this in the evening.

wilzbach · 2017-12-13T05:30:44Z

Alternative to #7420.

Okay, I think we should go step by step here and thus as I first step I cherry-picked your first commit and modified it slightly: #7433

Note that stack traces on Linux are broken in PIE, see e.g. https://issues.dlang.org/show_bug.cgi?id=18068 (a good reason why we should let at least one CI run the test suite with -fPIC)

andralex · 2017-12-26T15:58:14Z

pros and cons of this vs #7420 ?

JinShil · 2018-01-16T01:32:11Z

@ZombineDev ~~I'm leaning more towards #7420.~~ Any particular drawbacks of #7420 that this PR does not suffer from? ~~If not, is it OK to close this in favor of #7420?~~

UPDATE: I just realized that we can always add -fPIC to non-PIE-hardened systems, but I'm not aware of any way to do vise-versa. I see the value in this PR as it identifies, precisely, which tests are PIE sensitive and explicitly adds the -fPIC switch to them. I like that.

JinShil · 2018-01-16T02:38:31Z

I'm wondering, however, if we really need to add ${PIC_FLAG} to these tests. I think we can just explicitly add -fPIC to these tests as non-PIE-hardened systems can still verify the functionality of the test in PIC-mode.

The drawback, however, is there would be no verification of the test in non-PIC mode, but I'm not sure if that's a problem.

PetarKirov · 2018-01-16T07:14:45Z

@JinShil I am only propagating the PIC flag to the shell-based tests, because the rest pick it up from the REQUIRED_ARGS variable. An alternative solution would be propagate the REQUIRED_ARGS for the shell-based tests and not treat PIC specifically.

JinShil · 2018-01-16T07:19:43Z

I am only propagating the PIC flag to the shell-based tests,

Ah, right. Sorry, I'm still learning.

An alternative solution would be propagate the REQUIRED_ARGS for the shell-based tests and not treat PIC specifically.

That actually seems like the best approach. I'm wondering why it's not done already, as the name implies, it should be required. Can you give it a try?

UPDATE: Hold on. I clearly misunderstood this. So it looks like there's a lot of redundancy in the shell script tests. We should try to have one source of truth for compiler args, and not have to add such things to each and every shell script. I'm now leaning back towards #7420.

JinShil · 2018-01-16T08:10:12Z

Ok, so after thinking about this more, I'm thinking #7420 is the way to go. @ZombineDev is there any particular strength this PR has over #7420 that we should consider?

PetarKirov · 2018-01-16T08:56:18Z

@wilzbach substantially improved his PR in the meantime to use a docker container (I had the initial idea, but not the time to realize it), so now I'm very happy to go with #7420

PetarKirov and others added 3 commits December 12, 2017 10:24

Fix Issue 18013 - DMD test suite assertion failure in test_cdvecfill.d

d80fb61

Update test_cdvecfill test suite for -fPIC

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

PetarKirov changed the title ~~Alternative approach to allow to running the DMD testsuite on hardened systems~~ [WIP] Alternative approach to allow to running the DMD testsuite on hardened systems Dec 12, 2017

dlang-bot added the Review:WIP Work In Progress - not ready for review or pulling label Dec 12, 2017

JinShil reviewed Dec 12, 2017

View reviewed changes

wilzbach mentioned this pull request Dec 13, 2017

Fix issue 18014 - dmd test suite fails to link (part 1/2) #7433

Merged

wilzbach mentioned this pull request Dec 17, 2017

Fix Issue 18013 - DMD test suite assertion failure in test_cdvecfill.d #7454

Merged

dlang-bot added the Review:Needs Rebase label Jan 1, 2018

wilzbach mentioned this pull request Jan 2, 2018

Use a special D docker image for CircleCi #7579

Closed

dlang-bot added the Review:Needs Work label Jan 5, 2018

JinShil mentioned this pull request Jan 14, 2018

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

Closed

wilzbach mentioned this pull request Jan 15, 2018

Allow to run the DMD testsuite on hardened systems #7420

Merged

JinShil mentioned this pull request Jan 16, 2018

fixup for #7454 - always run asm tests with PIC #7716

Merged

PetarKirov closed this Jan 16, 2018

Uh oh!

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

dlang-bot commented Dec 12, 2017

Bugzilla references

Uh oh!

PetarKirov commented Dec 12, 2017

Uh oh!

JinShil Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

PetarKirov 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!

JinShil Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

wilzbach Dec 13, 2017

Choose a reason for hiding this comment

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!

wilzbach commented Dec 13, 2017

Uh oh!

andralex commented Dec 26, 2017

Uh oh!

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

Uh oh!

Uh oh!

JinShil commented Jan 16, 2018

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

JinShil commented Jan 16, 2018

Uh oh!

PetarKirov commented Jan 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

PetarKirov commented Dec 12, 2017 •

edited

Loading

PetarKirov Dec 12, 2017 •

edited

Loading

PetarKirov commented Dec 12, 2017 •

edited

Loading

JinShil commented Jan 16, 2018 •

edited

Loading

PetarKirov commented Jan 16, 2018 •

edited

Loading

JinShil commented Jan 16, 2018 •

edited

Loading