UB in SPI and Serial code

[TheZoq2]

@repnop pointed out to me that https://github.com/stm32-rs/stm32f1xx-hal/blob/master/src/spi.rs#L261 and the write_volatilecall in serial.rs is UB because constructing (or possibly writing) from a pointer created from a non-mut reference is UB.

A fix might be to just change it to

unsafe { ptr::write_volatile(&mut self.spi.dr as *const _ as *mut FrameSize, data) }

But spi.dr does not implement DerefMut so this fix does not compile. I don't have time to look into it deeper than that today but we should be aware of this potential UB.

Edit: Removed a reference to an unrelated issue
Edit2: Removed the svd2rust part as well

[chorman0773]

I'd be careful with references and volatile (I came here from the issue you, presumably accidentally, linked). As brought up in rust-lang/unsafe-code-guidelines#265, the compiler can invent reads to them at any time, for any or no reason. The raw_ref_macros may solve that issue though.

[thalesfragoso]

All registers are wrappers around VolatileCell, which is a wrapper around UnsafeCell, doesn't that prevent the UB ?

[repnop]

@thalesfragoso for more information about that, see the linked thread in the comment above yours. The tl;dr is more or less: that for MMIO, it is unsound since the compiler is allowed to add spurious reads (as long as they don't cause data races) for &UnsafeCell since in the LLVM IR references are always marked as dereferencable, which could have side effects unlike normal memory reads. This currently works because rustc isn't inserting these, but may in the future. There's more discussion on how to solve this problem in the linked issue, but this specific bug is unrelated to that and when Zoq made it I didn't adequately make that clear since some others and I were discussing the aforementioned problem as well as this one at the same time 😅

[thalesfragoso]

I don't get it, the spurious read from MMIO is indeed a problem (although I don't think is UB) that exists everywhere in the ecosystem, not just in the code linked in this issue. However, I think the issue in discussion here isn't that, but the writing to a memory location derived from an immutable reference, no ?

[repnop]

yes, which is what I said in the latter part of my comment

[thalesfragoso]

Oh, ok. My comment was addressing this issue btw, not the MMIO one.

Edit: Hmm, Reg from svd2rust isn't repr(transparent), so this might be a problem...

[repnop]

Oh, sorry, I misunderstood then. Going from &T to *const T to *mut T and then writing through it is most definitely undefined behavior, some references:

rust-lang/rust-clippy#4774
rust-lang/unsafe-code-guidelines#257
rust-lang/rfcs#2582

[thalesfragoso]

Going from &T to *const T to *mut T and then writing through it is most definitely undefined behavior

With the exception of UnsafeCell. In fact, the UnsafeCell::get() method is exactly that. That's why I mentioned it up there.

[repnop]

Ah, I gotcha now, it wasn't immediately obvious there was UnsafeCell involved since all of that code is autogenerated and hard to find the actual struct definitions. Guess I was mistaken then, sorry about that! 😅