Consider removing usage of `unsafe`

[jbg]

I was reading over the source code before considering using this, and was a bit surprised to find usage of unsafe in a relatively high-level, security-sensitive application like this. Removing them would IMO be worth considering because it makes the code much easier to audit, and ongoing maintenance risk is lower (even if all use of unsafe is sound now, you need to ensure it remains sound in future changes).

unsafe is used in three places:

1. copy_timeout_x11

This uses libc::fork() and libc::kill() to create and destroy a process (running the same executable image, since there is no exec()) to clear the clipboard.

Why not use std::process to fork off a subprocess to do that? fork() without exec() can be wildly unsafe in large programs; every library in use should be audited to ensure it is safe to continue using after a fork(), and the dependency tree is too big to realistically do that on an ongoing basis:

$ cargo tree | wc -l
491

2. u8_as_utf16

This is called by parse_output for the gnupg_bin crypto backend, with the comment:

/// Command output formatting might not always be consistent. This function tries to parse both as
/// UTF-8 and UTF-16.

In what situations would the command output be UTF-16 rather than UTF-8? Could it be another charset? (Why not?) Why not use an existing crate to handle UTF-16 decoding?

The unsafe block in u8_as_utf16 assumes the &[u8] is properly aligned to be interpreted as a &[u16], which is not guaranteed. It also assumes native byte order, which (if I've understood the context that this code is used in correctly) would be problematic if a repository is shared between systems of different endianness.

3. tests, verifying zero-on-drop

This seems perfectly reasonable — I don't know of a way to do this in safe Rust, but usage of unsafe could at least be confined to tests.

[timvisee]

Hi there. Thanks a lot for going through this! I'll take a better look into this at a later time. I'm not feeling well right now.

I do have some quick comments on this though. I did not go through the code for this so excuse me if I'm wrong.

1. Sadly, handling X11 clipboard operations is hard. Basically, to nicely handle this from the command line prs either has to fork, or it has to use an external clipboard manager. I don't want to depend on external clipboard managers, which is why forking is chosen. You're right that this may have unwanted side effects. I did do my best to eliminate these however.

2. GnuPGs output is weird. It uses different encodings depending on many things. I was unaware the parsing check used unsafe code. Thanks for noting this. It should definitely be doable with safe code.

3. For this, unsafe is confined to these tests right? With the above two points as only exception.

[timvisee]

Minor update: the second unsafe usage has been removed in 22180b1cef4a65fc29b2c53405fd618c48742b1d

The first remains until a better method is chosen.

[timvisee]

I've put work into removing all unsafe usages throughout prs.

The clipboard handling system has been fully rewritten to achieve this, which took quite a lot of work. This version with all unsafe removed can be found on this branch. I'll do some further testing before merging for release.

That means that when this is merged, prs is unsafe-free! Sorry this took some time.

(two usages of unsafe remain in the test suite, but that isn't super important)

[timvisee]

Great news! v0.5.0 is now released which all unsafe usages removed!

In all places where this did exist, a safer alternative is implemented.

(two usages of unsafe remain in the test suite, but that is not important)

[Pantamis]

The clipboard works much better now wow !

I had issues to paste into anything else than a terminal, now it works everywhere !

Great job ! :+1:

[timvisee]

@Pantamis Fantastic! Good to hear.

I had issues to paste into anything else than a terminal, now it works everywhere !

That's weird.

I know it's fixed now, but what OS and desktop environment are you using? And if on Linux, are you running X11 or Wayland?

[Pantamis]

I use Ubuntu 22.04 with X11 and XFCE !