(`c2rust-analyze`) Support ptr-to-ptr casts between safely transmutable types, for now limited to same-sized integers #839

kkysen · 2023-02-16T09:41:42Z

Fixes (c2rust-analyze) Handle compatible ptr-to-ptr casts #840

This introduces the concept of equivalent/compatible/safely transmutable types:

Lines 356 to 380 in 2915b8d

    
           /// Determine if `from` can be safely transmuted to `to`, 
        
           /// which is defined as `*(from as *const To)` being a safe operation, 
        
           /// where `from: *const From` and assuming `*from` already was safe. 
        
           /// 
        
           /// Note that this is one-way, and is slightly different from [`core::mem::transmute`], 
        
           /// and more similar to [`core::mem::transmute_copy`]. 
        
           /// 
        
           /// This forms a non-symmetric (one-way) equivalence relation, named `~` below. 
        
           /// Formally, `A ~ B` iff `*a` and `*(a as *const B)` are safe, where `a: *const A`. 
        
           /// 
        
           /// However, safe transmutability is difficult to check completely, 
        
           /// so this function only checks a subset of it, 
        
           /// with these formal rules for all types `A`, `B`: 
        
           /// 
        
           /// * `A = B => A ~ B` 
        
           /// * `A ~ B => *A ~ *B` 
        
           /// * `A ~ B => [A] ~ B` 
        
           /// * `A ~ B => [A; N] ~ B`, where `const N: usize` 
        
           /// * `A ~ B => [A] ~ [B; N]`, where `const N: usize` 
        
           /// * `A ~ B => [A; N] ~ [B]`, where `const N: usize` 
        
           /// * `uN ~ iN`, `iN ~ uN`, where `N` is an integer width 
        
           /// 
        
           /// Thus, [`true`] means it is definitely transmutable, 
        
           /// while [`false`] means it may not be transmutable. 
        
           pub fn is_transmutable_to<'tcx>(from: Ty<'tcx>, to: Ty<'tcx>) -> bool {

Thus, we can now allow ptr-to-ptr casts between safely transmutable pointee types, whereas previously they were only allowed for equal types. In particular, this enables support for string casts, which are produced by c2rust transpile as b"" as *const u8 as *const core::ffi::c_char, where c_char = i8. Thus, this fixes #840.

New tests are added in string_casts.rs to cover various ptr casts, though some of them crash in the rewriter due to having implicitly inserted MIR statements like implicit &raws, which are inserted with addr_of!s. Thus, for some of these (where it works), there are versions with explicit addr_of!s that succeed end-to-end.

c2rust-analyze/src/util.rs

c2rust-analyze/src/context.rs

fw-immunant

LGTM.

c2rust-analyze/src/dataflow/type_check.rs

c2rust-analyze/src/util.rs

c2rust-analyze/tests/analyze/string_casts.rs

kkysen · 2023-02-17T23:17:39Z

@aneksteind, I realized I missed the recursive case here, where a ~ b => *a ~ *b for all a, b where ~ means transmutable, so I've added that now. I'm not sure if this is used anywhere in lighttpd, but it makes the definition more sound and easier to reason about and isn't a large change.

…le types, for now limited to same-sized integers. This introduces the concept of equivalent/compatible/safely transmutable types. This forms an equivalence class among types, as the safe transmutability must be mutual (i.e. transmutable in both directions; no prefix-transmutability). Thus, we can now allow ptr-to-ptr casts between safely transmutable pointee types, whereas previously they were only allowed for equal types. Equal types could have their `PointerId`s unified as they had the same structure, which is still of safely transmutability types, which are safely transmutability because they have the same structure/layout. As safe transmutability is difficult to check abstractly for any two types, for now we limit it to commonly transmuted types that we know are definitely transmutable: same-sized integer types (with potentially different signedness). Thus, this enables support for string casts like `b"" as *const u8 as *const core::ffi::c_char`, where `c_char = i8`, which fixes #840. Note that the above cast (#833) is still not supported due to the string literal `b""` (#837), but the cast itself (in `string_casts.rs` in `fn cast_only`) works.

…ible/safetly transmutable types, not identical.

… `a ~ b` => `*a ~ *b` for all `a`, `b`).

…e-way, now allowing for arrays and slices to decay. This expands the definition of safe transmutability to be one-way. That is, it checks if `*T as *U` is safe, rather than also `*U as *T`. Thus, we can now allow for casts decaying pointers to arrays and slices to pointers to their element type. `do_unify` is modified to also be one-way, which it was already in all call sites. New tests are also added to `string_casts.rs` for all the types of ptr-to-ptr casts. Out of the full string cast, `b"" as *const u8 as *const core::ffi::c_char`, this adds support for the `as *const u8` (from `&[u8; _]`), so only support for the string literal itself remains.

… expanded defintion of safe transmutability.

spernsteiner · 2023-02-22T22:32:09Z

It seems odd to declare that usize and *mut u8 are not safely transmutable, when you can safely convert between them using as, and that conversion behaves exactly the same as mem::transmute. But I agree this conversion shouldn't be supported in this PR - usize has nowhere to attach a PointerId, which is basically the same problem we have with general use of void*. Maybe we need a different name for this property?

Also, it would be good to clearly distinguish between the precise logical property, which may be difficult or impossible to check, and the conservative approximation of that property that we use in the implementation. For example, in the trivial functions PR, the precise property is "calls to this function have no effect on pointer permissions in the caller", and the approximation is "there are no raw pointers reachable through the types in this function's signature".

The casts allowed by this PR will require some additional library support during rewriting - there's no way to safely convert &str to &[i8] using only the standard library.

kkysen · 2023-02-22T22:57:35Z

It seems odd to declare that usize and *mut u8 are not safely transmutable, when you can safely convert between them using as, and that conversion behaves exactly the same as mem::transmute. But I agree this conversion shouldn't be supported in this PR - usize has nowhere to attach a PointerId, which is basically the same problem we have with general use of void*. Maybe we need a different name for this property?

Ptr-to-int transmutes implicitly drop provenance (still being decided) and int-to-ptr transmutes are just UB. They're not safe, and the language I used for this is safe transmutability, so I think we're good here.

See rust-lang/unsafe-code-guidelines#286.

Also, it would be good to clearly distinguish between the precise logical property, which may be difficult or impossible to check, and the conservative approximation of that property that we use in the implementation. For example, in the trivial functions PR, the precise property is "calls to this function have no effect on pointer permissions in the caller", and the approximation is "there are no raw pointers reachable through the types in this function's signature".

I thought I did? Let me check. I at least did in the follow-up PR to this one.

I checked. It's in the documentation here.

The casts allowed by this PR will require some additional library support during rewriting - there's no way to safely convert &str to &[i8] using only the standard library.

Wdym by this? This PR doesn't allow that cast, nor anything to do with &str.

spernsteiner · 2023-02-23T00:05:54Z

It's in the documentation here.

Ah, got it - the official definition is the bit about whether *(a as *const B) and *(b as *const A) are safe.

Ptr-to-int transmutes implicitly drop provenance (still being decided) and int-to-ptr transmutes are just UB. They're not safe

I haven't read the full thread you linked, but I saw the bit at the end about ptr-to-int transmute dropping provenance, which I believe is the same behavior you get from the safe cast my_ptr as usize. Do you happen to know whether they intend to make mem::transmute::<usize, *mut u8> UB, contrary to the safe cast my_usize as *mut u8?

I guess this is a bit of a moot point, since "safely transmutable" is effectively defined in terms of "whatever the Rust devs decide is well-defined".

Wdym by this? This PR doesn't allow that cast, nor anything to do with &str.

Sorry, I forgot the example was using b"" rather than "". But I think the same point applies. The example b"" as *const u8 as *const core::ffi::c_char will get rewritten into something involving a cast from &[u8] to &[i8]. This operation is well-defined, but std doesn't provide a safe function for performing it (as far as I know). So we'll eventually need to write or import a library that does provide such a function so we can do the necessary rewriting.

c2rust-analyze/src/dataflow/type_check.rs

kkysen · 2023-02-23T01:24:25Z

Ah, got it - the official definition is the bit about whether *(a as *const B) and *(b as *const A) are safe.

Yeah.

I haven't read the full thread you linked, but I saw the bit at the end about ptr-to-int transmute dropping provenance, which I believe is the same behavior you get from the safe cast my_ptr as usize. Do you happen to know whether they intend to make mem::transmute::<usize, *mut u8> UB, contrary to the safe cast my_usize as *mut u8?

I guess this is a bit of a moot point, since "safely transmutable" is effectively defined in terms of "whatever the Rust devs decide is well-defined".

The ptr-to-int transmute drops provenance, corresponding to ptr.addr(). That's different from as usize, which is ptr.expose_addr() and preserves provenance. That's why it can't be round tripped back into a pointer, since it has no provence. If it has no provenance, turning it into a pointer is UB since the pointer needs provenance.

See core::ptr::invalid's implementation, which uses transmute precisely because it creates an invalid pointer.

Sorry, I forgot the example was using b"" rather than "". But I think the same point applies. The example b"" as *const u8 as *const core::ffi::c_char will get rewritten into something involving a cast from &[u8] to &[i8]. This operation is well-defined, but std doesn't provide a safe function for performing it (as far as I know). So we'll eventually need to write or import a library that does provide such a function so we can do the necessary rewriting.

It is in core. You can convert safely through CStr:

from &[u8]: CStr::from_bytes_with_nul
to *const c_char: CStr::as_ptr

kkysen · 2023-05-25T05:25:26Z

@spernsteiner, could you take a look at the new separated CastKind handling (4bbb306) if that and the reversion in 720c2fb addresses your comment here: #839 (comment)?

Also, regarding #909 (comment), I'm not sure why #919 is causing that crash related to FIXED. Even without #839 at all, this function

pub fn cast_array_to_ptr_explicit(s: &[u8; 0]) {
    std::ptr::addr_of!(*s) as *const u8;
}

causes master to crash with that same error:

thread 'rustc' panicked at 'building TypeDesc for FIXED pointer requires a related pointee type', c2rust-analyze/src/type_desc.rs:75:5

spernsteiner · 2023-05-26T22:49:07Z

I'll investigate that panic next week and also take a look at this PR.

Fixes the panic described in #839 (comment)

kkysen · 2023-06-05T20:06:48Z

@spernsteiner, I've merged master into this PR after you just merged #945, so that crash is fixed now.

c2rust-analyze/src/dataflow/type_check.rs

c2rust-analyze/src/util.rs

…to use well-defined instead of safe and to use "implies" instead of "and".

…rom `{pl,rv}_lty`, as they are interchangeable.

