#Fixing UB in random crates

4312 messages · Page 5 of 5 (latest)

tough leaf
#

hmmmm

tender nimbus
#

do we emit nounef on integers for -Zstrict-validity-checks or however it was called

tough leaf
#

no, that flag doesn't change noundef gen

#

it could

#

but that would be adding UB

#

to some programs

#

so, seemed unwise

tender nimbus
#

doesn't it panic though on ub

#

hmm, well, only on mem::unini

#

there are other ways to get uninit integers

haughty mica
#

Yes but people don't do those and think it's okay

tough leaf
#

like MaybeUninit::uninit().assume_init() which is what i do now if i need to get an uninit value that has noundef

#

(for demo purposes ofc, not prod code :P)

tender nimbus
#

I was more thinking of malloc

tough leaf
#

that too

haughty mica
#

malloc doesn't return a value and people know that reading from it is bad

tender nimbus
#

maybe aggressive vec::set_len

#

But I don't think this could cause issues usually

#

Even with noundef

haughty mica
#

idk about set_len

tough leaf
#

it does say the set_len is UB if the values aren't init

haughty mica
#

That's a fun/not fun case to me

tough leaf
#

but

#

it feels more like a safety invariant?

#

and like

#

and the library does not care

#

maybe

haughty mica
#

Drop cares

tough leaf
#

true

#

safety invariant: don't drop this

#

don't panic

haughty mica
#

Yeah exactly

tough leaf
#

hmmm how do i get a noundef load in C

haughty mica
#

Lulw

tough leaf
#

oh also @haughty mica are your asan run results public
or are they private for "this is actually a lot of real issues"

#

iirc you were doing asan runs?

haughty mica
#

They are not put up anywhere, more for "I don't know where to put them" reasons

tender nimbus
#

/asan/ub

haughty mica
#

miri.saethlin.dev/asan/ub thonk

#

Here are the ASan logs

#

I use rg + less -CR logs/the-crate to search and browse them

#

You will probably mostly find the LLVM bug that I reported, and I think I've patched all the other exciting ones

#

I really need to write my talk and plan my travel for RustConf

#

That is to say, I won't be doing this sort of stuff for a bit

tender nimbus
#

I wish rust conf wasn't on the other side of the world ferrisPensive

tender nimbus
haughty mica
thorny venture
tender nimbus
#

The magic crab told me that Zürich would be a really good location for RustConf ferrisClueless

thorny venture
#

