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

TLSSocket send/recv return WOULD_BLOCK error instead of NO_CONNECTION #9584

Merged
merged 1 commit into from Feb 12, 2019

Conversation

Projects
None yet
6 participants
@michalpasztamobica
Copy link
Contributor

michalpasztamobica commented Feb 1, 2019

Description

In case mbedtls handshake advertising MBEDTLS_ERR_SSL_WANT_READ or MBEDTLS_ERR_SSL_WANT_WRITE is not really a critical error and should be recoverable. We improve the accurateness of the return value from send/recv function by returning NSAPI_ERROR_WOULD_BLOCK instead of NSAPI_ERROR_NO_CONNECTION in such case.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@SeppoTakalo
@kjbracey-arm

@ciarmcom ciarmcom requested review from SeppoTakalo and ARMmbed/mbed-os-maintainers Feb 1, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Feb 1, 2019

@cmonr

cmonr approved these changes Feb 2, 2019

@SeppoTakalo
Copy link
Contributor

SeppoTakalo left a comment

I don't see why you are changing this?

When calling Socket::connect() in non blocking mode:

  1. First call returns NSAPI_ERROR_IN_PROGRESS which means that asyncronous operation started.
  2. All subsequent calls return NSAPI_ERROR_ALREADY until connection is reached.
  3. When connected, returns NSAPI_ERROR_IS_CONNECTED

There is no WOULD_BLOCK in Socket::connect()

What was the problem you are fixing?

features/netsocket/TLSSocketWrapper.cpp Outdated
@@ -197,9 +197,6 @@ nsapi_error_t TLSSocketWrapper::start_handshake(bool first_call)

ret = continue_handshake();
if (first_call) {
if (ret == NSAPI_ERROR_ALREADY) {
ret = NSAPI_ERROR_IN_PROGRESS; // If first call should return IN_PROGRESS

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Feb 4, 2019

Contributor

This sounds like you are changing the API.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:tlssocket_would_block branch 4 times, most recently Feb 6, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

michalpasztamobica commented Feb 6, 2019

@SeppoTakalo , thank you for your alertness, I overinterpreted my task. There should be no API change, indeed. The PR was only meant to address the return type of continue_handshake. Now I see that connect's behavior should remain unchanged. I amended the PR, I hope it's ok now.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:tlssocket_would_block branch 2 times, most recently Feb 6, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

michalpasztamobica commented Feb 6, 2019

With the current code the status is following:

  • continue_handshake returns NSAPI_ERROR_WOULD_BLOCK in case the WANT_READ or WANT_WRITE are returned from mbedtls_ssl_handshake().
  • In connect() call this will result in NSAPI_ERROR_IN_PROGRESS returned, as was the case so far
  • However the recv() and send() will now return NSAPI_ERROR_WOULD_BLOCK if they are in non-blocking mode, instead of NSAPI_ERROR_NO_CONNECTION. which was not exactly adequate in this context.

@SeppoTakalo , please confirm that we have the common understanding of the situation.
@kjbracey-arm , would you please share your thoughts on this?

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

SeppoTakalo commented Feb 6, 2019

Not exactly,

continue_handshake() returns:

  • NSAPI_ERROR_ALREADY if mbed TLS returned MBEDTLS_ERR_SSL_WANT_READ/WRITE
  • NSAPI_ERROR_AUTH_FAILURE on any failure
  • NSAPI_ERROR_IS_CONNECTED if connection was established.

This is called from connect(), which might translate calls, and return:

  • NSAPI_ERROR_IN_PROGRESS in case this was the first call to connect() and continue_handshake() returned NSAPI_ERROR_ALREADY
  • NSAPI_ERROR_OK in case this is first call, and continue_handshake() returned NSAPI_ERROR_IS_CONNECTED.

Same kind of translation might happen in send()/recv() calls, where they might call continue_handshake() in case TLS handshake was not completed. In those case it might return:

  • NSAPI_ERROR_NO_CONNECTION if continue_handshake() returns NSAPI_ERROR_ALREADY. (This can be changed, probably, not have not been tested).
  • NSAPI_ERROR_AUTH_FAILURE on any failure
    And in case of NSAPI_ERROR_IS_CONNECTED is returned, it continues to read/write.
TLSSocket returns WOULD_BLOCK error instead of ALREADY
In case mbedtls fails to execute handshake advertising
MBEDTLS_ERR_SSL_WANT_READ or MBEDTLS_ERR_SSL_WANT_WRITE,
TLSSocketWrapper::continue_handshake returns NSAPI_ERROR_WOULD_BLOCK.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:tlssocket_would_block branch to 9db9724 Feb 7, 2019

@michalpasztamobica michalpasztamobica changed the title TLSSocket returns WOULD_BLOCK error instead of ALREADY TLSSocket send/recv return WOULD_BLOCK error instead of NO_CONNECTION Feb 7, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

michalpasztamobica commented Feb 8, 2019

I have run an internal CI job against the mbed-client-testapp and all tests passed fine. @SeppoTakalo , do you have more ideas on what we could check before merging this into master?
It looks safe and reasonable to me :)

@SeppoTakalo
Copy link
Contributor

SeppoTakalo left a comment

Think this looks OK now

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

michalpasztamobica commented Feb 11, 2019

@0xc0170 , this PR is already approved. Could you change the label, please?

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Feb 11, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Feb 11, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 1
Build artifacts

@cmonr cmonr added ready for merge and removed needs: CI labels Feb 11, 2019

@cmonr cmonr merged commit 92e1464 into ARMmbed:master Feb 12, 2019

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+12 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9354 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Feb 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.