Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make pipelineIds unguessable #578

Closed
wants to merge 1 commit into from

Conversation

@cbrewster
Copy link
Member

cbrewster commented Nov 18, 2016

Needed for servo/servo#10542.
We need to get webrender updated in Servo before this can be ready along with servo/servo#14283.


This change is Reviewable

@cbrewster cbrewster mentioned this pull request Nov 18, 2016
4 of 5 tasks complete
@glennw
Copy link
Member

glennw commented Nov 21, 2016

This will need to update the sample/ and replay/ applications in this repository in order to pass CI - you can do that by running cargo build in each of those directories. Once that's done, I'm fine with this change. Let's not merge it until the accompanying Servo changes are reviewed though - to minimize the time when WR master is incompatible with Servo master.

@cbrewster cbrewster force-pushed the cbrewster:unguessable_pipelineid branch from 6e3a11a to 68f063e Nov 22, 2016
@kvark
kvark approved these changes Nov 24, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Nov 28, 2016

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

@cbrewster cbrewster force-pushed the cbrewster:unguessable_pipelineid branch from 68f063e to cd89e62 Dec 14, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Dec 24, 2016

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

@glennw
Copy link
Member

glennw commented Jan 8, 2017

Is this still in progress?

@cbrewster
Copy link
Member Author

cbrewster commented Jan 9, 2017

@glennw yup! the blocking changes just landed the other day. Should have this rebased soon

@cbrewster cbrewster force-pushed the cbrewster:unguessable_pipelineid branch from cd89e62 to f636a17 Jan 10, 2017
@cbrewster cbrewster force-pushed the cbrewster:unguessable_pipelineid branch from f636a17 to bcaec80 Jan 10, 2017
@cbrewster
Copy link
Member Author

cbrewster commented Jan 10, 2017

Rebased. Waiting on review for servo/servo#14283

@bors-servo
Copy link
Contributor

bors-servo commented Jan 12, 2017

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

@jrmuizel
Copy link
Contributor

jrmuizel commented Jan 19, 2017

So Gecko does not need this kind of thing because different processes do not share channels. What's the reason that Servo shares channels? It seems like sharing channels is asking for security trouble.

@cbrewster
Copy link
Member Author

cbrewster commented Jan 19, 2017

@jrmuizel the issue tracking this reasoning for this change is servo/servo#10542. Separating out the channels was one option; however, we opted for the UUID approach as its much simpler.

cc @asajeffrey

@jrmuizel
Copy link
Contributor

jrmuizel commented Jan 25, 2017

Wouldn't it be simpler to not mix everything together instead of mixing all the messages together and then depending on clients to make it so they can't be spoofed? I'd rather webrender/gecko not take on the complexity of having uuids because of servo.

@pcwalton
Copy link
Collaborator

pcwalton commented Jan 25, 2017

Well, we have out of process iframes in Servo, so we have potentially a lot more processes and channels. So the calculus may be different here than in Gecko, assuming that Gecko uses some sort of UUIDs at the subprocess level (does it?)

It might be worth doing some measurements to see whether using lots of channels actually hurts. I worry about the specter of FD exhaustion…

@glennw
Copy link
Member

glennw commented Jan 25, 2017

One option could be to just make the PipelineId in WR an arbitrary 16 byte field. Servo can use a UUID, and Gecko could use whatever arbitrary identifier it chooses to?

@jrmuizel
Copy link
Contributor

jrmuizel commented Jan 26, 2017

So the calculus may be different here than in Gecko, assuming that Gecko uses some sort of UUIDs at the subprocess level (does it?)

Gecko just has a hash table per file descriptor that it's listening on and maps ids that it receives on from that fd to objects. This means that there's no way to spoof ids because they don't share a namespace.

@glennw
Copy link
Member

glennw commented Feb 19, 2017

@cbrewster Should we keep this open or close for now? It seems like we probably won't end up having a uuid crate dependency in WR?

@cbrewster
Copy link
Member Author

cbrewster commented Feb 20, 2017

We can close this for now

@cbrewster cbrewster closed this Feb 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.