protect raylet against bad messages

[zhijunfu]

This is the PR for issue #3915. This fixes the raylet crashes caused by malformed messages.

This change updates node manager so that when it discovers a new node manager, it first sends a ConnectClient message to it with its client ID, before sending any other messages, so that the remote node manager knows that further messages from this connection come from a valid source.

This change then modifies client_connection, so that when receiving a malformed message (with incorrect protocol version), we'll check if it's from a valid source that has previously sent us a ConnectClient message, if it is then it's likely to be a real bug, otherwise we can know it's not from a legitimate source, and we just print a message and don't further process this connection.

[zhijunfu]

@pcmoritz @robertnishihara @raulchen

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/11722/
Test PASSed.

[pcmoritz]

It would be slightly clearer if we just have CheckProtocolVersion() here (not returning anything), and then inside of CheckProtocolVersion we call ServerConnection<T>::Close(); where the comment says "and stop processing the connection".

[zhijunfu]

I think we need to skip the rest of this function if CheckProtocolVersion fails, thus it still needs to return a value.

[pcmoritz]

Thanks, I left some comments.

Let's also change the ray_protocol_version to 0x5241590000000000 to contain some magic characters at the beginning (hex for "RAY" in this case), and add a comment about it. Rationale for this is that it could still be possible that some random program sends an int64_t which is zero at the beginning (zeros are quite common), but it is much less likely that a program sends this particular number. There is still plenty of space to encode the protocol version in the lower bits.

[robertnishihara]

@pcmoritz I don't think we should rely on ray_protocol_version for anything because we could easily get rid of it in the future.

[pcmoritz]

If we get rid of it in the future, we need to find a different mechanism to do it then. We need a magic cookie to send before the raylet tries to interpret the next int64_t as a size of the next message (and potentially crash).

[robertnishihara]

Why don’t we do that in this PR because I think it’s very likely well remove the protocol version and forget to do this.

…

On Sat, Feb 9, 2019 at 2:37 PM Philipp Moritz ***@***.***> wrote: If we get rid of it in the future, we need a different mechanism to do this. We need a magic cookie to send before the raylet tries to interpret the next int64_t as a size of the next message (and potentially crash). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4003 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPOrZMDBmgdQUYO0_rDO4mG6DYzkAQVks5vL025gaJpZM4aye67> .

[pcmoritz]

Let's just rename the protocol_version to ray_cookie.

[zhijunfu]

updated according to comments, and also renamed ray_protocol_version to ray_cookie

[AmplabJenkins]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/11738/
Test FAILed.

[pcmoritz]

Thanks, I added a python end-to-end test and fixed the linting.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/11750/
Test PASSed.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/11759/
Test PASSed.

[guoyuhong]

The Travis failure is not related to this PR. I will merge it.