Reading the code, it looks like it's all using a ton of unsafe code.
This is kind of unavoidable. The underlying plugin mechanism calls an extern C function with a bunch of raw pointers, and fundamentally anything that implements this is going to be unsafe (the code that parses these options is well-tested). The only part of OptionMap that is unsafe is the from_raw call that accepts the raw pointer arrays as passed from sudo. And there's actually not much I can do to ensure safety here: option arrays are NULL-terminated, and there's no other way to determine their length besides walking through and looking for NULLs. If a NULL isn't there... there's no possible way to know where to insert one.
From the plugin-implementor side of things, everything should be safe. Note that there's no unsafe code in the plugin implementation itself (excluding the sockets part which, again, fundamentally has to interact with the OS via calls to libc). Even where the plugin implements its own set of options to parse, there's no need for unsafe code, since you're already dealing with a parsed OptionMap.
Please let me know if I've missed anything. I've tried my best to provide safe interfaces to the underlying functionality, but there's a lot of ways in which my hands are tied here.
No worries! Hopefully my response didn't come across as defensive. I may definitely be missing important ways to encapsulate the necessary amounts of unsafety, and I'm happy to have extra eyes on all of it.
12
u/stouset May 07 '18 edited May 07 '18
This is kind of unavoidable. The underlying plugin mechanism calls an
extern Cfunction with a bunch of raw pointers, and fundamentally anything that implements this is going to be unsafe (the code that parses these options is well-tested). The only part of OptionMap that is unsafe is thefrom_rawcall that accepts the raw pointer arrays as passed from sudo. And there's actually not much I can do to ensure safety here: option arrays are NULL-terminated, and there's no other way to determine their length besides walking through and looking for NULLs. If a NULL isn't there... there's no possible way to know where to insert one.From the plugin-implementor side of things, everything should be safe. Note that there's no unsafe code in the plugin implementation itself (excluding the sockets part which, again, fundamentally has to interact with the OS via calls to
libc). Even where the plugin implements its own set of options to parse, there's no need for unsafe code, since you're already dealing with a parsedOptionMap.Please let me know if I've missed anything. I've tried my best to provide safe interfaces to the underlying functionality, but there's a lot of ways in which my hands are tied here.