Skip to content

Conversation

@wilzbach
Copy link
Contributor

... on Linux x86_64 distros where PIC/PIE is enforced.

(cherry-picked from #7427 by @ZombineDev)


This is the non-controversial part and common part of #7240 and #7427.
@ZombineDev I did two minor modifications:

  • I removed REQUIRED_ARGS for now as the test suite is still failing. Obviously we should let one CI run the test suite afterwards though
  • I used DEBUG_ARGS=-g -fPIC instead of directly adding $(PIC_FLAG) to the d_do_test compile invocations as this looks more general purpose.

Original commit message:

For more details, see:

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.

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

See also:

@wilzbach wilzbach added the Review:Trivial typos, formatting, comments label Dec 13, 2017
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

Bugzilla references

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

@echo "OS: '$(OS)'"
@echo "MODEL: '$(MODEL)'"
@echo "PIC: '$(PIC_FLAG)'"
$(DMD) -conf= $(MODEL_FLAG) $(DEBUG_FLAGS) -unittest -run d_do_test.d -unittest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You haven't added PIC_FLAG here. Without this you still can't build the test suite.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above: DEBUG_FLAGS=$(PIC_FLAG) -g

@PetarKirov
Copy link
Member

@wilzbach I would prefer if we focus on one PR and make changes there. (You can force-push to my branch on #7427 if you need).

@PetarKirov
Copy link
Member

Though if you're in a hurry I'm ok with doing things step-by-step as I don't have time at the moment.

@wilzbach
Copy link
Contributor Author

I would prefer if we focus on one PR and make changes there.
Though if you're in a hurry I'm ok with doing things step-by-step as I don't have time at the moment.

Well, I cherry-picked your commit to this PR because it's ready now & already allows to run the test suite.
Fixing the stacktracing problem will very likely take much longer.

@wilzbach
Copy link
Contributor Author

./generated/windows/release/32/dmd.exe -conf= -m64 -fPIC -g -unittest -run d_do_test.d -unittest
Error: unrecognized switch '-fPIC'

Seems like we should either make the switch no-op on Windows or change the PIC code to be used on Posix only. Though I am interested why it passed before and on the other Windows machine ...

... 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
@PetarKirov
Copy link
Member

I'm for making it a no-op.

@JinShil
Copy link
Contributor

JinShil commented Dec 15, 2017

... change the PIC code to be used on Posix only

I assume you're referring to the test_cdvecfill.d code. We have two versions of this code now: one for PIC and one for non-PIC. Does the non-PIC one run on Windows? I'm assuming it does because it passed before, right?

So, I suggest the following:

  • If windows does not support PIC builds, then emit an error if the $PIC variable is 1. You could also make the $PIC variable a no-op in the makefile for Windows instead of modifying the compiler to make -fPIC a no-op on Windows.
  • Add version(D_PIC) to test_cdvecfill.d to support both builds

Am I not understanding something?

@wilzbach
Copy link
Contributor Author

I'm for making it a no-op.

Oh, I already moved the regarding bits PIC_FLAG definition into the Posix-only of the Makefile as this seemed to be the less controversial option, but I'm also for making it a no-op (but in another PR?)

I assume you're referring to the test_cdvecfill.d code. We have two versions of this code now: one for PIC and one for non-PIC. Does the non-PIC one run on Windows?

Oh I was talking about the fact that we can't pass -fPIC by default on x86_64 as then d_do_test fails to compile on Windows due to the unrecognized switch.

@dlang-bot dlang-bot merged commit 6442402 into dlang:master Dec 15, 2017
@PetarKirov
Copy link
Member

This PR was merged too quickly. This half of the commit message is wrong:

* 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.

@wilzbach wilzbach deleted the hardening-part-1 branch December 19, 2017 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants