Fix some minor record layer issues#16086
Conversation
Various comments referred to s->packet and s->packet_length instead of s->rlayer.packet and s->rlayer.packet_length. Also fixed is a spot where RECORD_LAYER_write_pending() should have been used. Based on the review comments in openssl#16077.
mattcaswell
added
approval: review pending
kaduk
left a comment
kaduk
left a comment
There was a problem hiding this comment.
Approved with punctuation fixes
| * thisrr->data by either the decryption or by the decompression When | ||
| * the data is 'copied' into the thisrr->data buffer, thisrr->input will | ||
| * be pointed at the new buffer | ||
| * ok, we can now read from 's->rlayer.packet' data into 'thisrr' |
There was a problem hiding this comment.
It feels like there should be a sentence break at the end of this line?
| * at rr->length bytes, which need to be copied into rr->data by either | ||
| * the decryption or by the decompression When the data is 'copied' into | ||
| * the rr->data buffer, rr->input will be pointed at the new buffer | ||
| * ok, we can now read from 's->rlayer.packet' data into 'rr' rr->input |
There was a problem hiding this comment.
Missing sentence breaks here as well.
And perhaps we should consider marking these comments with /*- to avoid re-wrapping (but there's not really much formatting other than the attempt to use end-of-line to replace full-stop).
| || !SSL_IS_TLS13(s)) | ||
| return 0; | ||
| s->ext.extra_tickets_expected++; | ||
| if (s->rlayer.wbuf[0].left == 0 && !SSL_in_init(s)) |
There was a problem hiding this comment.
(FWIW, I came up with this implementation by cribbing from the ssl3_write_bytes() implementation. I am not entirely sure that just looking at wbuf[0] is safe there, either (and it is checked for many things, not just the "extra tickets" logic).)
kaduk
added
approval: done
|
I think this should go to 1.1.1 as well (though the cherry-pick is not clean due to moving |
Er, I seem to have commented on the wrong issue. The |
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
Various comments referred to s->packet and s->packet_length instead of s->rlayer.packet and s->rlayer.packet_length. Also fixed is a spot where RECORD_LAYER_write_pending() should have been used. Based on the review comments in #16077. Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Ben Kaduk <[email protected]> (Merged from #16086)
|
Pushed to master with extra punctuation. Thanks! |
Various comments referred to s->packet and s->packet_length instead of s->rlayer.packet and s->rlayer.packet_length. Also fixed is a spot where RECORD_LAYER_write_pending() should have been used. Based on the review comments in openssl#16077. Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Ben Kaduk <[email protected]> (Merged from openssl#16086) (cherry picked from commit ca00152)
|
1.1.1 version in #16105 |
4 participants
Various comments referred to
s->packetands->packet_lengthinstead ofs->rlayer.packetands->rlayer.packet_length. Also fixed is a spot whereRECORD_LAYER_write_pending()should have been used. Based on the reviewcomments in #16077.