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

Fix smtp starttls #13048

Closed
wants to merge 1 commit into from
Closed

Fix smtp starttls #13048

wants to merge 1 commit into from

Conversation

Flakebi
Copy link
Contributor

@Flakebi Flakebi commented Mar 5, 2024

In cases where the connection was fast, curl sometimes failed to open a connection.
This fixes a regression of c2d9736.

The regression triggered in these steps:

  1. Create an smtp connection

  2. Use STARTTLS

  3. Receive the response

  4. We are inside the loop in smtp_statemachine, calling smtp_state_starttls_resp

  5. In the good flow, we exit the loop, re-enter smtp_statemachine and run smtp_perform_upgrade_tls at the start of the function.

    In the bad flow, we stay in the while loop, calling Curl_pp_readresp, which reads part of the TLS handshake and things go wrong.

The reason is that Curl_pp_moredata changed behavior and always returns true, so we stay in the loop in smtp_statemachine. With a slow connection Curl_pp_readresp cannot read new data and returns CURL_AGAIN, so we leave the loop and re-enter smtp_statemachine.

With a fast connection, Curl_pp_readresp reads new data from the tcp connection, which is part of the TLS handshake.

The fix is in Curl_pp_moredata, which needs to take the final line into account and return false if only the final line is stored.

A test would be nice, but I’m not accustomed to the testing system, so I there’s no test in this change.

In cases where the connection was fast, curl sometimes failed to open a
connection.
This fixes a regression of c2d9736.

The regression triggered in these steps:
1. Create an smtp connection
2. Use STARTTLS
3. Receive the response
4. We are inside the loop in `smtp_statemachine`, calling `smtp_state_starttls_resp`
5. In the good flow, we exit the loop, re-enter `smtp_statemachine` and
   run `smtp_perform_upgrade_tls` at the start of the function.

   In the bad flow, we stay in the while loop, calling `Curl_pp_readresp`,
   which reads part of the TLS handshake and things go wrong.

The reason is that `Curl_pp_moredata` changed behavior and always
returns `true`, so we stay in the loop in `smtp_statemachine`. With a
slow connection `Curl_pp_readresp` cannot read new data and returns
`CURL_AGAIN`, so we leave the loop and re-enter `smtp_statemachine`.

With a fast connection, `Curl_pp_readresp` reads new data from the tcp
connection, which is part of the TLS handshake.

The fix is in `Curl_pp_moredata`, which needs to take the final line
into account and return `false` if only the final line is stored.
@bagder
Copy link
Member

bagder commented Mar 5, 2024

Thanks!

@Flakebi Flakebi deleted the fix-smtps branch March 5, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants