Crate does not compile with latest beta

[virtualritz]

Since this is an issue when building with the beta channel already it will forseeably become a bug when trying to build this crate with stable. Basically, there are dozens of those:

error: casting `&T` to `&mut T` is undefined behavior, even if the reference is unused, consider instead using an `UnsafeCell`
   --> src/c_interface.rs:192:31
    |
192 |                 unsafe { &mut *(self._parent as *const P as *mut P) },
    |                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[deny(cast_ref_to_mut)]` on by default

[jabuwu]

it will forseeably become a bug when trying to build this crate with stable

I wonder if that's true? I thought the Rust compiler wouldn't retroactively break code in this way. It's still an issue if this doesn't work in beta, though.

A lot of the errors are coming from c2rust. Hopefully it has been addressed there somehow. C is littered with UB so it's a tricky problem.

[jabuwu]

It seems like this error will be switched back to allow-by-default, at least temporarily: rust-lang/rust#113422

If this is brought to stable, it will break c2rust generated code, so I might have to open an issue over there.

[virtualritz]

it will forseeably become a bug when trying to build this crate with stable

I wonder if that's true? I thought the Rust compiler wouldn't retroactively break code in this way. It's still an issue if this doesn't work in beta, though.

Fair enough. I searched for relevant discussions in rust-lang/rust but wasn't as successfull as you.

Hopefully it will be switched back. I, too, have a bunch of code in my AfterEffects API Rust binding that will cease to work if it isn't.

[Urgau]

Rust compiler contributors here and author of the invalid_reference_casting lint (previously cast_ref_to_mut)

Saw this issue as linked to rust-lang/rust#113422 and wanted to clear some questions you were wondering.

---

it will forseeably become a bug when trying to build this crate with stable

I wonder if that's true? I thought the Rust compiler wouldn't retroactively break code in this way. It's still an issue if this doesn't work in beta, though.

The Rust compiler promises to never break compiling sound code, that means the code must not violate any invariants. This is automatically the case for safe code, for unsafe code it's up to the developer to respect those invariants (otherwise safe code would be as unsafe as unsafe code).

These invariants include but are not limited to (Unsafe Code Guidelines to see more) aliasing, which as the lint is says, the code is pure violation of it. That's why T-lang agreed to have a lint against and it to be deny-by-default.

If this hasn't any caused any miscompilation yet, I wouldn't be surprised if a future version of the compiler miscompiled the code around it, as &mut references are annotated with the noalias LLVM attribute, indicating that nothing else will be accessing that point of memory will that ptr is alive, which is definitely not the case here.

If this is brought to stable, it will break c2rust generated code, so I might have to open an issue over there.

That a shame that c2rust is generating such unsound/UB code. They should really use Cell/UnsafeCell or raw pointers.

Hopefully it will be switched back. I, too, have a bunch of code in my AfterEffects API Rust binding that will cease to work if it isn't.

Sorry, rust-lang/rust#112431 just landed are re-enabled the lint with an improved diagnostics under the invalid_reference_casting name.

[virtualritz]

Hopefully it will be switched back. I, too, have a bunch of code in my AfterEffects API Rust binding that will cease to work if it isn't.

Sorry, rust-lang/rust#112431 just landed are re-enabled the lint with an improved diagnostics under the invalid_reference_casting name.

Looks like my fears were a tad premature so far for the crate I mentioned. I am not doing anything like c2rust in there.

[jabuwu]

@Urgau That makes sense, thanks for dropping in! Indeed, c2rust generates this type of code, but I don't think their usage creates UB. I don't believe that my usage creates UB either. In both cases, a mutable reference is created from a pointer, but care is taken that only one mutable reference is ever created at a time.

Do you have recommendations for how to go from a mutable pointer to a mutable reference? Do you recommend replacing all pointers with UnsafeCell?

[Urgau]

[...] but I don't think their usage creates UB. I don't believe that my usage creates UB either. In both cases, a mutable reference is created from a pointer, but care is taken that only one mutable reference is ever created at a time.

Except that the mutable reference was created from a read-only pointer (the shared reference). Making the creation of the mutable reference immediately UB, no matter if it's used or not. See this example with Miri on playground.

Do you have recommendations for how to go from a mutable pointer to a mutable reference? Do you recommend replacing all pointers with UnsafeCell?

Well I'm not familiar with this code, but from a quick look I'm wondering why the self._parent isn't a mutable reference, since the only place that use CTmpMutIterator is ...

rusty_spine/src/c_interface.rs

Lines 612 to 621 in abf7844

	#[must_use]
	pub fn $rust_mut(
	&mut self,
	) -> crate::c_interface::CTmpMutIterator<$parent_type, $type, $c_type> {
	crate::c_interface::CTmpMutIterator::new(
	self,
	unsafe { self.c_ptr_ref().$c },
	self.$count_fn() as usize,
	)
	}

... which takes a mutable reference, as far as I can tell.

As for a general recommendation, I cannot really give one as I'm not that prolifically with unsafe code, but I would say stick with raw pointers when you have a doubt, if you can satisfy the all the aliasing requirements use RefCell/UnsafeCell. BUT in any case I would highly recommend adding a # SAFETY section to each unsafe block and running your code under Miri (as part of CI if possible or manually instead) so to catch most UB.

[jabuwu]

Thanks for your help @Urgau!

This problem is easily solved by simply adding the necessary muts, so I guess the lint is working as intended.

The c2rust issue is coming from a const cast.

*(&(*bone).b as *const c_float as *mut c_float) *= s;

needs to become:

*(&mut (*bone).b as *const c_float as *mut c_float) *= s;

[jabuwu]

I released 0.6.1 with fixes. It's still broken in beta but working in nightly. afaik the issue is that beta hasn't been built with rust-lang/rust#112431 yet, which improved the lint.

[Urgau]

This problem is easily solved by simply adding the necessary muts, so I guess the lint is working as intended.

❤️ Thanks, that makes it all worth while.

It's still broken in beta but working in nightly. afaik the issue is that beta hasn't been built with rust-lang/rust#112431 yet, which improved the lint.

Well it's no really that is "improves" stuff but that it temporarily doesn't lint on things that requires an aliasing/memory model, ie Stack Borrow or Tree Borrow, since neither of them are official.

The c2rust issue is coming from a const cast.

*(&(*bone).b as *const c_float as *mut c_float) *= s;

needs to become:

*(&mut (*bone).b as *const c_float as *mut c_float) *= s;

I should just note that this is still UB per Miri since Stack Borrow consider intermediate cast as relevant and casting &mut -> *const makes a read-only ptr.

error: Undefined Behavior: attempting a write access using <1559> at alloc852[0x0], but that tag only grants SharedReadOnly permission for this location
  --> src/main.rs:10:14
   |
10 |     unsafe { *(&mut (*bone).a as *const c_float as *mut c_float) = 1.2f32 };
   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |              |
   |              attempting a write access using <1559> at alloc852[0x0], but that tag only grants SharedReadOnly permission for this location
   |              this error occurs as part of an access at alloc852[0x0..0x4]
   |
   = 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: <1559> was created by a SharedReadOnly retag at offsets [0x0..0x4]
  --> src/main.rs:10:16
   |
10 |     unsafe { *(&mut (*bone).a as *const c_float as *mut c_float) = 1.2f32 };
   |                ^^^^^^^^^^^^^^
   = note: BACKTRACE (of the first span):
   = note: inside `main` at src/main.rs:10:14: 10:74

I would highly recommend just removing those unnecessary and potentially future UB cast.

- *(&mut (*bone).b as *const c_float as *mut c_float) *= s;
+ *(&mut (*bone).b as *mut c_float) *= s;

EDIT: The as *mut is also unnecessary and the complete expression could just be: *&mut (*bone).a = ...

[jabuwu]

Thanks again @Urgau. The c2rust code is probably a landmine of UB, unfortunately.

[jabuwu]

This has been fixed with the release of rustc beta 1.73 and rusty_spine 0.6.1, and I've made UB fixes that should hopefully appease future rust lints.

There is some work to be done to automate these UB fixes when generating the rust code from spine-c. The UB actually originates from non-performance related UB in the spine-c runtimes (specifically, const casts). It might warrant a PR for spine-c, but since it doesn't appear to be an issue in practice, it's hard to justify the change.

[virtualritz]

It might warrant a PR for spine-c, [...]

Why not just open a ticket and link this issue to create awareness with the spine-c peeps for now?

[jabuwu]

Why not just open a ticket and link this issue to create awareness with the spine-c peeps for now?

I plan to, at some point. I don't really know what to say. In all my experience writing C, this is a purely theoretical problem. If someone has a more compelling argument, it would be helpful.