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.

Sign up for GitHub

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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Support for custom permissions for custom sections. #105

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
56b682f
WIP support for custom permissions for custom sections.
kitlith Dec 6, 2019
446ef4f
FIXUP: cargo fmt
kitlith Dec 9, 2019
ff3e946
Add tests, add more flags to Decl's.
kitlith Dec 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions src/artifact/decl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,24 @@ impl DefinedDecl {
}
}

/// Accessor to determine whether contents are executable
pub fn is_executable(&self) -> bool {
match self {
DefinedDecl::Section(a) => a.is_executable(),
DefinedDecl::Function(_) => true,
DefinedDecl::Data(_) => false,
Copy link
Owner

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?

Copy link
Owner

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?

Copy link
Contributor Author

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?

Copy link
Owner

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,
}

?

Copy link
Owner

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?

Copy link
Contributor Author

@kitlith kitlith Dec 8, 2019 •
edited
Loading

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.

Copy link
Owner

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:

  1. it's a breaking change
  2. 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)
  3. 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.

Copy link
Contributor Author

@kitlith kitlith Dec 8, 2019 •
edited
Loading

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.

Copy link
Owner

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?

Copy link
Contributor Author

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.

}
}

/// Accessor to determine whether contents will be loaded at runtime
pub fn is_loaded(&self) -> bool {
match self {
DefinedDecl::Section(a) => a.is_loaded(),
DefinedDecl::Function(_) => true,
DefinedDecl::Data(_) => true,
}
}

/// Accessor to determine the minimal alignment
pub fn get_align(&self) -> Option<u64> {
match self {
Expand Down Expand Up @@ -500,6 +518,9 @@ pub struct SectionDecl {
kind: SectionKind,
datatype: DataType,
align: Option<u64>,
writable: Option<bool>,
executable: Option<bool>,
loaded: bool
}

impl SectionDecl {
Expand All @@ -512,6 +533,9 @@ impl SectionDecl {
kind,
datatype: DataType::Bytes,
align: None,
writable: None,
executable: None,
loaded: false
}
}

Expand All @@ -521,14 +545,68 @@ impl SectionDecl {
false
}

/// Setter for writability
pub fn set_writable(&mut self, writable: bool) {
self.writable = Some(writable);
}

/// Builder for writability
pub fn with_writable(mut self, writable: bool) -> Self {
self.writable = Some(writable);
self
}

/// Accessor to determine whether contents are writable
pub fn is_writable(&self) -> bool {
if let Some(writable) = self.writable {
return writable;
}

match self.kind {
SectionKind::Data => true,
kitlith marked this conversation as resolved.
Show resolved Hide resolved
SectionKind::Debug | SectionKind::Text => false,
}
}

/// Setter for executability
pub fn set_executable(&mut self, executable: bool) {
self.executable = Some(executable);
}

/// Builder for executability
pub fn with_executable(mut self, executable: bool) -> Self {
self.executable = Some(executable);
self
}

/// Accessor to determine whether contents are executable
pub fn is_executable(&self) -> bool {
if let Some(executable) = self.executable {
return executable;
}

match self.kind {
SectionKind::Text => true,
SectionKind::Data | SectionKind::Debug => false,
}
}

/// Setter for loadability
pub fn set_loaded(&mut self, loaded: bool) {
self.loaded = loaded;
}

/// Builder for loadabliity
pub fn with_loaded(mut self, loaded: bool) -> Self {
self.loaded = loaded;
self
}

/// Accessor to determine whether contents are loaded at runtime
pub fn is_loaded(&self) -> bool {
self.loaded
}

/// Get the kind for this `SectionDecl`
pub fn kind(&self) -> SectionKind {
self.kind
Expand Down
40 changes: 19 additions & 21 deletions src/elf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
use crate::{
artifact::{
self, Artifact, Data, DataType, Decl, DefinedDecl, ImportKind, LinkAndDecl, Reloc, Scope,
Visibility,
SectionKind, Visibility,
},
target::make_ctx,
Ctx,
Expand Down Expand Up @@ -514,33 +514,31 @@ impl<'a> Elf<'a> {
(_, DefinedDecl::Section(_)) => name.to_owned(),
};

let section = match decl {
DefinedDecl::Function(d) => SectionBuilder::new(def_size as u64)
.section_type(SectionType::Bits)
.alloc()
.writable(false)
.exec(true)
.align(d.get_align()),
DefinedDecl::Data(d) => SectionBuilder::new(def_size as u64)
.section_type(Self::section_type_for_data(
let mut section = SectionBuilder::new(def_size as u64)
.writable(decl.is_writable())
.exec(decl.is_executable())
.align(decl.get_align())
.section_type(match decl {
DefinedDecl::Function(_) => SectionType::Bits,
DefinedDecl::Data(d) => Self::section_type_for_data(
d.get_datatype(),
def.data.is_zero_init(),
))
.alloc()
.writable(d.is_writable())
.exec(false)
.align(d.get_align()),
DefinedDecl::Section(d) => SectionBuilder::new(def_size as u64)
.section_type(
),
DefinedDecl::Section(d) => {
// TODO: this behavior should be deprecated, but we need to warn users!
if name == ".debug_str" || name == ".debug_line_str" {
SectionType::String
} else if d.kind() == SectionKind::Text && d.get_datatype() == DataType::Bytes {
SectionType::Bits
} else {
Self::section_type_for_data(d.get_datatype(), def.data.is_zero_init())
},
)
.align(d.get_align()),
};
}
}
});

if decl.is_loaded() {
section = section.alloc();
}

let shndx = match def.data {
Data::Blob(bytes) => self.add_progbits(section_name, section, bytes),
Expand Down