Different handling of mode socket parameter between OTP 23.2.6 and 23.2.7

[kleinernik]

Describe the bug
When a TCP socket is upgraded with ssl:connect pre OTP 23.2.7 the mode (binary or list) of the TCP port was preserved. Now the setting in the options given to ssl:connect is used.

The bug is caused by the following implementation: In

otp/lib/ssl/src/ssl.erl

Line 581 in 961bde8

try handle_options(SslOptions0 ++ SocketValues, client) of

the sockets options of the underlying tcp sockets get appended to the ssl_connect options. So if the underlying socket has a different mode than in ssl_connect options, it comes later in the list. It seems to me like the last is used. Now the new implementation 961bde8#diff-c6e4e01fbddfaa6057fc43144940a337eb3db17891c62cce9ad2123e0107f776R2712 happens to reverse the list, now the mode set in the ssl_connect options comes last and is used

To Reproduce
here is a minimal test script in elixir

tcp_opts = [mode: :binary, active: false, packet: :line, ip: {0,0,0,0}, port: 0]

ssl_opts = [mode: :list, active: false, depth: 0, packet: :line, ip: {0,0,0,0}, port: 0]

Application.ensure_all_started :ssl
{:ok, ip} = '172.217.18.99' |> :inet.parse_address
{:ok, port} = :gen_tcp.connect ip, 443, tcp_opts
:inet.getopts(port, [:mode]) |> IO.inspect
:tls_socket.getopts(:gen_tcp, port, :tls_socket.emulated_options) |> IO.inspect
{:ok, sslsocket} = :ssl.connect port, ssl_opts
:inet.getopts(port, [:mode]) |> IO.inspect
:ssl.getopts(sslsocket, [:mode]) |> IO.inspect

With erlang 23.2.6 the last line prints {:ok, [mode: :binary]}and with erlang 23.2.7 it prints {:ok, [mode: :list]}

Expected behavior
The behavoiur should not change between minor versions.

Affected versions
23.2.7

Additional context
See gen-smtp/gen_smtp#250

[arjan]

961bde8#r47903311 is probably the fix

[IngelaAndin]

Obviously it was not the intent to change the behavior in a minor release only to restore the behavior of the legacy option log_alert and extend log_level to accept the values none and all. There might be reasons to add lists:reverse to behave the same, however this error report seems confusing to me. If you set the ssl-options to something else than the TCP socket I would expect them to be used!? However if you have set TCP options that are not default and set no socket options in ssl:connect I would expect to keep the TCP options set instead of getting the default, which I believe is the case.

[kleinernik]

I agree, the current (23.2.7) behaviour is how it should behave. But some widespread libraries like gen_smtp and allegedly ranch (at least it was mentioned in #4586 ) rely on the old behaviour and this breaks productions environments for some users (like it happened to us and others gen-smtp/gen_smtp#250). So maybe going back to the old behaviour in OTP 23 and switching to the more logical one in OTP 24 with a warning in the changelog would be better?

[IngelaAndin]

Normally we will no be bug compatible, but we can make an exception for this particular case as the fix of the bug was a side effect of fixing something else and nobody has reported the bug and we where unaware of it too. Which I think is due to it being a quite unusual use case and probably also a reason why we have missed to create a test case for it. So in this particular case it probably cause more problems than it solves to stick a 100 % to our principal. We will restore the old behavior for OTP-23.3 and fix the bug with a test case for OTP-24.

[IngelaAndin]

@kleinernik Hi I have been working on the fix of the options for OTP-24 and I realized that the fix should be done differently than how the bug was happened to be fixed by my other patch. So I have a question, where the problems you have seen due to the socket options or was it perhaps other ssl options? I pushed a solution, where I included the reverse of the list to keep the order of the provided ssloptions and still handling the socket options. https://github.com/IngelaAndin/otp/pull/new/ingela/ssl/GH-4585
Would this solution work for your case ? Or do we still need to have a bug-compatible solution for OTP-23?

[kleinernik]

@IngelaAndin Sorry, I can't really help. I just experienced the bug as a user or gen_smtp. There they expected messages to by binaries but somehow do not set the socket ops accordingly, which wasn't a problem up until the change. So maybe @arjan can say more about it?

[IngelaAndin]

@arjan It would be good to know if gen_smtp works correctly with this solution. We would rather not make bug-compatible patches if we can avoid it. But as stated earlier we might make an exception if circumstances are such that it would cause more harm than good to fix it right away.

[IngelaAndin]

I believe my PR request solves the issue for gen_smtp, and that no bug-compatibility is needed. It is merged now to make it for OTP-23.3. Feel free to reopen if any problem should persist.

[arjan]

Thank you! And sorry for not responding earlier. Great that this will make it in the next 23 release.

I agree that the way gen_smtp used the socket opts relied on undefined behaviour. On the gen_smtp (client) side this is also patched now by explicitly specifying binary in the upgrade.