Audit Clap

[omarabid]

Clap: https://github.com/clap-rs/clap

There is a single unsafe usage according to Cargo Geiger.

[alex]

Looks like two instances of unsafe:

https://github.com/clap-rs/clap/blob/85f820fa04959be2acef9c831f98f1093c67f987/src/macros.rs#L470-L487

This seems fine, though it looks like an ad-hoc implementation of lazy_static!?

https://github.com/clap-rs/clap/blob/85f820fa04959be2acef9c831f98f1093c67f987/src/util/osstringext.rs#L23-L30

I think this is fine, although it's extremely not obvious -- I can barely tell what the desired behavior is.

[TyPR124]

In the first instance, could Box::into_raw be replaced with Box::leak to remove the need for unsafe pointer dereference?

[Lokathor]

yeah that whole thing can just be a Box::leak

[KillTheMule]

I think the latter function needs to be marked unsafe, since it does not check for wtf8 validity, see https://github.com/rust-lang/rust/blob/master/src/libstd/sys_common/wtf8.rs#L492.

[Shnatsel]

@KillTheMule nice find! I think this might cause some actual memory unsafety down the line, potentially turning into a privilege escalation vulnerability.

Could you file a bug against clap describing the issue?

[KillTheMule]

I‘ll do so in the next few days.

[Shnatsel]

I've opened a pull request to replace raw pointers with Box::leak(): clap-rs/clap#1593

[evanjs]

Okay, so it looks like we're going to be making these changes against master?
Wanted to clarify, what with the whole 3.0 rework and everything.

[Shnatsel]

This code is identical in 2.33 and master (future 3.0), so I'd start with master and then backport to 2.x series.

[evanjs]

Sounds good. Thanks!
Didn’t look too deeply, I just remember having issues with feature parity between the two so I wasn’t sure.

[KillTheMule]

Opened an issue. Seems a tad more complicated to solve properly, but pretty contained and doable.

[kbknapp]

For clarity, the PR is against master which will become v3.0. However, for things like this (removing unsafe), I'm also open to a PR against v2-master which could be put out as v2.34 until v3.0 is ready.

[brightly-salty]

Looks like this should be closed, as all related PRs and issues have been merged or closed?

[Shnatsel]

Indeed. Thanks!

I'll also happily accept a PR adding clap to the trophy case in README.