An in-memory RNG that shares its file descriptor. #14351

Merged
merged 2 commits into from Jan 5, 2017

Projects

None yet

6 participants

@asajeffrey
Member
asajeffrey commented Nov 23, 2016 edited

This PR implements an in-memory random number generator that only uses an OS RNG for (re)seeding. The OS RNG is shared, so there's only one file descriptor for /dev/urandom being used.

The PR also implements a tidy check that we don't accidentally introduce an RNG. Rather annoyingly, there are a lot of transitive dependencies on rand, notably hash maps in std.

This PR makes it possible to use uuids for identifiers such as pipeline and frame ids.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes do not require tests because it's fixing a resource issue

This change is Reviewable

@emilio emilio was assigned by highfive Nov 23, 2016
@highfive

Heads up! This PR modifies the following files:

  • @bholley: components/style/Cargo.toml
  • @wafflespeanut: python/tidy/servo_tidy/tidy.py
  • @kichjang: components/script/lib.rs, components/script/Cargo.toml, components/script/dom/crypto.rs, components/script/dom/htmlformelement.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/dom/serviceworkerglobalscope.rs
  • @fitzgen: components/script/lib.rs, components/script/Cargo.toml, components/script/dom/crypto.rs, components/script/dom/htmlformelement.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/dom/serviceworkerglobalscope.rs
  • @emilio: components/style/Cargo.toml
@highfive

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify style and script code, but no tests are modified. Please consider adding a test!
@asajeffrey
Member

cc @ConnorGBrewster @larsbergstrom @nox

@asajeffrey
Member

@avadacatavra this replaces the RNG in crypto. We should double-check that we are okay with that.

@emilio
Member
emilio commented Nov 23, 2016

FWIW, rayon uses rand but only for XorShiftRng, so probably we should be able to filter this in a better way?

@asajeffrey
Member

We don't scan dependency source, so it's difficult to see how we can do much better than a whitelist.

@nox
Member
nox commented Nov 24, 2016

That's quite a lot of code. I would rather we use the Fnv one explicitly when we see failures due to file limits with a RNG-related stack, than use a custom hasher.

@asajeffrey
Member

@nox: yes, we can use Fnv for hash maps, but that doesn't help for uuids or other cases where we need an RNG, not a hash function.

Most of the code here is just cut-and-paste from rand, in particular there's no new RNG algorithms, it just reuses existing ones. The only new bit is sharing an OsRng (and hence sharing an fd) rather than creating a new one for each thread.

@bors-servo
Contributor

☔️ The latest upstream changes (presumably #14381) made this pull request unmergeable. Please resolve the merge conflicts.

@bors-servo
Contributor

☔️ The latest upstream changes (presumably #14292) made this pull request unmergeable. Please resolve the merge conflicts.

@bors-servo
Contributor

☔️ The latest upstream changes (presumably #14473) made this pull request unmergeable. Please resolve the merge conflicts.

@asajeffrey asajeffrey referenced this pull request Dec 13, 2016
Closed

Use unguessable PipelineIds and FrameIds #14283

4 of 5 tasks complete
@bors-servo
Contributor

☔️ The latest upstream changes (presumably #14592) made this pull request unmergeable. Please resolve the merge conflicts.

@asajeffrey
Member

Rebased.

@bors-servo
Contributor

☔️ The latest upstream changes (presumably #14652) made this pull request unmergeable. Please resolve the merge conflicts.

@asajeffrey
Member

Rebased. @emilio or @nox: any chance of a review?

@bors-servo
Contributor

☔️ The latest upstream changes (presumably #14687) made this pull request unmergeable. Please resolve the merge conflicts.

@emilio
Member
emilio commented Dec 25, 2016

This is fine for me when rebased, but maybe @nox has still concerns.

python/tidy/servo_tidy/tidy.py
@@ -81,6 +81,24 @@
" accessible to\n// web pages."
]
+WHITELISTED_DEPENDENCIES = {
@Wafflespeanut
Wafflespeanut Jan 5, 2017 Member

We should really be moving this to the config file, but that shouldn't be a concern for this PR. I'll take care of it in an easy issue 😄

@asajeffrey
Member

IRC conversation with @nox: http://logs.glob.uno/?c=mozilla%23servo&s=5+Jan+2017&e=5+Jan+2017#c587629

TL;DR he r+s once it's split into two commits, and WHITELISTED_DEPENDENCIES is renamed.

asajeffrey added some commits Jan 5, 2017
@asajeffrey asajeffrey An in-memory RNG that shares its file descriptor. 7ace30f
@asajeffrey asajeffrey Added test-tidy check for blocked packages.
e3a8e3b
@asajeffrey
Member

@bors-servo r=emilio

@bors-servo
Contributor

📌 Commit e3a8e3b has been approved by emilio

@bors-servo
Contributor

⌛️ Testing commit e3a8e3b with merge 36ddf76...

@bors-servo bors-servo added a commit that referenced this pull request Jan 5, 2017
@bors-servo bors-servo Auto merge of #14351 - asajeffrey:servo-rand-share-fds, r=emilio
An in-memory RNG that shares its file descriptor.

<!-- Please describe your changes on the following line: -->

This PR implements an in-memory random number generator that only uses an OS RNG for (re)seeding. The OS RNG is shared, so there's only one file descriptor for `/dev/urandom` being used.

The PR also implements a tidy check that we don't accidentally introduce an RNG. Rather annoyingly, there are a lot of transitive dependencies on `rand`, notably hash maps in `std`.

This PR makes it possible to use uuids for identifiers such as pipeline and frame ids.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes do not require tests because it's fixing a resource issue

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14351)
<!-- Reviewable:end -->
36ddf76
@bors-servo bors-servo merged commit e3a8e3b into servo:master Jan 5, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment