Rust crate approval

[Adam Gashlin]

I'm in the process of writing my first Rust for Firefox, a standalone
Windows service to be used for background updates. I've found a few good
documents on how to handle the build technically, but I'm unclear on what
process we use to review external crates. If there are general guidelines
for external libraries in any language I'd appreciate pointers to that as
well.

More specifically:

* Already vendored crates
Can I assume any crates we have already in mozilla-central are ok to use?
Last year there was a thread that mentioned making a list of "sanctioned"
crates, did that ever come about?

* Updates
I need winapi 0.3.5 for BITS support, currently third_party/rust/winapi is
0.3.4. There should be no problem updating it, but should I have this
reviewed by the folks who originally vendored it into mozilla-central?

* New crates
I'd like to use the windows-service crate, which seems well written and has
few dependencies, but the first 0.1.0 release was just a few weeks ago. I'd
like to have that reviewed at least as carefully as my own code,
particularly given how much unsafety there is, but where do I draw the
line? For instance, it depends on "widestring", which is small and has been
around for a while but isn't widely used, should I have that reviewed
internally as well? Is popularity a reasonable measure?

Thanks!
-Adam Gashlin

[Bobby Holley]

Hi Adam,

At present, I think you should raise your questions with Nathan Froyd and
Ehsan Akhgari, who are the owners of the C++/Rust usage module [1].

There has been some discussion around creating a Rust-in-Firefox Advisory
Committee to handle questions like this, but it hasn't happened yet. In the
mean time, I'm comfortable saying that the organization fully trusts Ehsan
and Nathan to make intelligent decisions in this area, delegate to
subject-area experts as appropriate, and consult the FTLM for any larger
policy questions.

Cheers,
bholley

[1]
https://wiki.mozilla.org/Modules/Core#C.2B.2B.2FRust_usage.2C_tools.2C_and_style

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[Simon Sapin]

On 27/06/18 19:45, Bobby Holley wrote:
> Hi Adam,
>
> At present, I think you should raise your questions with Nathan Froyd and
> Ehsan Akhgari, who are the owners of the C++/Rust usage module [1].
>
> There has been some discussion around creating a Rust-in-Firefox Advisory
> Committee to handle questions like this, but it hasn't happened yet. In the
> mean time, I'm comfortable saying that the organization fully trusts Ehsan
> and Nathan to make intelligent decisions in this area, delegate to
> subject-area experts as appropriate, and consult the FTLM for any larger
> policy questions.
>
> Cheers,
> bholley

It sounded to me that the question was not about Rust specifically
(despite the subject line), but rather about licensing and code quality
concerns around vendoring and using third-party code. In another
instance where the language would be C++, do we have a policy for
importing code like this?

(I’m not at all questioning Ehsan’s and Nathan’s position to make this
call anyway.)

--
Simon Sapin

[Bobby Holley]

On Wed, Jun 27, 2018 at 12:42 PM Simon Sapin <simon...@exyr.org> wrote:

> On 27/06/18 19:45, Bobby Holley wrote:
> > Hi Adam,
> >
> > At present, I think you should raise your questions with Nathan Froyd and
> > Ehsan Akhgari, who are the owners of the C++/Rust usage module [1].
> >
> > There has been some discussion around creating a Rust-in-Firefox Advisory
> > Committee to handle questions like this, but it hasn't happened yet. In
> the
> > mean time, I'm comfortable saying that the organization fully trusts
> Ehsan
> > and Nathan to make intelligent decisions in this area, delegate to
> > subject-area experts as appropriate, and consult the FTLM for any larger
> > policy questions.
> >
> > Cheers,
> > bholley
>
>
> It sounded to me that the question was not about Rust specifically
> (despite the subject line), but rather about licensing and code quality
> concerns around vendoring and using third-party code. In another
> instance where the language would be C++, do we have a policy for
> importing code like this?
>

I think the same probably applies (the module is specifically for both C++
and Rust usage). Some questions may be big or thorny enough to pull in
other folks, but I think Ehsan and Nathan are a good place to start.

>
> (I’m not at all questioning Ehsan’s and Nathan’s position to make this
> call anyway.)
>
> --
> Simon Sapin

[Nathan Froyd]

Thanks for raising these points.

On Tue, Jun 26, 2018 at 10:02 PM, Adam Gashlin <agas...@mozilla.com> wrote:
> * Already vendored crates
> Can I assume any crates we have already in mozilla-central are ok to use?
> Last year there was a thread that mentioned making a list of "sanctioned"
> crates, did that ever come about?

I don't recall the discussion on sanctioned crates, do you have a
pointer to that thread?

Regardless, anything that's already vendored should be OK.

> * Updates
> I need winapi 0.3.5 for BITS support, currently third_party/rust/winapi is
> 0.3.4. There should be no problem updating it, but should I have this
> reviewed by the folks who originally vendored it into mozilla-central?

While we can accommodate multiple versions of crates in-tree, we would
prefer that only one version of a given crate is vendored into the
tree at any one time, but sometimes this is an impractical goal to
achieve. So if upgrading whatever uses winapi 0.3.4 to use 0.3.5
instead is reasonable, please do that first. If it turns out to be
impractical, go ahead and vendor the duplicate crate.

For review concerns, see below.

> * New crates
> I'd like to use the windows-service crate, which seems well written and has
> few dependencies, but the first 0.1.0 release was just a few weeks ago. I'd
> like to have that reviewed at least as carefully as my own code,
> particularly given how much unsafety there is, but where do I draw the
> line? For instance, it depends on "widestring", which is small and has been
> around for a while but isn't widely used, should I have that reviewed
> internally as well? Is popularity a reasonable measure?

Our normal review process is all that we have used so far; I think
thus far we have assumed that Rust's safety guarantees enable us to
forego a more stringent review process that has sometimes been used
for (some) C/C++ code. (e.g. I think modules/brotli underwent some
amount of scrutiny, whereas mfbt/double-conversion was a more
rubber-stamp sort of import.) This is probably not a tenable
long-term position, especially given how easy it is to pull in Rust
code vs. a C/C++ library.

We have generally trusted people to use good judgement in what they
use and how much review is required. Accordingly, I think you should
request review from the people who would normally review your code,
and if you have concerns about specific crates that are being
vendored, you should call those crates out as needing especial review.
If you or your reviewers think such reviews fall outside of your
comfort zone/area of expertise/Rust capabilities, please flag myself
or Ehsan, and we will work on finding people to help.

Thanks,
-Nathan

[Adam Gashlin]

On Thu, Jun 28, 2018 at 4:42 PM, Nathan Froyd <nfr...@mozilla.com> wrote:

> Thanks for raising these points.
>

Thanks for the response!

On Tue, Jun 26, 2018 at 10:02 PM, Adam Gashlin <agas...@mozilla.com> wrote:
> > * Already vendored crates
> > Can I assume any crates we have already in mozilla-central are ok to use?
> > Last year there was a thread that mentioned making a list of "sanctioned"
> > crates, did that ever come about?
>
> I don't recall the discussion on sanctioned crates, do you have a
> pointer to that thread?
>

It turns out what I was thinking of was just a brief suggestion here:
https://groups.google.com/d/msg/mozilla.dev.platform/a_vhnvoM3co/yxwzqOkUBgAJ

> Regardless, anything that's already vendored should be OK
>

That's generally been my assumption, but a number of things have been
vendored that may not have been reviewed for possible inclusion in shipping
code. I was wondering what winapi was doing in the tree (as it is essential
for the Windows-specific stuff I'm doing), it seems to have been pulled in
for geckodriver.

I'll use judgment, as recommended :)

-Adam

[Tom Ritter]

On Thu, Jun 28, 2018 at 11:42 PM, Nathan Froyd <nfr...@mozilla.com> wrote:

> We have generally trusted people to use good judgement in what they
> use and how much review is required. Accordingly, I think you should
> request review from the people who would normally review your code,
> and if you have concerns about specific crates that are being
> vendored, you should call those crates out as needing especial review.
> If you or your reviewers think such reviews fall outside of your
> comfort zone/area of expertise/Rust capabilities, please flag myself
> or Ehsan, and we will work on finding people to help.
>

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...)

-tom

[Lars Bergstrom]

​

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.)​? Sort of in the same
way that (I assume) you are checking for prohibited licenses in the
Cargo.toml.

[Eric Rescorla]

On Sat, Jun 30, 2018 at 9:35 AM, Lars Bergstrom <lars...@mozilla.com>
wrote:

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?

-Ekr

[Xidorn Quan]

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.

- Xidorn

[Eric Rescorla]

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

[Bobby Holley]

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.

[Lars Bergstrom]

Cool! I was just worried, especially since hyper and tokio/mio are
*already* vendored in-tree (though openssl is not yet). It appears they're
used for network access in some test tools and an audio ipc server
prototype. So we're not talking about a bunch of code "appearing" in
third-party, but rather people knowing that they can use certain components
in one part of the tree (presumably test/non-product code?) and not in
another.

This topic also isn't entirely academic, as I was in a conversation with
some folks at the SF all hands unaware of the issues with using a different
(Rust) network & TLS stack in some new code they are writing in their
Firefox modules.
- Lars

[Ehsan Akhgari]

On Mon, Jul 2, 2018 at 4:05 PM, Lars Bergstrom <lars...@mozilla.com> wrote:

> Cool! I was just worried, especially since hyper and tokio/mio are
> *already* vendored in-tree (though openssl is not yet). It appears they're
> used for network access in some test tools and an audio ipc server
> prototype. So we're not talking about a bunch of code "appearing" in
> third-party, but rather people knowing that they can use certain components
> in one part of the tree (presumably test/non-product code?) and not in
> another.
>

Indeed. Since it probably won't be possible to ban such crates from being
included for non-product code, I think it's going to be difficult to
enforce rules around not using imported crates (or C++ libraries for that
matter) for things like network access at the build system level... I
think it's reasonable to expect reviewers to look for such issues.

--
Ehsan

[Henri Sivonen]

On Wed, Jun 27, 2018 at 5:02 AM, Adam Gashlin <agas...@mozilla.com> wrote:
> * Already vendored crates
> Can I assume any crates we have already in mozilla-central are ok to use?

Seems like a reasonable assumption.

> * Updates
> I need winapi 0.3.5 for BITS support, currently third_party/rust/winapi is
> 0.3.4. There should be no problem updating it, but should I have this
> reviewed by the folks who originally vendored it into mozilla-central?

In my opinion, it should be enough for _someone_ qualified to review
code in the area of Windows integration to review the diff.

> * New crates
> I'd like to use the windows-service crate, which seems well written and has
> few dependencies, but the first 0.1.0 release was just a few weeks ago. I'd
> like to have that reviewed at least as carefully as my own code,
> particularly given how much unsafety there is, but where do I draw the
> line? For instance, it depends on "widestring", which is small and has been
> around for a while but isn't widely used, should I have that reviewed
> internally as well? Is popularity a reasonable measure?

In principle, all code landing in m-c needs to be reviewed, but
sometimes the reviewer may rubber-stamp code instead of truly
reviewing it carefully. All the newly-vendored code should be part of
the review request and then it's up to the reviewer to decide if it's
appropriate to look at some code less carefully because there are
other indicators of quality.

As for widestring specifically, a cursory look at the code suggests
that it's a quality crate and should have no trouble passing review.
It is also small enough that it should be actually feasible to review
it instead of rubber-stamping it.

(For Mozilla-developed code that is on a performance-sensitive path,
there exists encoding_rs::mem (particularly
https://docs.rs/encoding_rs/0.8.4/encoding_rs/mem/fn.convert_str_to_utf16.html
and https://docs.rs/encoding_rs/0.8.4/encoding_rs/mem/fn.convert_utf16_to_str.html),
which doesn't provide the ergonomics that widestring provides but
provides faster (SIMD-accelerated on our tier-1 CPU architectures and
aarch64, which is on path to tier-1) conversions for long (16 code
units or longer) strings containing mostly ASCII code points. An
update service probably isn't performance-sensitive in this way. I'm
mentioning this to generate awareness generally on the topic of UTF-16
conversions in m-c Rust code.)

--
Henri Sivonen
hsiv...@mozilla.com

[Ted Mielczarek]

On Sun, Jul 1, 2018, at 7:56 PM, Xidorn Quan wrote:
> 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.

FYI, we had some discussion about the policy and mechanisms of reviewing vendored Rust crates in the recent past. I floated a strawman proposal[1] that didn't seem to upset anyone, but we got thrown off track by the Servo VCS sync needing to do auto-vendoring. AIUI, now that the pace of the stylo work has slowed, the Servo syncing is being done on a manual basis, so it seems like we could revisit that discussion.

The TL;DR on my proposal is: "We should make sure that someone has reviewed each new vendored crate in a bug separate from the one with the patch that adds code using it."

-Ted

1. https://bugzilla.mozilla.org/show_bug.cgi?id=1322798#c11