Allow 0 as a valid ret value for mbedtls_ssl_write #1006

andresag01 · 2017-07-11T13:42:59Z

This patch modifies the documentation for mbedtls_ssl_write() to allow
0 as a valid return value as this is the correct number of bytes that
should be returned when an empty TLS Application record is sent.

NOTES:

This fix is related to incorrect return value in ssl_write_real() #839
The change needs to be backported to mbed TLS 2.7 and 2.1.

gilles-peskine-arm

Ok, but I think we should add some comments inside ssl_tls.c as well. This patch was triggered by a proposed change to ssl_tls.c which we analyzed and rejected; we should document the relevant place in ssl_write_real to explain what's happening.

gilles-peskine-arm

Without modifying ssl_tls.c, this doesn't resolve the issue. We've concluded that the code shouldn't be modified, but we should add a comment to explain the current behavior.

andresag01 · 2017-09-28T13:43:12Z

@gilles-peskine-arm: I have added more documentation to ssl_write_real() as requested.

gilles-peskine-arm · 2017-11-27T18:57:17Z

Documentation-only change, so ok to merge despite the CI failed (and anyway all the CI steps suceeded??)

andresag01 · 2017-11-28T19:24:33Z

@gilles-peskine-arm: Indeed, I am not sure why the overall test reports a failure, but it seems that none of the individual pipeline stages failed.

hanno-becker · 2017-11-29T09:19:23Z

include/mbedtls/ssl.h

 *                 value or MBEDTLS_ERR_SSL_WANT_READ/WRITE, the ssl context
 *                 becomes unusable, and you should either free it or call
 *                 \c mbedtls_ssl_session_reset() on it before re-using it for
 *                 a new connection; the current connection must be closed.
 *
 * \note           When this function returns MBEDTLS_ERR_SSL_WANT_WRITE/READ,
 *                 it must be called later with the *same* arguments,
- *                 until it returns a positive value.
+ *                 until it returns a value greater that or equal to 0.


It should be greater than or here.

You mean that the sentence should be ... until it returns a value greater then or 0?

until it returns a value greater than or equal to 0

hanno-becker · 2017-11-29T09:21:11Z

library/ssl_tls.c

+        /*
+         * The user is trying to send a message the first time, so we need to
+         * copy the data into the internal buffers and setup the data structure
+         * to keep track of partial writes


Thanks for this additional documentation 👍

Nitpicking: There are full stops missing at the end of this and the previous documentation snippet.

I think I will leave it as it is. I think the reason for this is that the doxygen documentation should be well punctuated, etc because it will ultimately end up as a formal documentation website. However, these are only in code comments, if you look at the other comments like this, you will see that (1) the style is fairly inconsistent across the library and (2) a lot of them don't have a full stop at the end either.

Please correct it in case you add an additional commit for the other documentation fix - it's not a big thing and where we see such things we should correct them on the fly.

hanno-becker · 2017-11-29T09:42:48Z

I noticed we currently don't support request_size=0 in ssl_client2. I think it should be added at this occasion, together with a corresponding test in tests/ssl-opt.sh.

hanno-becker · 2017-11-29T09:50:41Z

@andresag01 While I would prefer the test changes to be done in this PR, I might open a separate PR for them to not block progress here. @gilles-peskine-arm What's your opinion on that?

hanno-becker

Left some comments on grammar in the documentation, but otherwise I'm fine with the changes. I've also read the code and ran a test to confirm that it is possible to send empty AD records.

Leaving it to @gilles-peskine-arm whether or not to correct the documentation, they are not blockers.

I think we should add tests for empty AD records, which would involve a small adaption to ssl_client2.c. I could do that here if desired, or defer it to a separate PR.

hanno-becker

I ran some more tests and noticed unexpected behavior in some cases that I need to investigate. Please don't merge, yet.

gilles-peskine-arm · 2017-11-29T15:17:04Z

Sending and receiving 0-sized application records is not well-tested, but documenting it well gives the impression that it's well-supported. Therefore, I'm holding off merging this for the time being.

Tracked internally in IOTSSL-1255

simonbutcher · 2018-03-02T18:39:31Z

LGTM.

simonbutcher · 2018-06-21T16:59:23Z

@andresag01 - Could you please rebase this PR and create backports?

This patch modifies the documentation for mbedtls_ssl_write() to allow 0 as a valid return value as this is the correct number of bytes that should be returned when an empty TLS Application record is sent.

andresag01 · 2018-06-21T18:40:30Z

@sbutcher-arm: I have rebased the PR and created the backports:

andresag01 added bug mbed TLS team labels Jul 11, 2017

andresag01 requested review from gilles-peskine-arm and hanno-becker July 11, 2017 13:43

andresag01 changed the title ~~Allow 0 as a valid ret value for mbedtls_ssl_write~~ DRAFT: Allow 0 as a valid ret value for mbedtls_ssl_write Jul 11, 2017

andresag01 force-pushed the iotssl-1255-ssl_write_real branch from c35b56d to c0f1ad4 Compare July 11, 2017 15:16

andresag01 changed the title ~~DRAFT: Allow 0 as a valid ret value for mbedtls_ssl_write~~ Allow 0 as a valid ret value for mbedtls_ssl_write Jul 11, 2017

gilles-peskine-arm reviewed Sep 27, 2017

View reviewed changes

gilles-peskine-arm requested changes Sep 27, 2017

View reviewed changes

gilles-peskine-arm approved these changes Nov 27, 2017

View reviewed changes

hanno-becker reviewed Nov 29, 2017

View reviewed changes

hanno-becker approved these changes Nov 29, 2017

View reviewed changes

hanno-becker reviewed Nov 29, 2017

View reviewed changes

simonbutcher added the approved for design label Mar 2, 2018

simonbutcher added approved Design and code approved - may be waiting for CI or backports needs-backports Backports are missing or are pending review and approval. and removed approved Design and code approved - may be waiting for CI or backports labels Jun 21, 2018

simonbutcher added the needs-work label Jun 21, 2018

Andres Amaya Garcia added 3 commits June 21, 2018 19:14

Add ChangeLog entry for mbedtls_ssl_write() docs

a562c26

Allow 0 as a valid ret value for mbedtls_ssl_write

7ee25d7

This patch modifies the documentation for mbedtls_ssl_write() to allow 0 as a valid return value as this is the correct number of bytes that should be returned when an empty TLS Application record is sent.

Document ssl_write_real() behaviour in detail

5b92352

andresag01 force-pushed the iotssl-1255-ssl_write_real branch from ed071d9 to 5b92352 Compare June 21, 2018 18:25

This was referenced Jun 21, 2018

Backport 2.7: Allow 0 as a valid ret value for mbedtls_ssl_write #1768

Merged

Backport 2.1: Allow 0 as a valid ret value for mbedtls_ssl_write #1769

Merged

andresag01 removed needs-backports Backports are missing or are pending review and approval. needs-work labels Jun 21, 2018

simonbutcher added the approved Design and code approved - may be waiting for CI or backports label Jun 22, 2018

This was referenced Jun 24, 2018

Is this a fatal error in ssl_write_real()? I'm not sure #1707

Closed

Fixed a problem with mbedtls_write_real() returning incorrect length #1726

Closed

simonbutcher merged commit 5b92352 into Mbed-TLS:development Jun 29, 2018

simonbutcher mentioned this pull request Jun 29, 2018

incorrect return value in ssl_write_real() #839

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow 0 as a valid ret value for mbedtls_ssl_write #1006

Allow 0 as a valid ret value for mbedtls_ssl_write #1006

andresag01 commented Jul 11, 2017 •

edited

Loading

gilles-peskine-arm left a comment

gilles-peskine-arm left a comment

andresag01 commented Sep 28, 2017

gilles-peskine-arm commented Nov 27, 2017

andresag01 commented Nov 28, 2017

hanno-becker Nov 29, 2017

andresag01 Nov 29, 2017

hanno-becker Nov 29, 2017

hanno-becker Nov 29, 2017

andresag01 Nov 29, 2017

hanno-becker Nov 29, 2017 •

edited

Loading

hanno-becker commented Nov 29, 2017

hanno-becker commented Nov 29, 2017 •

edited

Loading

hanno-becker left a comment

hanno-becker left a comment •

edited

Loading

gilles-peskine-arm commented Nov 29, 2017

simonbutcher commented Mar 2, 2018

simonbutcher commented Jun 21, 2018

andresag01 commented Jun 21, 2018

Allow 0 as a valid ret value for mbedtls_ssl_write #1006

Allow 0 as a valid ret value for mbedtls_ssl_write #1006

Conversation

andresag01 commented Jul 11, 2017 • edited Loading

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

andresag01 commented Sep 28, 2017

gilles-peskine-arm commented Nov 27, 2017

andresag01 commented Nov 28, 2017

hanno-becker Nov 29, 2017

Choose a reason for hiding this comment

andresag01 Nov 29, 2017

Choose a reason for hiding this comment

hanno-becker Nov 29, 2017

Choose a reason for hiding this comment

hanno-becker Nov 29, 2017

Choose a reason for hiding this comment

andresag01 Nov 29, 2017

Choose a reason for hiding this comment

hanno-becker Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

hanno-becker commented Nov 29, 2017

hanno-becker commented Nov 29, 2017 • edited Loading

hanno-becker left a comment

Choose a reason for hiding this comment

hanno-becker left a comment • edited Loading

Choose a reason for hiding this comment

gilles-peskine-arm commented Nov 29, 2017

simonbutcher commented Mar 2, 2018

simonbutcher commented Jun 21, 2018

andresag01 commented Jun 21, 2018

andresag01 commented Jul 11, 2017 •

edited

Loading

hanno-becker Nov 29, 2017 •

edited

Loading

hanno-becker commented Nov 29, 2017 •

edited

Loading

hanno-becker left a comment •

edited

Loading