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

Pass 'raw' options through ssl:listen/1 #824

Merged
merged 2 commits into from
Dec 16, 2015

Conversation

rlipscombe
Copy link
Contributor

In Erlang R16B03-1, I've been passing raw options to ssl:listen as
follows, and it's been working fine:

% The constants are defined elsewhere.
LOpts = [{raw, ?IPPROTO_TCP, ?TCP_MAXSEG, <<MSS:32/native>>} | ...],
{ok, LSocket} = ssl:listen(0, LOpts)

In Erlang 17.3, this fails with
{option_not_a_key_value_tuple,{raw,6,2,<<64,2,0,0>>}}

I originally reported this in
http://erlang.org/pipermail/erlang-questions/2014-October/081226.html

I need to pass this particular raw option to ssl:listen, because it needs to be applied when the socket is first opened -- between inet:open and prim_inet:listen -- it cannot be applied later by setopts. This means that it needs to be done by inet_tcp:listen/2 -- well, actually by inet:open/8, but...

Otherwise it's racey -- a client could connect between prim_inet:listen and the setopts call. The MSS option is advertised in the SYN,ACK packet, and can't be changed later.

@OTP-Maintainer
Copy link

Patch has passed first testings and has been assigned to be reviewed


I am a script, I am not human


@IngelaAndin
Copy link
Contributor

I do not remember seeing your question, however it is easy to sometimes miss a thread
as erlang-question have a lot of questions. The commit that check for the input list is a proplist was added to improve error-handling of options, as we had some subtle next_protocol bad behavior due to missing tuple markings, and not to disallow raw inet options. Alas the inet driver options do not completely conform to proplists, so there are some exceptions that need to be handled in the ssl code. So I think your patch is ok, but we would like a better commit message, explaining why it is needed.

@rlipscombe
Copy link
Contributor Author

I've updated the commit message. Let me know if you need more details.

@IngelaAndin
Copy link
Contributor

Humm are you sure you pushed the new commit message? I can not see it!

@rlipscombe
Copy link
Contributor Author

Er, my bad. I mean I updated the PR comment. I'll update the commit tomorrow. Sorry.

In Erlang R16B03-1, I've been passing raw options to ssl:listen as
follows, and it's been working fine:

% The constants are defined elsewhere.
LOpts = [{raw, ?IPPROTO_TCP, ?TCP_MAXSEG, <<MSS:32/native>>} | ...],
{ok, LSocket} = ssl:listen(0, LOpts)
In Erlang 17.3, this fails with
{option_not_a_key_value_tuple,{raw,6,2,<<64,2,0,0>>}}

I originally reported this in
http://erlang.org/pipermail/erlang-questions/2014-October/081226.html

I need to pass this particular raw option to ssl:listen, because it
needs to be applied when the socket is first opened -- between inet:open
and prim_inet:listen -- it cannot be applied later by setopts. This
means that it needs to be done by inet_tcp:listen/2 -- well, actually by
inet:open/8, but...

Otherwise it's racey -- a client could connect between prim_inet:listen
and the setopts call. The MSS option is advertised in the SYN,ACK
packet, and can't be changed later.
@rlipscombe
Copy link
Contributor Author

I've copied the details to the commit itself.

@OTP-Maintainer
Copy link

Patch has passed first testings and has been assigned to be reviewed


I am a script, I am not human


@IngelaAndin
Copy link
Contributor

Looks good :) By the way it would be great if you could add a test case also that uses the raw-option,
so that no one breaks it in the future.

@proxyles
Copy link
Contributor

any progress on that tests case?

@rlipscombe
Copy link
Contributor Author

any progress on that tests case?

Unfortunately, no. I started looking at it, and then got snowed under with more important work-related stuff.

A couple of problems:

  1. Since the raw option numbers are platform-specific, what's the recommended way to only run the test on recognised platforms (i.e. Linux only, I guess)?
  2. I hadn't figured out a good way to actually assert that it worked. I guess we could just settle for "nothing crashed", but that doesn't protect against it inadvertently being dropped in a future revision. It felt wrong to introduce a mock, just for this case, even if I could figure out a way to do that cleanly (I guess taking a dependency on meck would be frowned upon).

When I'm a little less busy, I'll try to get the test cases finished up.

@IngelaAndin
Copy link
Contributor

You can test for platform in all init_suite/group/testcase functions and skip test if the requirements are
not fulfilled. There are other test in ssl that do that kind of thing.

Maybe you could read the option back with ssl:getopts that will fallback to inet:getopts for non "ssl" otpions. Make sure not to set the default value.

@rlipscombe
Copy link
Contributor Author

I've added a test; it uses TCP_KEEPALIVE, because TCP_MAXSEG can't be read back reliably.

@OTP-Maintainer
Copy link

The summary line of the commit message is too long and/or ends with a "."
Make sure the whole message follows the guidelines here: https://github.com/erlang/otp/wiki/Writing-good-commit-messages.

Bad message: Ensure that a single 'raw' option is passed to ssl:listen correctly


I am a script, I am not human


Add a test to ensure that a single 'raw' option can be passed to
ssl:listen correctly.

Note: multiple raw options are (incorrectly) handled by
inet:listen_options. See
http://erlang.org/pipermail/erlang-questions/2014-March/078371.html
@proxyles proxyles merged commit 3dd86e7 into erlang:maint Dec 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants