I think the key distinction here is that, unlike other rust-based projects (i.e. Servo), Firefox vendors all cargo dependencies into the tree, so it's more obvious to reviewers when a patch indirectly pulls in a new large dependency. In Servo the reviewer would have to look carefully at the Cargo.lock diff, where it's easier to miss.
This does mean, however, that reviewers should _not_ just rubber stamp any changes in third_party Rust. A line-by-line review may be too much to ask in some cases, but the reviewer should at least look at what's added and removed and make sure it's sane. On Sun, Jul 1, 2018 at 7:24 PM Eric Rescorla <e...@rtfm.com> wrote: > On Sun, Jul 1, 2018 at 4:56 PM, Xidorn Quan <m...@upsuper.org> wrote: > > > On Mon, Jul 2, 2018, at 9:03 AM, Eric Rescorla wrote: > > > On Sat, Jun 30, 2018 at 9:35 AM, Lars Bergstrom <larsb...@mozilla.com> > > > wrote: > > > > > > > On Fri, Jun 29, 2018 at 8:33 AM, Tom Ritter <t...@mozilla.com> wrote: > > > > > > > > > > > > > > I know that enumerating badness is never a comprehensive solution; > > but > > > > > maybe there could be a wiki page we could point people to for > things > > that > > > > > indicate something is doing something scary in Rust? This might > let > > us > > > > > crowd-source these reviews in a safer manner. For example, what > > would I > > > > > look for in a crate to see if it was: > > > > > - Adjusting memory permissions > > > > > - Reading/writing to disk > > > > > - Performing unsafe C/C++ pointer stuff > > > > > - Performing network connections of any type > > > > > - Calling out to syscalls or other kernel functions (especially > > > > win32k.sys > > > > > functions on Windows) > > > > > - (whatever else you can think of...) > > > > > <https://lists.mozilla.org/listinfo/dev-platform> > > > > > > > > > > > > > Building on that, is there a list of crates that should *never* be > > > > included in Firefox that you could scan for? Such as, anything that > is > > not > > > > nss (openssl bindings) or necko (use of a different network stack > that > > > > might not respect proxies, threading concerns, etc.)? > > > > > > Is this a crate-specific issue? Suppose that someone decided to land > > > a new C++ networking stack, that would presumably also be bad but > > > should be caught in code review, no? > > > > The point is that adding a new crate dependency is too easy accidentally, > > and it is very possible for reviewers to overlook that. So it may make > > sense to introduce a blacklist-ish thing to avoid that to happen. > > > > But in order for it to have an effect other than just cluttering up the > build, it would have to be wired into Firefox. And if reviewers don't > notice that, we have bigger problems. > > -Ekr > > - Xidorn > > _______________________________________________ > > dev-platform mailing list > > dev-platform@lists.mozilla.org > > https://lists.mozilla.org/listinfo/dev-platform > > > _______________________________________________ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform > _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
