-
Notifications
You must be signed in to change notification settings - Fork 26
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 for custom permissions for custom sections. #105
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually looks good; i think adding it to mach will be even easier. Can you post your binary dump here or in the tracking issue; would be interested in testing to see if it works :)
src/artifact/decl.rs
Outdated
match self { | ||
DefinedDecl::Section(a) => a.is_executable(), | ||
DefinedDecl::Function(_) => true, | ||
DefinedDecl::Data(_) => false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I thought the PR would allow code like:
obj.declare("foo", Decl::data().writeable().executable())?;
But this forces data to always not be executable and functions to always be executable; we might as well default to sane values here, but ultimately let user choose to set these differently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g., I think some JIT engines will want WX flags on data section? Or if i want to write self modifying code, etc., and have this in the data segments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make it a property of the Decl/DefinedDecl instead, I was just trying to make minimal changes. This may require either adding flags to every varient of Decl or converting DefinedDecl into a struct that contains an enum + flags or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah like something like:
/// A declaration that is defined inside this artifact
pub enum DefinedDeclKind {
/// A function defined in this artifact
Function(FunctionDecl),
/// A data object defined in this artifact
Data(DataDecl),
/// A section defined in this artifact
Section(SectionDecl),
}
pub struct DefinedDecl {
kind: DefinedDeclKind,
writable: bool,
executable: bool,
}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's probably maybe a good idea (if those fields are truly shared amongst each of the Decl variants), but i suspect the diff might be quite large if DefinedDecl became a struct with a kind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the number of differences will be interesting, yes, but I'll put in the legwork if it's a good idea. EDIT: i.e. my first implementation is an attempt to do minor changes, but with feedback from people who know what they're doing i'll follow their lead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it does worry me because:
- it's a breaking change
- I don't know who's using DefinedDecl, i think its effectively internal, but it would have been nice if it was marked
pub(crate)
- have a default value doesn't translate as well, because the default for a function decl is different than a data, etc.
At this point i think your PR is fine and we should just move forward with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still possible to just add these flags to every varient, i.e. FunctionDecl, DataDecl, and SectionDecl, just like datatype_methods!()
, and then the methods on DefinedDecl just delegate to the impl for the correct enum.
This wouldn't be a breaking change, right?
edit: might want to break it up into a macro invocation for alloc, write, and exec seperately so that we can distinguish which decls need which flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that wouldn't be a breaking change; i was just referring to changing the DefinedDecl to a struct; I think what you have now is basically done, modulo some of the comments I made, unless I'm missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so yes, i think this is (mostly?) done, unless you want me to add is_writable to FunctionDecl and/or is_executable to DataDecl.
we need to make sure not to forget making it work on mach, too.
so, RE binary dump stuff: here's roughly code I used along with the contents of this PR: https://gist.github.com/kitlith/a26a1021c268b34bef60d57cfc6cc19f#file-wasm_lib-rs-L32-L46 -- if you want to follow along I can make some effort to actually get this up in a repo. Here's the raw binary and linked binary in a zip together: NewOrleans.zip This is for the msp430 arch, so if you want to run objdump or smth you may need to grab a toolchain, i'll just run objdump -x on both of these real quick for you: raw_dump:
linked:
This is a dump of New Orleans (which is the level immediately after the tutorial) from Microcorruption, sans symbols because the interface I was trying to use doesn't seem to be implemented yet. |
I just use
|
So, something I just kinda realized is that afaict this PR basically makes Might be something to keep in mind for future breaking changes? |
sure! if you could break add a breaking change, how would you design it? |
Breaking change ideas: DefinedDecl kinda goes away, as it's been subsumed by SectionDecl, as does FunctionDecl and DataDecl (edit: or we could rename SecitonDecl to DefinedDecl). The impls on Decl roughly become types of defaults for SectionDecl, for better backwards compat:
around this time i'd like to remind you i'm talking out of my ass, and my mind is going in this direction because of how they seem similar, but restricted. i.e. i don't get why the original design was the way it was. there are probably good reasons to keep these seperate. |
@kitlith For Mach-O all functions are placed in the same |
How can you specify a RWX section? EDIT: not quite so simple... Mach-o files have the concept of segments, which, among other things, specify the maximum and initial memory protections of all of their sections. However, it has been defined that "an intermediate object file only contains one segment, ... containing all the sections destigned for different segments in the file." So with that said, I guess I don't know how generating a rwx section/segment works for a intermediate object, only an executable. :/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is looking pretty good to go; probably should add a small test verifying functions And or data can be read write ?
Okay, so I should add write flags to FunctionDecl is what I'm hearing? (and then write tests to prove that it's possible?) Or am I misunderstanding? And then there's the mach backend which currently silently ignores these new flags afaik. Are we ignoring that for now? |
I thought you added write flags to function decl? Or just executable to data and sections ? Anyway yea test would be good. And ideally add it to mach if you’re feeling adventurous or probably easier to do in a separate PR (if you want) |
We discussed it, but I never got a definitive answer so I held off on implementing. After all, adding stuff is a minor release, but removing stuff is a breaking change. For clarification:
I'll work on these changes tomorrow. As for mach-o, this library seems to try and abstract away from the underlying format as much as it can. so, I'm focusing more on mach-o so much because if there's no way to do it for mach-o, this could be a leaky/bad abstraction. As such, I feel uncomfortable suggesting this be merged until either finding an API that works for both, or (possibly more likely at this rate) figuring out that it's not possible in mach-o outside of linked executables, and thus (currently) out of scope for fixing in the library, so we should just more forward with something. |
Ok this sounds good to me, as long as careful about breaking changes (I don’t see any so I think we’re good). Yes it’s a good idea to work on think about Mach too, thank you! |
I found a way to make this work for mach-o: LC_LINKER_OPTION allows us to embed linker options inside our binary. combine this with the segprot linker option ( tbh this feels very meh, but I'll take it. I feel more comfortable about the API proposed by this PR now. |
Mach has section flags, I would be surprised if they didn’t have read/write at least? The null segment is mapped this way, making it not readable or write able, so null pointers fault. |
Eg are these not sufficient ? Maybe that is only for use by dynamic linker though ... needs more investigating |
Those are, afaik, segment flags, not section flags. And since there is only one segment in intermediate object files, there's no way to specify the permissions for custom segments. This is the reason why I've been concerned about mach-o the whole time. EDIT: yeah, rereading format docs says that the flags on sections hold the section type and section attributes, and none of those (officially? as far as we know?) specify protection flags. What does specify protection flags is maxprot and initprot on the segment commands. |
Can we just emit another segment with different flags? E.g., right now we just emit |
I doubt the linker will be happy with another segment. Does the macos linker even use the segment flags in the object file? I expected it to ignore them. Should be easy to test for someone with a macos system. |
Looking at the lld source, it probably treats multiple segments the same as if it was only one segment, and the only segment field that I can see it uses is |
JIT compilers works by |
technically, emitting a "__TEXT" segment instead of "" is already off spec (unless i've completely missed something and this doesn't generate intermediate object files, but executables) -- and while I haven't looked at linker code myself, i'd be inclined to believe philip's guess. and, of course, there is still a way to define a segment w/ rwx or whatever perms you want. linker flags. if you need it, you know how to add flags to LDFLAGS. or, since you can embed flags in object files with LC_LINKER_OPTION, have the object file specify the section. This is probably best done with some sort of switch to specify whether a linker options should be embedded, as well as if a section should be placed in a custom segment. Blast if I know how to make a good API for that though, since we're sharing an API with elf files. It's probably easiest to define our own """standard""" segments with custom permissions ("__FAERIE_RWX" anybody?) that we can sort stuff into if they match the permissions, and include the flags for the segments being used. Somewhere in the middle would be generating segment names based on section/symbol names and that seems worse than both the other options. @m4b at least you get why I was so reluctant about the api and kept bringing up mach-o now, right? I don't want to add features to your cross-format API that only work for one format. |
Ah yea that makes sense! I find it hard to believe you can’t write self modifying code on mach though, but maybe.
Sure ! So some steps moving forward: we can abandon this approach; the easiest hack, but very much a hack, is I believe for an elf person to manually alter the write/execute flags on sections. Any other ideas ? |
This is representable in executable files, just not in object files, which is what this library targets. It actually makes sense -- there's no need to embed this information in the object file when what really matters is the final link, when the linker will have all the information on what segments go where from the linker script/arguments. So all we need is a way to embed information in the object file that the linker will pay attention to (LC_LINKER_OPTION) or tell the user to add stuff to a linker script. Ways to proceed:
|
@m4b have any comments on how to proceed here? (since you wished i had pinged sooner on the other PR) |
I'm not sure where we landed to be honest. This is still marked a draft, but you seemed to have some reservations about merging? Do you still feel that way, or do you want to merge the changes as it stands here? Could you give a high-level overview of the downsides or dangers (if any) of merging what you have? |
My initial reservation about merging this came from not being able to find a way to implement this feature for mach-o. I then found a way, but still had/have reservations because the method is kinda meh (embedding linker command-line options into the object file). Which lead to the options i mentioned awhile ago. For the most part, I think this is okay to merge? (I don't remember the state of the code though.) I just want there to be a plan for how it'll eventually be implemented for mach-o. And if the plan is not to implement it for mach-o (because the potential solution is diagusting or causes issues or something) then I want you to consider whether it's okay to have an api that'll only ever work for one file type. That's it. |
I'm not sure how any of this translates over to the mach file format, so I haven't touched it yet.
Fixes #104 , though I haven't tested it yet.
A different approach may be needed, as this is kinda just mirroring the SectionBuilder for ELF.