…` to the slice and array rules.

…han `N > 0` to avoid ZSTs, as then the rule would be unsound.

… it's only sound for non-empty slices, but we can't check that at compile-time.

spernsteiner

Looks good once the conflict + CI is fixed

c2rust-analyze/src/util.rs

…ngth `Const`s.

… docs.

kkysen · 2023-06-09T07:28:32Z

@spernsteiner, given #934 is all approved now, if you plan to merge it tomorrow, I'll wait until that merges to rebase/merge this before merging it into master, as I also want to check if #934 fixes the implicit &raw issue (#909).

kkysen · 2023-06-09T07:33:33Z

@spernsteiner, given #934 is all approved now, if you plan to merge it tomorrow, I'll wait until that merges to rebase/merge this before merging it into master, as I also want to check if #934 fixes the implicit &raw issue (#909).

Actually, there seem to be issues with #934 running on the string_casts.rs tests, and I think it's issues with #934, so I'm going to merge this PR first and then you can take a look at those issues before merging #934.

…tring literals in `string_casts.rs` and `lighttpd-minimal`.

…iled string literals in `string_casts.rs` and `lighttpd-minimal`.

…955) With #839 (adding string cast support) and #934 (fixing #909) now merged, we can now enable transpiled string literals in `string_casts.rs` and `lighttpd-minimal`.

kkysen requested review from aneksteind and spernsteiner February 16, 2023 09:41

kkysen force-pushed the kkysen/analyze-string-casts branch 3 times, most recently from fa98935 to 88babe2 Compare February 16, 2023 19:30

fw-immunant reviewed Feb 16, 2023

View reviewed changes

c2rust-analyze/src/util.rs Outdated Show resolved Hide resolved

fw-immunant reviewed Feb 16, 2023

View reviewed changes

c2rust-analyze/src/context.rs Outdated Show resolved Hide resolved

fw-immunant approved these changes Feb 16, 2023

View reviewed changes

fw-immunant reviewed Feb 16, 2023

View reviewed changes

c2rust-analyze/src/dataflow/type_check.rs Outdated Show resolved Hide resolved

kkysen force-pushed the kkysen/analyze-string-casts branch 2 times, most recently from a7f06c4 to 19d0dac Compare February 16, 2023 22:12

Base automatically changed from kkysen/analyze-strings-casts-tests to master February 16, 2023 22:33

kkysen force-pushed the kkysen/analyze-string-casts branch from 255a8be to d78b925 Compare February 16, 2023 22:39

aneksteind reviewed Feb 16, 2023

View reviewed changes

c2rust-analyze/src/util.rs Outdated Show resolved Hide resolved

aneksteind previously approved these changes Feb 16, 2023

View reviewed changes

aneksteind reviewed Feb 16, 2023

View reviewed changes

c2rust-analyze/tests/analyze/string_casts.rs Show resolved Hide resolved

kkysen added 4 commits February 18, 2023 20:58

(c2rust-analyze) Clarified that do_unify now requires only compat…

68461db

…ible/safetly transmutable types, not identical.

(c2rust-analyze) Support deeper levels of ptr transmutability (e.x.…

da4d961

… `a ~ b` => `*a ~ *b` for all `a`, `b`).

(c2rust-analyze) Formally defined the safe transmutability definition.

62ec8dc

kkysen force-pushed the kkysen/analyze-string-casts branch from e665306 to 62ec8dc Compare February 19, 2023 04:58

kkysen added 3 commits February 18, 2023 21:10

(c2rust-analyze) Updated do_unify docs after the renaming.

a148146

(c2rust-analyze) Fix the is_transmutable_to docs, formalizing the…

26a4275

… expanded defintion of safe transmutability.

spernsteiner reviewed Feb 23, 2023

View reviewed changes

c2rust-analyze/src/dataflow/type_check.rs Outdated Show resolved Hide resolved

Merge branch 'master' into kkysen/analyze-string-casts

b15e24e

spernsteiner mentioned this pull request Jun 5, 2023

analyze: fix panic on addr_of!(...) as ... #945

Merged

Merge branch 'master' into kkysen/analyze-string-casts

6ea8a7e

spernsteiner added a commit that referenced this pull request Jun 5, 2023

analyze: fix panic on addr_of!(...) as ... (#945)

f082f17

Fixes the panic described in #839 (comment)

Merge branch 'master' into kkysen/analyze-string-casts

1fd669c

spernsteiner reviewed Jun 6, 2023

View reviewed changes

kkysen added 5 commits June 8, 2023 12:31

(c2rust-analyze) Adjust wording on safe transmutability definition …

a73e64d

…to use well-defined instead of safe and to use "implies" instead of "and".

(c2rust-analyze) Revert the arg names of do_unify to lty{1,2} f…

435e399

…rom `{pl,rv}_lty`, as they are interchangeable.

(c2rust-analyze) Update safe transmutability rules to add `A ~ B =>…

1317e03