I would defo fly to zurich for a conf (admittedly, it's just an hour away so 🤷)

tender nimbus
#

i would also go to Zürich for a conf ferrisBut

knotty oar
#

they had one there in 2016 i think

tender nimbus
haughty mica
#

I would also fly there

tough leaf
#

okay
strict uninit checks checking by simply making an instance of the type and then asking "hey are you valid" is Being Worked On

#

and it's actually simpler

#

?miri

#![feature(arbitrary_enum_discriminant)]
#[repr(u8)]
enum InvalidAtZero {
  Zero(std::num::NonZeroU16) = 0,
  One(u8) = 1,
}

let _: InvalidAtZero = unsafe { std::mem::zeroed() };
small apexBOT
# 
warning: variants `Zero` and `One` are never constructed
 --> src/main.rs:5:3
  |
4 | enum InvalidAtZero {
  |      ------------- variants in this enum
5 |   Zero(std::num::NonZeroU16) = 0,
  |   ^^^^
6 |   One(u8) = 1,
  |   ^^^
  |
  = note: `#[warn(dead_code)]` on by default

error: Undefined Behavior: constructing invalid value at .value.<enum-variant(Zero)>.0: encountered 0, but expected something greater or equal to 1
 --> src/main.rs:9:33
  |
9 | let _: InvalidAtZero = unsafe { std::mem::zeroed() };
  |                                 ^^^^^^^^^^^^^^^^^^ constructing invalid value at .value.<enum-variant(Zero)>.0: encountered 0, but expected something greater or equal to 1
  |
  = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
  = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
  = note: backtrace:
  = note: inside `main` at src/main.rs:9:33

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
tough leaf
#

yeah
miri catches this

#

but strict-init-checks can't

#

so once we remove the hacks to keep shit working, we can simplify the code for mem::uninit/zeroed checks by simply going "just make an instance of the type and ask if it's valid"

tough leaf
#

MSan has an "eager checks" option which checks noundef arguments and return values. Rustc currently doesn't enable this option, but probably should?

#

👀

#

yes

yes it should

left folio
#

Hi! I'm the author of second-stack - a fast allocator for large, short-lived or variable sized allocations (similar to arena allocators like bumpalo, but with stack semantics instead of arena semantics... allowing for nice things like Drop support).

#

I'd like to know generally how to get a code review for potential UB.

#

The main concern for me about UB is less about the threadlocal APIs (though, those require care and should be reviewed) and more with the Stack type which affords more control. Because the API uses interior mutability, there is sort of a potential for shared mutability. But, I think that potential is removed because Stack is both !Send and !Sync due to the UnsafeCell contained within.

#

Is this the right place to check?

tough leaf
#

miri complains about the soak test (i lowered the thread count to make miri actually able to complete running it)

#

the full stack trace, i can probably minimise this down to a simpler test case

📎 message.rs
left folio
#

@tough leaf Thanks. I'll figure out how to run the test with Miri.

tough leaf
# 
    let handle_count = if cfg!(miri) { 4 } else { 64 };
    let mut handles = Vec::with_capacity(handle_count);

    for _ in 0..handle_count {
tender nimbus
left folio
tender nimbus
#

tmp is dropped though? when it returns

#

or wait

tough leaf
#

yeah it's freed when you return
but

#

vec hands out "dangling" memory

#

for zsts

#

but also

tender nimbus
#

oh it's only read during the the function

tough leaf
#

using a vec here os really overkill

tender nimbus
#

it's fine

left folio
#

Yeah, I see that as_mut_ptr does not consume self, so this would be freed after f is called.

tough leaf
#

i guess

tender nimbus
#

also some internal functions should be marked unsafe

#

like force_dealloc

left folio
tender nimbus
#

and the Miri issue that 522 found

#

When writing unsafe code, always run miri

#

(also I don't think this channel is for requesting unsafe code review, #dark-arts is just fine but whatever)

left folio
#

Yes, I'm trying to reproduce that issue now but haven't seen it come up. (The test is randomized to include every possible problem that I couldn't think of)

left folio
haughty mica
#

Run Miri, and run with ASan + build-std

left folio
haughty mica
#

Both are nightly-only

Miri is

rustup component add miri
cargo miri test

ASan is

RUSTDOCFLAGS=-Zsanitizer=address ASAN_OPTIONS="detect_leaks=0 detect_stack_use_after_return=1" RUSTFLAGS=-Zsanitizer=address cargo test -Zbuild-std --target=$(rustc --version --verbose | grep ^host: | cut -d' ' -f2)
#

I feel like people talk or joke about me detecting UB or finding UB and whatnot but all I do is run the above

#

Very rarely would I audit code by hand, not that it is a bad idea to do that

left folio
#

I'm not having much luck with the last part:

# 
  = note: clang: warning: argument unused during compilation: '-pie' [-Wunused-command-line-argument]
          ld: warning: directory not found for option '-L/Users/zacharyburns/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/x86_64-unknown-linux-gnu/lib'
          ld: warning: directory not found for option '-L/Users/zacharyburns/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/x86_64-unknown-linux-gnu/lib'
          ld: unknown option: --as-needed
          clang: error: linker command failed with exit code 1 (use -v to see invocation)
tough leaf
#

cargo fuzz run is all i do

manic tangle
haughty mica
#

Apple struggle

tough leaf
#

ah rip

#

does apple not support sanitizers

left folio
#

Ah... because I have a macbook. I didn't ask for this, my employer gave it to me.

haughty mica
#

Oh

#

I see

#

no no no

#

I have a --target in there that you need to switch to your apple host target

left folio
#

How does this fair on Windows? I have a Windows machine as well.

haughty mica
#

I should really replace that with a subshell thingy that extracts the host target

tough leaf
haughty mica
#

UGH

tough leaf
#

rustc knows the host target doesn't it

haughty mica
#

There's an issue about this on the std-aware-cargo repo I think

left folio
#

In your snippet you have... cargo test -Zbuild-std --target=$(rustc --version --verbose | grep ^host: | cut -d' ' -f2)

haughty mica
#

Yes I just edited it

left folio
#

Shouls this be cargo miri test...?

haughty mica
#

No

neon tiger
haughty mica
#

Aaaaaaaaaaaaaaa

left folio
#

Ok. What's the point of running the test with std built locally without miri?

#

(Sorry, learning and asking basic questions)

haughty mica
# left folio Ok. What's the point of running the test with std built locally without miri?

The standard library is by default built

  1. Without the ASan instrumentation
  2. Without debug assertions
    Because it is distributed in a compiled form, and only one compiled form, which is the default release profile.

When using sanitizers you should always use -Zbuild-std, because the sanitizers alter code generation to insert a bunch of extra stuff all over the place, and if part of your program doesn't have the extra stuff in it, things can go awry. You can get false positives and false negatives.
Additionally, turning on -Zbuild-std and running in debug mode gets you all the debug assertions in the standard library which I wrote many of so I love them very much. They also catch some things that people do which they think are valid but aren't if you read the docs. It's just easy to intuit the wrong thing, depending on your background.

#

I have plenty of patience for beginnner questions, but -Zbuild-std and then the what about Windows is really hitting all the things that frustrate me

#

-Zbuild-std should be the default, it's a huge mistake that it wasn't, I think it was motivated by a lot of the wrong reasons, and the fact that you need to pass the target explicitly is just an unnecessary thing to top it all off

left folio
#

I apologize for touching on some sore spots there.

#

Just trying to write the best libraries I can.

neon tiger
#

I was pulling your leg a lil there too

haughty mica
#

The day has been a rollercoaster thats for sure

neon tiger
#

fwiw I totally agree, -Zbuild-std should've been the default for a long time

#

it's immensely useful for so many totally different things

haughty mica
#

I'm so haunted by it honestly, ever since I randomly saw the tweet from Oxide about how my debug assertions caught a bug in heapless that caused their bootloader to hang

#

I think a lot of embedded people use it and simply don't talk about it much because they are used to being uncared-for

#

Bryan used the term "self-sufficient" when I asked about it

#

I think that's his spin tbh

tough leaf
#

it does inflate the build time but like

miri builds std
and is fine, because it only needs to do it on every update
there's no reason why rust can't cache the built-std in some common place

haughty mica
#

Yes exactly

#

And if we're so worried about download size, shipping the source is smaller

neon tiger
#

gee if only there was some kind of sound caching mechanism for build artefacts

tough leaf
#

i do it

neon tiger
#

also, the average crate has 250000 dependencies

neon tiger
tough leaf
#

i have my $CARGO_TARGET_DIR set to ~/.cache/cargo/target

#

so all build artifacts go in there

neon tiger
#

I just no longer have any patience for tooling that makes me do cache invalidation manually

#

"oh I changed this part I rarely change, now I need to delete this and run two commands instead of the one"

#

if you cache things, please key the cache by all the things that are relevant

#

oof I just remembered having to mess around with xargo

#

and then cargo-xbuild

haughty mica
#

Miri uses xargo

neon tiger
#

wait what

haughty mica
#

Yuuuuuuup

neon tiger
#

why not build-std?!

#

probably historical reasons but still

haughty mica
#

I do not think so

neon tiger
#

xargo has been dead for years

haughty mica
#

No actually

#

Ralf maintains it

#

Checkmate

solemn pollen
#

sorry

#

didnt know there was an alternative to xargo

neon tiger
#

it's just a flag to cargo

#

-Zbuild-std

#

it's built-in, but it's "experimental"

#

I've been using it for all kinds of weird crap pretty much since the day it existed (that's years ago) with 0 issues. It works scarily well

#

most robust experimental feature I've ever seen

#

(it's possible I've just been lucky)

haughty mica
#

It has edge cases where it is totally broken

#

The one I'm familiar with is the conflict with -Zinstrument-coverage

neon tiger
#

not that I'm surprised about that

#

but also, not building std is similarly broken in edge cases ferrisBigBrain

haughty mica
#

Yeah that's why we shove everything into the nightly-only category so we can pretend it isn't broken and is just shipped in an incomplete state or something

#

Really I'm just worried about the understaffing of the cargo team

bitter nymph
tender nimbus
#

bitvec does many int2ptr casts corro

left folio
#

I'm back with a minified UB from second-stack that I would like to better understand.

# 
fn minified_ub() {
    unsafe {
        let mut vec = Vec::<u8>::with_capacity(2);
        let base_ptr = vec.as_mut_ptr();
        std::mem::forget(vec);

        let mut slice = slice::from_raw_parts_mut(base_ptr as *mut MaybeUninit<u8>, 1);
        // Re-assign to grow the slice from the same allocation.
        // Miri does not complain if we replace "slice.as_mut_ptr()" with "base_ptr"
        slice = slice::from_raw_parts_mut(slice.as_mut_ptr(), 2);
    }
}
#

Here we: 1) crate an allocation, 2) Get a slice of it, 3) later decide we want to reserve more of the allocation, so overwrite our mutable reference with an expanded view of the same allocation

tender nimbus
#

What's the Miri error?

left folio
# 
test minified ... error: Undefined Behavior: trying to reborrow <192429> for Unique permission at alloc80483[0x1], but that tag does not exist in the borrow stack for this location
   --> /Users/zacharyburns/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/slice/raw.rs:141:9
    |
141 |         &mut *ptr::slice_from_raw_parts_mut(data, len)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |         |
    |         trying to reborrow <192429> for Unique permission at alloc80483[0x1], but that tag does not exist in the borrow stack for this location
    |         this error occurs as part of a reborrow at alloc80483[0x0..0x2]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <192429> was created by a retag at offsets [0x0..0x1]
tender nimbus
#

Ah right

#

So

#

slice has a length of 1
So it's only allowed to access a single element

#

And not the second element

#

When you get the pointer from the slice, that pointer still only has access to this one element

#

So you can't create a slice of length 2 from it

#

But with the base_ptr, that's fine, since it has permissions to access the whole memory of the vec

left folio
#

Ok. So, I need to keep around a copy of the base ptr (even though it has the same value as the slice ptr). Correct?

tender nimbus
#

That would solve the problem

#

Another solution is to not use reference sliced but pointer slices instead

#

This permission (provenance) shrinking is a property of references

#

Using *mut [T] wouldn't have the problem

left folio
#

Yes, I understand now. Thanks.

tender nimbus
#

With std::ptr::slice_from_raw_parts

left folio
#

Is this worth yanking my previously published versions of the crate for or can I just fix the code, publish a new release and move on?

tender nimbus
#

just fix it

#

no yanking required

#

that UB isn't dangerous right now and won't break horribly

left folio
#

Great, thanks for your and everyone else's help today and yesterday! I'll go fix it now.

haughty mica
#

Or, well, at least the span part

#

I should really improve these errors

#

It's a pretty easy leap to add a "because" or something in there

#

BTW you're supposed to be able to put together what nils explained because

trying to reborrow <192429> for Unique permission at alloc80483[0x1]

help: <192429> was created by a retag at offsets [0x0..0x1]

#

So you're supposed to be able to realize that the access using the tag is at an offset beyond the retag that created the tag

#

But I am realizing more how many people are using Miri when they are not familiar with these concepts already, so I will have to convince Ralf that is the case

tough leaf
#

but that more applies to heavily used crates tbh

left folio
#

Yeah. So, this is unfortunate, but I agree with the assessment that not everyone will be familiar with stacked borrows in-depth. Ideally everyone who writes unsafe code is intimately familiar. But in practice, the content is dense and people have to weigh the tradeoff of learning that against other constraints on their time.

haughty mica
#

Oof I don't know about that

#

Tools should be helping, but in unsafe code there is absolutely no substitute for learning the rules properly. That is the nature of unsafe programming. We could tip the scales a bit by trying to make it easier to learn, but that doesn't change the fact that you do need to learn. If your code passes the borrow checker, it doesn't have borrowing issues. If your code passes Miri... 🤷 it might still have aliasing problems.

#

I also think Ralf actually expects people to click that URL and read about Stacked Borrows, and I think more prompting is required

left folio
#

For sure. To counter that though, there were multiple previous versions of stacked borrows that were wrong (which people may have read and carried confusion with), and a ton of the content out there comes with several caveats and statements of ambiguity/unfinished ideas.

#

Even the very error message says "but the Stacked Borrows rules it violated are still experimental"

#

There's a lot of prominent people (including Ralf) who often answer questions about UB with "I don't know, that's being worked out".

tender nimbus
#

Well, it's numbers next to numbers for the top one

haughty mica
#

The allocid is not irrelevant though that's the thing

tender nimbus
#

yes, it should be displayed, but it's not as relevant as the offset here

left folio
#

I think the URL would be more helpful if it pointed to a relevant part of the stacked borrows spec. I did click the URL, and then reacted with oh yeah, I remember trying to read this last year and it was a mess. (I think it was a previous version of the spec though).

haughty mica
#

That is useful feedback

tough leaf
#

Can we reliably identify specific "things" that people do wrong under SB usefully?

haughty mica
#

Absolutely

tough leaf
#

like calling as_mut_ptr twice as one Thing

haughty mica
#

That too

tough leaf
#

and then have basicaly rustc --explain

#

but for SB

#

and probably a link to a document on the miri repo

haughty mica
#

Hm that's a lot of work for something Ralf wants to fix

tender nimbus
#

has anyone ever used rustc --explain ?

haughty mica
#

Yes

#

It's dogshit

tough leaf
#

i haven't, tbh

tender nimbus
#

same

haughty mica
#

The explanations rarely contain more information than the errors themselves

#

And I've never found them useful if they do

tough leaf
#

i think the general "here's what you did wrong generally speaking, here's what that looks like, here's how to fix it"

#

is not a bad idea

tender nimbus
#

especially since errors tend to be very general

haughty mica
#

How to fix it is very hard

#

I think just a better error message is much higher yield

tough leaf
#

yeah

left folio
#

(BTW) I'll end up reading the whole spec again personally... none of the sentiment I have expressed about aversion to do so should be taken as a "correct" stance, I just think that people will be lazy.

tough leaf
#

i mean i haven't read it

left folio
#

lol

haughty mica
#

I totally read it once but it didn't help me understand

left folio
#

(Sorry for being snarky 👆 )

haughty mica
#

That doesn't mean I'm ignorant of the rules

tender nimbus
#

I've read it once and while it has helped me know a bit more about SB, it was not the reason I could help you above, that was just by experience ferrisBut

haughty mica
#

I spend most of my evenings editing the code that implements SB, and that's how I know it

tender nimbus
# left folio

saethlin knows like the entire Miri stacked borrows checking source code ferrisBut

#

which is just a spec but written in rust instead of english

left folio
#

Sure. But we can expect basically 0 users of unsafe to have that same experience as "I am Saethlin"

haughty mica
#

I agree in general

#

But I do suggest that people read the code that implements SB instead of the spec because that's what I found accessible

left folio
haughty mica
#

The core implementation is 1k lines of Rust

left folio
#

If that's the standard, maybe link to the code.

#

There's a lot of demand to write unsafe code. But the expectations for doing so need to be clear.

haughty mica
#

Uhhhhhhh

#

Hunh

#

I think there is an expectation mismatch here

#

I expect that people writing C know the C language and all its UB because they are constantly on the hook to avoid it.

I think that is considerably harder than reading any reference material for Stacked Borrows

left folio
#

I think we're generally in agreement about most things (for example, that people should be reasonably expected to learn the requirements of using unsafe). But, consider the story from my perspective... I've been using Rust for ~4 years, and programming for ~20. I've read the nomicon. I tried to read SB (when it was more like a PhD thesis). I have proactively attempted to learn the rules and abide by them. By the admission of everyone here, the SB spec linked to is "unhelpful". So the question I ask is, for someone on the journey to learning unsafe, how are they supposed to discover the path? Apparently the path is to read the Miri source. Somewhere in the documentation I would hope that a learner could discover a link to that source. Without a link, I would never have guessed that the most fruitful path would be to try to locate and understand the source code implementing SB. Will I read it? Sure. But I have no expectation that other writers of unsafe will discover the path.

#

Please accept this criticism with grace. I very much appreciate everything that all of you do.

tender nimbus
#

my path was hanging out in #dark-arts and using unsafe myself with miri

#

there's a pretty good high level explanation of SB at the end of "learning rust with entirely too many linked lists"

#

but it's maybe a bit too high level

left folio
#

I read most of that too... but I think that was a long while ago before I was interested in unsafe. My bad.

#

Also, the more that the path (whichever path it is) respects people's time, the more likely it will be that people write correct unsafe. Everybody has a breaking point.

#

There's a curve there.

tender nimbus
#

i have yet to see a cute diagram about sb. it's always either rust or english. that would probably help as well

#

imagine a diagram where all the tags are getting thrown out the stack when it's borrowed as &mut and they're all like :O

left folio
#

Also know that I understand that there are constraints on your time, and no-one is obligated to do anything. I'm just trying to establish consensus that the situation for learning unsafe could be improved rather than landing at "well, you should have known to read the source".

tender nimbus
#

oh it absolutely should be improved

#

but yeah

left folio
#

Also, ❤️ to everyone here.

haughty mica
#

There is interest in an SB visualizer but the amount of data gets out of hand very fast

#

I'm sure there are ways to deal with it but we have 3 people with no experience in that kind of data vis in the conversation

west violet
tender nimbus
#

(there's a post called "fuzzing random crates" here by 522, might be relevant ferrisBut)

manic tangle
#

seize data race is fixed ferrisRelieved

haughty mica
#

Now that my optimization patches have landed, I'm re-running all the crates I know of that OOM or time out.

#

This has so far gone exactly how you would expect

tender nimbus
#

UB? Or more OOMs?

haughty mica
#

I OOMed my system

#

Fixed my run config, now I'm just dancing on the edge

#

The re-running of the timeouts is going to take about 3 days

tender nimbus
tender nimbus
#

gonna hijack this thread since the SB thread is busy
Although maybe #957720175619215380 would be better but whatever ferrisballSweat

haughty mica
#

Yeah

#

I'm not surprised

tender nimbus