Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(221)

Issue 8852044: code review 8852044: crypto/tls: ignore empty TLS records. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 5 months ago by agl1
Modified:
9 years, 4 months ago
Reviewers:
albert.strasheim
CC:
golang-dev, r, minux1
Visibility:
Public.

Description

crypto/tls: ignore empty TLS records. OpenSSL can be configured to send empty records in order to randomise the CBC IV. This is an early version of 1/n-1 record splitting (that Go does) and is quite reasonable, but it results in tls.Conn.Read returning (0, nil). This change ignores up to 100 consecutive, empty records to avoid returning (0, nil) to callers. Fixes 5309.

Patch Set 1 #

Patch Set 2 : diff -r 13e00572ed0b https://code.google.com/p/go/ #

Total comments: 6

Patch Set 3 : diff -r 13e00572ed0b https://code.google.com/p/go/ #

Patch Set 4 : diff -r 7e0e60679942 https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+617 lines, -12 lines) Patch
M src/pkg/crypto/tls/conn.go View 1 2 1 chunk +23 lines, -12 lines 0 comments Download
M src/pkg/crypto/tls/handshake_client_test.go View 1 2 2 chunks +594 lines, -0 lines 0 comments Download

Messages

Total messages: 12
agl1
It's unclear whether this is suitable for Go 1.1. It is a bug fix, but ...
9 years, 5 months ago (2013-04-19 18:05:42 UTC) #1
r
worth testing? will consider for go 1.1 https://codereview.appspot.com/8852044/diff/2001/src/pkg/crypto/tls/conn.go File src/pkg/crypto/tls/conn.go (right): https://codereview.appspot.com/8852044/diff/2001/src/pkg/crypto/tls/conn.go#newcode795 src/pkg/crypto/tls/conn.go:795: // Some ...
9 years, 5 months ago (2013-04-19 18:13:50 UTC) #2
agl1
I thought about testing, but the tests seemed to be more cost than gain. Having ...
9 years, 5 months ago (2013-04-19 18:45:07 UTC) #3
minux1
please update description: s/four/100/
9 years, 5 months ago (2013-04-19 18:46:10 UTC) #4
agl1
On Fri, Apr 19, 2013 at 2:46 PM, <minux.ma@gmail.com> wrote: > please update description: > ...
9 years, 5 months ago (2013-04-19 18:56:15 UTC) #5
agl1
post Go-1.1 poke. Cheers AGL
9 years, 4 months ago (2013-05-14 21:43:59 UTC) #6
r
LGTM
9 years, 4 months ago (2013-05-14 22:05:53 UTC) #7
agl1
Hello golang-dev@googlegroups.com, r@golang.org, minux.ma@gmail.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
9 years, 4 months ago (2013-05-15 14:25:52 UTC) #8
agl1
*** Submitted as https://code.google.com/p/go/source/detail?r=b37d2fdcc4d9 *** crypto/tls: ignore empty TLS records. OpenSSL can be configured to ...
9 years, 4 months ago (2013-05-15 14:26:07 UTC) #9
albert.strasheim
Strangely, this test is failing for me. build.golang.org is all green, of course. go version ...
9 years, 4 months ago (2013-05-16 08:38:19 UTC) #10
agl1
On Thu, May 16, 2013 at 4:38 AM, <fullung@gmail.com> wrote: > Strangely, this test is ...
9 years, 4 months ago (2013-05-16 16:23:32 UTC) #11
agl1
9 years, 4 months ago (2013-05-16 16:29:57 UTC) #12
On Thu, May 16, 2013 at 12:23 PM, Adam Langley <agl@golang.org> wrote:
> I had a pair of defer statements in the wrong order.
> https://codereview.appspot.com/9187047 pending to fix it.

Submitted. Please let me know if there's still any issues.


Cheers

AGL
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b