Possible undefined behavior and other oddly looking code patterns

[dpc]

Please see:

https://lobste.rs/s/njfvjb/rustdesk_with_tailscale_on_arch_linux#c_5dyag0

I took a quick look and least the unsafe use seems unwarranted and plain UB, which might expose users to severe security risks.

[rustdesk]

Thanks. Yes, these unsafe sync are not warranted, we intended to do so, because those places are not needed to be strictly warranted.

And for 64-bit system, writing / reading a 64-bit number (aligned) is atomic.

Of course, changing to safe sync code will be more warranted and friendly. We will do.

[rustdesk]

We also welcome PR.

[kbknapp]

And for 64-bit system, writing / reading a 64-bit number (aligned) is atomic.

That doesn't account for the write making its way through the cache lines all the way back to main memory. While the write itself may be atomic in the processor (if the address is properly aligned), using real synchronization is still required in order to make sure the write makes it all way back to main memory to be observed correctly. Using real synchronization also invalidates the cache lines properly.

[kbknapp]

Here's a minimal playgound link where you can also see the UB using miri (click Tools->Miri)

[dpc]

RustDesk is a remote control software that people unaware of best practices will be installing on their personal machines often exposing it to the public Internet. From many types of software project this one is particularly security sensitive, as bugs can easily translate into people getting harmed financially or even worse ways.

Looks like you are trying to commercialize it, so your responsibility of providing a secure software are even greater (possibly even legally liable, IANAL).

Unless you have some solid explanation why these unsafe blocks are safe (in which case it would be best to put reference to in the code, right next to each unsafe use), you should absolutely not be using unsafe. AFAICT these are straight UBs which can lead to all sorts of weird unexpected results, possibly removing security checks, or otherwise leading to remote code execution vulnerabilities.

Please do some more research on the matter. My advice: get rid of all unsafe blocks, use atomics if atomic behavior is what you want and never use unsafe again.

[rustdesk]

Thanks all, 1142cf1

[ltguillaume]

Why is this closed @rustdesk? Your commit 1142f1 only addresses a fraction of the issues listed in/under https://lobste.rs/s/njfvjb/rustdesk_with_tailscale_on_arch_linux E.g. what about the use of awk/cat wrappers?