Could you please submit a CLA? https://www.openssl.org/policies/cla.html

ok，I sent a successfully signed pdf email yesterday.

@t8m the CLA email always send fail， may I send other email address？
rcpt handle timeout,last handle info: Can not connect to opensslfoundation.org:185.231.103.75:25

@mattcaswell @kaduk Can you help review this PR?

@mattcaswell @kaduk Can you help review this PR?

Yes, I spent some time looking at this yesterday, but need to spend a bit more time on it before I provide comments.

I don't think the fix presented in this PR is the correct solution.

There are two problems being addressed.

1. A key update operation that is triggered while a partial record read has occurred fails when the remainder of the record is subsequently read.

The root cause of this problem is a bug in the function ssl3_setup_read_buffer(). This function may be called at times when the read buffer has already been set up. Notably this occurs at the start of the "init" phase (as occurs during a key update operation). In that case this function should "do nothing". One particular line in that function resets the value of the s->rlayer.packet pointer. This pointer is used to determine where the start of the read buffer is after alignment has been taken into account. ssl3_setup_read_buffer() doesn't correctly calculate this and just sets it to the start of the buffer regardless of alignment. This doesn't normally make a difference because it gets reset to the correct value when we start reading a new record in ssl3_read_n:

This goes wrong when we read a partial record, perform a key update operation (and hence end up calling ssl3_setup_read_buffer) and then continue the read operation afterwards.

The reset of the packet pointer in ssl3_setup_read_buffer is completely unnecessary, and simply removing it solves the problem:

Author:     Matt Caswell <[email protected]>
AuthorDate: Tue Jul 13 17:19:12 2021 +0100
Commit:     Matt Caswell <[email protected]>
CommitDate: Tue Jul 13 17:19:12 2021 +0100

    Don't reset the packet pointer in ssl3_setup_read_buffer
    
    Sometimes this function gets called when the buffers have already been
    set up. If there is already a partial packet in the read buffer then the
    packet pointer will be set to an incorrect value. The packet pointer already
    gets reset to the correct value when we first read a packet anyway, so we
    don't also need to do it in ssl3_setup_read_buffer.

diff --git a/ssl/record/ssl3_buffer.c b/ssl/record/ssl3_buffer.c
index 861610a08b..daa175d98c 100644
--- a/ssl/record/ssl3_buffer.c
+++ b/ssl/record/ssl3_buffer.c
@@ -73,7 +73,6 @@ int ssl3_setup_read_buffer(SSL *s)
         b->len = len;
     }
 
-    RECORD_LAYER_set_packet(&s->rlayer, &(b->buf[0]));
     return 1;
 }
 

2. The second problem being addressed is where SSL_write() is called and SSL_ERROR_WANT_WRITE is returned, before subsequently calling SSL_key_update().

IMO this is just incorrect API usage. If you get SSL_ERROR_WANT_WRITE returned from SSL_write() then you must not do any other write operations other than a repeat of the previous SSL_write() call.

This is what the docs say about it:

It is safe to call SSL_read() or SSL_read_ex() when more data is available
even when the call that set this error was an SSL_write() or SSL_write_ex().
However, if the call was an SSL_write() or SSL_write_ex(), it should be called
again to continue sending the application data.

I think the docs need to be updated to be even more explicit, i.e. if you get SSL_ERROR_WANT_WRITE from SSL_write then you should not do any other IO operation other than a repeat of the previous SSL_write call.

It would also be sensible to immediately fail if SSL_key_update is called when this is the case:

Author:     Matt Caswell <[email protected]>
AuthorDate: Tue Jul 13 17:44:44 2021 +0100
Commit:     Matt Caswell <[email protected]>
CommitDate: Tue Jul 13 17:44:44 2021 +0100

    Disallow SSL_key_update() if there are writes pending
    
    If an application is halfway through writing application data it should
    not be allowed to attempt an SSL_key_update() operation. Instead the
    SSL_write() operation should be completed.

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index c1e8e41f02..892a417d93 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -2262,6 +2262,11 @@ int SSL_key_update(SSL *s, int updatetype)
         return 0;
     }
 
+    if (RECORD_LAYER_write_pending(&s->rlayer)) {
+        ERR_raise(ERR_LIB_SSL, SSL_R_BAD_WRITE_RETRY);
+        return 0;
+    }
+
     ossl_statem_set_in_init(s, 1);
     s->key_update = updatetype;
     return 1;

With all of that said I think the tests presented in this PR are very useful and should be retained, with the exception of test_key_update_local_in_write which should be modified to confirm that calling SSL_key_update when a partial write has occurred produces an error as per my patch above.

@yangyangtiantianlonglong - would you be willing to update this PR to incorporate my suggested fixes above, remove yours, and update the tests as I suggest?

@mattcaswell Thank you for having to review it in detail. OK, I agree with the two points you made above, and I'll update the PR and test cases.
On the second point, I think the docs need to be updated to be even more explicit, i.e. if you get SSL_ERROR_WANT_WRITE from SSL_write then you should not do any other IO operation other than a repeat of the previous SSL_write call. Is this only limited to other write I/O operations? It is safe to call SSL_read() or SSL_read_ex().

@mattcaswell to make things easier, I'd suggest submitting your two fixes as a separate PR, which I'd approve immediately. After it is merged @yangyangtiantianlonglong can rebase this PR and remove the unneeded changes but keep the tests (with the necessary adjustment).

Is this only limited to other write I/O operations? It is safe to call SSL_read() or SSL_read_ex().

No, if you call SSL_write() and get SSL_ERROR_WANT_WRITE, then the next IO operation must be SSL_write().

@mattcaswell to make things easier, I'd suggest submitting your two fixes as a separate PR, which I'd approve immediately. After it is merged @yangyangtiantianlonglong can rebase this PR and remove the unneeded changes but keep the tests (with the necessary adjustment).

Yes, ok. I can do that.

Yes, ok. I can do that.

Raised as #16077

yes, After merge in #16077，I will rebase this PR, commit testcase and doc.

My operation problem caused this PR to be closed, and I have recreated it in #16085，already rebase #16077.