Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support acting as startup code of a dynamic linker #108

Merged
merged 5 commits into from
Feb 27, 2024

Conversation

bjorn3
Copy link
Contributor

@bjorn3 bjorn3 commented Feb 18, 2024

Previously this would result in the relocation code using the wrong offset as AT_PHDR points to the program headers of the main executable rather than of the dynamic linker itself.

Copy link
Owner

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

I've not finished reviewing the relocation logic, but here's an initial few comments:

src/arch/x86_64.rs Outdated Show resolved Hide resolved
// We don't need to relocate ourself as the dynamic linker has already
// done this. `AT_PHDR` points to our own program headers and `AT_ENTRY`
// to our own entry point. `AT_BASE` contains the relocation offset of
// the dynamic linker.
Copy link
Owner

Choose a reason for hiding this comment

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

As far as I can tell, this case 3 doesn't need the AT_BASE value; is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's presence is used to check that we are case 3. The actual value is not used by case 3.

src/relocate.rs Outdated Show resolved Hide resolved
@@ -213,6 +227,10 @@ pub(super) unsafe fn relocate(envp: *mut *mut u8) {
}
}

// FIXME split function into two here with a hint::black_box around the
// function pointer to prevent the compiler from moving code between the
// functions.
Copy link
Owner

Choose a reason for hiding this comment

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

I've thought about something like this too, however I think it's actually better if we don't have to do this. If it's dangerous for this code at the end of relocate to mix with our relocation code, that suggests our relocation code itself is dangerous.

So instead, we use inline asm to do the actual relocation loads and stores, which hides the crucial part which Rust can't think about from Rust, and it should also have the effect of obliging the optimizer to preserve the ordering of the relocations with other code.

Copy link
Contributor Author

@bjorn3 bjorn3 Feb 19, 2024

Choose a reason for hiding this comment

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

The inline asm doesn't prevent moving a (speculative) load of a static that needs relocations to before the inline asm as the compiler assumes that relocations have already happened and loading a static doesn't have any side effects. Musl does this kind of fence between every relocation step (after R_RELATIVE relocation resolving (_dlstart_c -> __dls2), after setting the thread pointer (__dls2b -> __dls3) and finally after symbol relocations resolving and right before calling the entry point of the main program (__dls3 -> AT_ENTRY).

Copy link
Owner

Choose a reason for hiding this comment

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

I guess on a practical level, you're right that a black_box would avoid some obscure possibilities, though not all. I'd accept a PR for it.

On a theoretical level though, Rust doesn't make the kinds of guarantees that this code ultimately needs, and black_box, function pointers, volatile, asm! barriers, etc. aren't enough. We'd need guarantees like:

  • "The compiler shall not codegen certain language constructs into calls
    to other crates, such as translating x == null() to x.is_null().
  • "The compiler shall not codegen certain language constructs using uses
    of static data."

Rust doesn't make these kinds of guarantees. C doesn't either, even with optimization barriers. My best idea right now for making this code theoretically sound is to precompile relocate, hand-verify it, and embed it with include_bytes in the _start code so that it runs before any Rust code runs.

@bjorn3 bjorn3 force-pushed the fix_relocation branch 2 times, most recently from 80e9031 to a838e3f Compare February 19, 2024 11:26
@bjorn3
Copy link
Contributor Author

bjorn3 commented Feb 19, 2024

I can't get LLVM to produce a R_386_PC32 _DYNAMIC relocation for i686. 1: add dword ptr [esp], _DYNAMIC-1b is accepted by GCC, but not by LLVM. Clang complains with "cannot use more than one symbol in memory operand" and rustc produces a useless error message.

@bjorn3 bjorn3 force-pushed the fix_relocation branch 2 times, most recently from 7fc9df3 to 3740bbf Compare February 19, 2024 13:58
@bjorn3
Copy link
Contributor Author

bjorn3 commented Feb 19, 2024

I've disabled it on x86 for now. It also seems that the relocation code is never needed on x86 anyway for any of the other tests. Either there is a dynamic linker to do relocations or the executable is a statically linked executable. It is never a static PIE binary.

@bjorn3 bjorn3 force-pushed the fix_relocation branch 5 times, most recently from 86d014b to 1dc91fb Compare February 19, 2024 15:57
@bjorn3
Copy link
Contributor Author

bjorn3 commented Feb 19, 2024

CI should pass now.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Feb 21, 2024

Rebased

Copy link
Owner

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

Overall this looks good; just one more logic simplification that I think we can do:

src/relocate.rs Outdated Show resolved Hide resolved
src/relocate.rs Outdated Show resolved Hide resolved
@bjorn3
Copy link
Contributor Author

bjorn3 commented Feb 27, 2024

Applied the simplification.

@sunfishcode sunfishcode merged commit fa055e9 into sunfishcode:main Feb 27, 2024
5 checks passed
@sunfishcode
Copy link
Owner

Thanks!

@bjorn3 bjorn3 deleted the fix_relocation branch February 27, 2024 14:29
@sunfishcode
Copy link
Owner

This is now released in origin 0.18.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants