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

0.4.0 #14

Merged
merged 18 commits into from
Mar 18, 2020
Merged

0.4.0 #14

merged 18 commits into from
Mar 18, 2020

Conversation

maciejhirsz
Copy link
Owner

@maciejhirsz maciejhirsz commented Mar 18, 2020

  • Fixed a memory leak in beef::lean::Cow, found thanks to Miri and @RalfJung (closes lean leaks #12).
  • Added impl_serde feature which provides a Serialize and Deserialize trait implementations. Note that it's currently impossible to make it work with #[serde(borrow)] (relevant PR).
  • Added is_borrowed and is_owned methods.
  • beef::Cow<'a, T> is now a type alias to beef::generic::Cow<'a, T, Wide> where Wide is private. This doesn't change anything, merely hides Option<NonZeroUsize> as implementation detail and makes errors more readable.
  • Due to internal refactoring, const_fn feature now adds two separate functions instead of one: const_str and const_slice for borrowing &'static str and &'static [T] respectively. This is the only breaking change, this release.

@maciejhirsz maciejhirsz mentioned this pull request Mar 18, 2020
@RalfJung
Copy link
Contributor

Looks like one of the new tests does a lot, and since Miri is pretty slow that just takes waaaay too long.

What we do in other test suites is to either disable that test in Miri, or (better) reduce the number of iterations in Miri. You can use cfg(miri) to test if this is being compiled for Miri.

@maciejhirsz
Copy link
Owner Author

Great, thank you a lot for the help @RalfJung! Since Miri was complaining about the fat pointer, and since according to documentation of ptr::slice_from_raw_parts creating a slice with invalid length is UB (though I don't think there was any unsafety in practice), I've split the pointer, new Cow looks like:

pub struct Cow<'a, T: Beef + ?Sized + 'a, U: Capacity> {
    /// Pointer to data
    ptr: NonNull<T::PointerT>,

    /// This usize contains length, but it may contain other
    /// information pending on impl of `Capacity`, and must therefore
    /// always go through `U::len` or `U::unpack`
    fat: usize,

    /// Capacity field. For `beef::lean::Cow` this is 0-sized!
    cap: U::Field,

    /// Lifetime marker
    marker: PhantomData<&'a T>,
}

I've renamed len to fat where appropriate to differentiate between usable length from which a fat pointer can be made, and potentially invalid length that might contain capacity as is the case in owned lean::Cow. I was afraid I wouldn't be able to make const_fn borrows work without a fat pointer, but it turns out to be workable with separate functions for str and slices.

src/lib.rs Outdated Show resolved Hide resolved
@CAD97
Copy link

CAD97 commented Mar 18, 2020

since according to documentation of ptr::slice_from_raw_parts creating a slice with invalid length is UB

That's not my reading. And ptr::slice_from_raw_parts is safe to call, so it cannot be the case.

This function is safe, but actually using the return value is unsafe.

What's unsafe is then dereferencing the constructed raw slice pointer, which is UB via all the normal pointer provenance rules.

@maciejhirsz
Copy link
Owner Author

What's unsafe is then dereferencing the constructed raw slice pointer, which is UB via all the normal pointer provenance rules.

Right, the problem is I was dereferencing this invalid pointer, even though all I was doing with it was calling .len(). Since there is no ptr::slice_into_raw_parts I'd have to do something ugly like transmute it into (usize, usize) and read that, but that too cursed for me.

@maciejhirsz
Copy link
Owner Author

That said, these are equivalent apparently:

#[no_mangle]
fn len(ptr: *const [u8]) -> usize {
    unsafe { std::mem::transmute::<_, (usize, usize)>(ptr).1 }
}

#[no_mangle]
fn len_UB(ptr: *const [u8]) -> usize {
    unsafe { (*ptr).len() }
}
len:
	movq	%rsi, %rax
	retq
.set len_UB, len

@CAD97
Copy link

CAD97 commented Mar 18, 2020

I think I've mentioned it before, but you actually can get the length from a raw slice pointer on stable today:

fn raw_slice_len<T>(slice: ptr::NonNull<[T]>) -> usize {
    let slice = slice.as_raw() as *mut [()];
    (&*slice).len()
}

This works because [()] is a zero sized type with minimal alignment, and so long as the pointer is properly aligned and nonnull, Rust says dereferencing a pointer to a zero-sized place is defined and in fact does not even depend on the pointer having been allocated.

That said, I think the split design for beef::Cow is probably better in the long run.

@RalfJung
Copy link
Contributor

Cc rust-lang/rust#60639

@RalfJung
Copy link
Contributor

@maciejhirsz

That said, these are equivalent apparently

It is meaningless to look at the assembly of a program that has UB. In that case the compiler just outputs something which might or might not be valid assembly, and you can read it like tea leafs. Also see this blog post.

@maciejhirsz
Copy link
Owner Author

maciejhirsz commented Mar 18, 2020

That said, I think the split design for beef::Cow is probably better in the long run.

Yeah. I'm just running some benchmarks against my experimental json-rust branch and there are no performance drops I can measure, so I'm quite happy with this.

Just waiting for serde 1.0.105 to be published now so I can add tests for borrows from json and going to merge and publish this (speaking of, those probably should be disabled in Miri).

@maciejhirsz maciejhirsz merged commit 8c05a0d into master Mar 18, 2020
@maciejhirsz maciejhirsz deleted the happy-miri branch March 18, 2020 19:20
@RalfJung
Copy link
Contributor

speaking of, those probably should be disabled in Miri

Why that?

@maciejhirsz
Copy link
Owner Author

I didn't think we need to run serde_json parsing through Miri, and Serialize / Deserialize impls are both one-liners. Looking at travis logs now, it only adds a second, somehow I thought it would make a bigger difference.

@RalfJung
Copy link
Contributor

Miri is slow but not that slow. ;)

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.

lean leaks
3 participants