Skip to content

Conversation

@iximeow
Copy link
Member

@iximeow iximeow commented Jul 15, 2022

i tripped over this crate after looking at usdt (unfortunately, eBPF USDT and DTrace USDT are different things, and so this crate turned out not so useful for me :) ) - one of the things that stuck out is that some of the asm!() use doesn't seem so necessary. so i've tugged on the threads a bit and this is the first/simplest/easiest thing to fall out.

while this is about equivalent, it makes an unsoundness a little
more apparent: the &u8 to be swapped into is a shared reference, e.g.
Rust can expect that it is not going to be mutated. we will mutate the
referent by AtomicU8::swap'ing it. there was a comment here talking
about using AtomicU8::from_mut to get an AtomicU8 which would have
exposed the same issue. the data being modified needs to be &mut [u8]
to make any impl of this function sound w.r.t borrowing rules.

now, i'm not making that change in this commit, and here's why:

  • read_and_update_record_version is called by ..
  • process_probe_record, which is called by ..
  • process_section, which is called by ..
  • (a): extract_probe_records_from_section. this function talks about
    storing symbols and data in a mutable section so it's loaded in a way we
    can mutate, so the from_raw_parts here could defensibly replaced
    with a from_raw_parts_mut and get us towards a &mut [u8]-taking
    process_section ...
  • (b): dusty::main::probe_records. this is a bigger problem. this
    reads a file (sure, could be made mut data), then uses object to
    parse the object and find a section containing dtrace probes. object
    is not designed to be handing out mutable refs to sections, or even
    mutate the object being parsed at all. we could lean into the
    unsoundness and "errmmm trust me" of what's happening here, transmute
    the section to &mut, and hope that this generally continues working.
    it probably will?

but before doing this i'd like to at least discuss options a bit :)

full disclosure: i don't have dtrace on my linux system, and don't have an alternate setup with dtrace support immediately handy. so i've run the in-crate tests to the extent i can (i can't sudo dtrace, for example!) and those seem to pass. it could be that switching from the lock xchg asm to an AtomicU8 lets rustc move some code around that makes this before/after less equivalent than it should be, due to the unsoundness of mutating data in a &[u8].

while this is semantically equivalent, it makes an unsoundness a little
more apparent: the `&u8` to be swapped into is a shared reference, e.g.
Rust can expect that it is not going to be mutated. we will mutate the
referent by `AtomicU8::swap`'ing it. there *was* a comment here talking
about using `AtomicU8::from_mut` to get an `AtomicU8` which would have
exposed the same issue. the `data` being modified needs to be `&mut
[u8]` to make any impl of this function sound w.r.t borrowing rules.

now, i'm not making that change in this commit, and here's why:
* `read_and_update_record_version` is called by ..
* `process_probe_record`, which is called by ..
* `process_section`, which is called by ..
* (a): `extract_probe_records_from_section`. this function talks about
  storing symbols and data in a mutable section so it's loaded in a way we
  _can_ mutate, so the `from_raw_parts` here could defensibly replaced
  with a `from_raw_parts_mut` and get us towards a `&mut [u8]`-taking
  `process_section` ...
* (b): `dusty::main::probe_records`. this is a bigger problem. this
  reads a file (sure, could be made `mut data`), then uses `object` to
  parse the object and find a section containing dtrace probes. `object`
  is not designed to be handing out mutable refs to sections, or even
  mutate the object being parsed at all. we could lean into the
  unsoundness and "errmmm trust me" of what's happening here, transmute
  the section to `&mut`, and hope that this generally continues working.
  it probably will?

but before doing this i'd like to at least discuss options a bit :)
@bnaecker
Copy link
Collaborator

Obsoleted by #79. Thanks again for the detailed questions and suggestions @iximeow!

@bnaecker bnaecker closed this Jan 18, 2023
@iximeow
Copy link
Member Author

iximeow commented Jan 20, 2023

oh that addresses the unsoundness w/ transmuting into a mut ref too, that's great! thanks!

@iximeow iximeow deleted the less-asm branch January 20, 2023 01:23
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