…` to the slice and array rules.

(c2rust-analyze) For the array safe transmutability rule, require t…

60a4376

…han `N > 0` to avoid ZSTs, as then the rule would be unsound.

(c2rust-analyze) Remove the slice rule for safe transmutability, as…

9b43bd6

… it's only sound for non-empty slices, but we can't check that at compile-time.

kkysen force-pushed the kkysen/analyze-string-casts branch from 6b3bdd8 to 9b43bd6 Compare June 8, 2023 20:33

kkysen requested a review from spernsteiner June 8, 2023 20:34

spernsteiner approved these changes Jun 8, 2023

View reviewed changes

c2rust-analyze/src/util.rs Outdated Show resolved Hide resolved

kkysen added 2 commits June 9, 2023 00:07

(c2rust-analyze) .unwrap() on .try_to_scalar_int() for array le…

355b56b

…ngth `Const`s.

(c2rust-analyze) Add fat ptr analysis/proof to safe transmutability…

7f87dc1

… docs.

Merge branch 'master' into kkysen/analyze-string-casts

31baf0a

kkysen merged commit d0fa6ea into master Jun 9, 2023

kkysen deleted the kkysen/analyze-string-casts branch June 9, 2023 08:19

kkysen mentioned this pull request Jun 9, 2023

(c2rust-analyze) Add more (still incomplete) dataflow constraints for ptr casts #947

Open

spernsteiner mentioned this pull request Jun 9, 2023

analyze: new implementation of MIR-to-HIR rewrite lifting #934

Merged

kkysen added a commit that referenced this pull request Jun 12, 2023

(c2rust-analyze) With #839 and #934 now merged, enable transpiled s…

d0d8308

…tring literals in `string_casts.rs` and `lighttpd-minimal`.

kkysen added a commit that referenced this pull request Jun 12, 2023

(c2rust-analyze/tests) With #839 and #934 now merged, enable transp…

7a8e949

…iled string literals in `string_casts.rs` and `lighttpd-minimal`.

kkysen mentioned this pull request Jun 12, 2023

(c2rust-analyze/tests) Enable transpiled string literals in tests #955

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(`c2rust-analyze`) Support ptr-to-ptr casts between safely transmutable types, for now limited to same-sized integers #839

(`c2rust-analyze`) Support ptr-to-ptr casts between safely transmutable types, for now limited to same-sized integers #839

kkysen commented Feb 16, 2023 •

edited

Loading

fw-immunant left a comment

kkysen commented Feb 17, 2023

spernsteiner commented Feb 22, 2023

kkysen commented Feb 22, 2023 •

edited

Loading

spernsteiner commented Feb 23, 2023

kkysen commented Feb 23, 2023 •

edited

Loading

kkysen commented May 25, 2023

spernsteiner commented May 26, 2023 •

edited

Loading

kkysen commented Jun 5, 2023

spernsteiner left a comment

kkysen commented Jun 9, 2023

kkysen commented Jun 9, 2023

	/// Determine if `from` can be safely transmuted to `to`,
	/// which is defined as `(from as const To)` being a safe operation,
	/// where `from: const From` and assuming `from` already was safe.
	///
	/// Note that this is one-way, and is slightly different from [`core::mem::transmute`],
	/// and more similar to [`core::mem::transmute_copy`].
	///
	/// This forms a non-symmetric (one-way) equivalence relation, named `~` below.
	/// Formally, `A ~ B` iff `a` and `(a as const B)` are safe, where `a: const A`.
	///
	/// However, safe transmutability is difficult to check completely,
	/// so this function only checks a subset of it,
	/// with these formal rules for all types `A`, `B`:
	///
	/// * `A = B => A ~ B`
	/// * `A ~ B => A ~ B`
	/// * `A ~ B => [A] ~ B`
	/// * `A ~ B => [A; N] ~ B`, where `const N: usize`
	/// * `A ~ B => [A] ~ [B; N]`, where `const N: usize`
	/// * `A ~ B => [A; N] ~ [B]`, where `const N: usize`
	/// * `uN ~ iN`, `iN ~ uN`, where `N` is an integer width
	///
	/// Thus, [`true`] means it is definitely transmutable,
	/// while [`false`] means it may not be transmutable.
	pub fn is_transmutable_to<'tcx>(from: Ty<'tcx>, to: Ty<'tcx>) -> bool {

(c2rust-analyze) Support ptr-to-ptr casts between safely transmutable types, for now limited to same-sized integers #839

(c2rust-analyze) Support ptr-to-ptr casts between safely transmutable types, for now limited to same-sized integers #839

Conversation

kkysen commented Feb 16, 2023 • edited Loading

fw-immunant left a comment

Choose a reason for hiding this comment

kkysen commented Feb 17, 2023

spernsteiner commented Feb 22, 2023

kkysen commented Feb 22, 2023 • edited Loading

spernsteiner commented Feb 23, 2023

kkysen commented Feb 23, 2023 • edited Loading

kkysen commented May 25, 2023

spernsteiner commented May 26, 2023 • edited Loading

kkysen commented Jun 5, 2023

spernsteiner left a comment

Choose a reason for hiding this comment

kkysen commented Jun 9, 2023

kkysen commented Jun 9, 2023

(`c2rust-analyze`) Support ptr-to-ptr casts between safely transmutable types, for now limited to same-sized integers #839

(`c2rust-analyze`) Support ptr-to-ptr casts between safely transmutable types, for now limited to same-sized integers #839

kkysen commented Feb 16, 2023 •

edited

Loading

kkysen commented Feb 22, 2023 •

edited

Loading

kkysen commented Feb 23, 2023 •

edited

Loading

spernsteiner commented May 26, 2023 •

edited

Loading