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.

Sign up for GitHub

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

Jump to bottom

Possible race sending headers from server while receiving message over size limit #1972

Closed
dfawley opened this issue Apr 6, 2018 · 4 comments · Fixed by containerd/containerd#3192, docker/cli#1811, moby/moby#39014 or moby/swarmkit#2847
Closed

Possible race sending headers from server while receiving message over size limit #1972

dfawley opened this issue Apr 6, 2018 · 4 comments · Fixed by containerd/containerd#3192, docker/cli#1811, moby/moby#39014 or moby/swarmkit#2847
Assignees
@MakMukhi
Labels
P2 Type: Bug

Comments

@dfawley
Copy link
Member

dfawley commented Apr 6, 2018

This scenario, while unlikely, could lead to two concurrent calls to WriteHeader, which is not safe due to the unlocked checking and setting of the headerOK variable.
@a-robinson
Copy link

CockroachDB recently started using grpc-go v1.12.0 and have almost immediately seen this race happen in our race tests (cockroachdb/cockroach#25641, cockroachdb/cockroach#25643). d0a21a3 appears to have introduced it, although it seems as though you're already aware.

Do you have a particular fix in mind beyond just re-adding the mutex or using an atomic compare-and-swap?

a-robinson added a commit to a-robinson/grpc-go that referenced this issue May 18, 2018
@a-robinson
transport: fix data race on Stream.headerOk
a044e96 
Fixes grpc#1972
a-robinson added a commit to a-robinson/grpc-go that referenced this issue May 18, 2018
@a-robinson
transport: fix data race on Stream.headerOk
6e230b0 
Fixes grpc#1972
@a-robinson a-robinson mentioned this issue May 18, 2018
Closed
@a-robinson
Copy link

I've sent out #2090 as a possible fix, but I'm not at all attached to it if you guys have other plans.

@MakMukhi
Copy link
Contributor

Hey @a-robinson, sorry about the regression. I've created a fix for this which is in review. I should've linked it here. I'll have our team prioritize its review.

Thanks for starting out #2090 , but unfortunately the problem is a bit more nuanced.
Since, reader goroutine can WriteStatus in parallel to WriteHeader happening in writer goroutine it can lead to multiple problems:

  1. Race on headerOk and header metadata as you know.
  2. Potentially sending header after status.

Adding the lock back doesn't have much performance impact(~500ns on 120us that it takes for a unary RPC) since it's a non contended lock.

@MakMukhi MakMukhi mentioned this issue May 18, 2018
Merged
@a-robinson
Copy link

Great to hear, thanks @MakMukhi!

@lock lock bot locked as resolved and limited conversation to collaborators Nov 18, 2018
thaJeztah added a commit to thaJeztah/docker that referenced this issue Apr 6, 2019
@thaJeztah
bump google.golang.org/grpc v1.12.2
6f572c8 
full diff: grpc/grpc-go@v1.12.0...v1.12.2

- grpc/grpc-go#2074 transport/server: fix race between writing status and header
  - fix grpc/grpc-go#1972 Possible race sending headers from server while receiving message over size limit
- grpc/grpc-go#2074 transport: account for user configured small io write buffer
  - fix grpc/grpc-go#2089 Server abruptly terminates connections if write buffer is small enough

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/cli that referenced this issue Apr 6, 2019
@thaJeztah
bump google.golang.org/grpc v1.12.2
fd2d7f4 
full diff: grpc/grpc-go@v1.12.0...v1.12.2

- grpc/grpc-go#2074 transport/server: fix race between writing status and header
  - fix grpc/grpc-go#1972 Possible race sending headers from server while receiving message over size limit
- grpc/grpc-go#2074 transport: account for user configured small io write buffer
  - fix grpc/grpc-go#2089 Server abruptly terminates connections if write buffer is small enough

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/swarmkit that referenced this issue Apr 6, 2019
@thaJeztah
bump google.golang.org/grpc v1.12.2
c3a1567 
full diff: grpc/grpc-go@v1.12.0...v1.12.2

- grpc/grpc-go#2074 transport/server: fix race between writing status and header
  - fix grpc/grpc-go#1972 Possible race sending headers from server while receiving message over size limit
- grpc/grpc-go#2074 transport: account for user configured small io write buffer
  - fix grpc/grpc-go#2089 Server abruptly terminates connections if write buffer is small enough

Signed-off-by: Sebastiaan van Stijn <[email protected]>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this issue Apr 8, 2019
@thaJeztah
bump google.golang.org/grpc v1.12.2
7e42ac7 
full diff: grpc/grpc-go@v1.12.0...v1.12.2

- grpc/grpc-go#2074 transport/server: fix race between writing status and header
  - fix grpc/grpc-go#1972 Possible race sending headers from server while receiving message over size limit
- grpc/grpc-go#2074 transport: account for user configured small io write buffer
  - fix grpc/grpc-go#2089 Server abruptly terminates connections if write buffer is small enough

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Upstream-commit: 6f572c8154c20253474a20decc87f677feb3321e
Component: engine
adhulipa pushed a commit to adhulipa/docker that referenced this issue Apr 11, 2019
@thaJeztah @adhulipa
bump google.golang.org/grpc v1.12.2
e019c19 
full diff: grpc/grpc-go@v1.12.0...v1.12.2

- grpc/grpc-go#2074 transport/server: fix race between writing status and header
  - fix grpc/grpc-go#1972 Possible race sending headers from server while receiving message over size limit
- grpc/grpc-go#2074 transport: account for user configured small io write buffer
  - fix grpc/grpc-go#2089 Server abruptly terminates connections if write buffer is small enough

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/cli that referenced this issue Apr 12, 2019
@thaJeztah
bump google.golang.org/grpc v1.12.2
25e6a64 
full diff: grpc/grpc-go@v1.12.0...v1.12.2

- grpc/grpc-go#2074 transport/server: fix race between writing status and header
  - fix grpc/grpc-go#1972 Possible race sending headers from server while receiving message over size limit
- grpc/grpc-go#2074 transport: account for user configured small io write buffer
  - fix grpc/grpc-go#2089 Server abruptly terminates connections if write buffer is small enough

Signed-off-by: Sebastiaan van Stijn <[email protected]>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this issue Apr 18, 2019
@thaJeztah
bump google.golang.org/grpc v1.12.2
7e37a58 
full diff: grpc/grpc-go@v1.12.0...v1.12.2

- grpc/grpc-go#2074 transport/server: fix race between writing status and header
  - fix grpc/grpc-go#1972 Possible race sending headers from server while receiving message over size limit
- grpc/grpc-go#2074 transport: account for user configured small io write buffer
  - fix grpc/grpc-go#2089 Server abruptly terminates connections if write buffer is small enough

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Upstream-commit: 25e6a64e2a1792cb5ae5a74741f5d6f2f4531cf8
Component: cli
tiborvass pushed a commit to tiborvass/cli that referenced this issue Apr 23, 2019
@thaJeztah
bump google.golang.org/grpc v1.12.2
662f6a6 
full diff: grpc/grpc-go@v1.12.0...v1.12.2

- grpc/grpc-go#2074 transport/server: fix race between writing status and header
  - fix grpc/grpc-go#1972 Possible race sending headers from server while receiving message over size limit
- grpc/grpc-go#2074 transport: account for user configured small io write buffer
  - fix grpc/grpc-go#2089 Server abruptly terminates connections if write buffer is small enough

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/containerd that referenced this issue May 5, 2019
@thaJeztah
bump google.golang.org/grpc v1.12.2
d1c5951 
full diff: grpc/grpc-go@v1.12.0...v1.12.2

- grpc/grpc-go#2074 transport/server: fix race between writing status and header
  - fix grpc/grpc-go#1972 Possible race sending headers from server while receiving message over size limit
- grpc/grpc-go#2074 transport: account for user configured small io write buffer
  - fix grpc/grpc-go#2089 Server abruptly terminates connections if write buffer is small enough

Signed-off-by: Sebastiaan van Stijn <[email protected]>
kiku-jw pushed a commit to kiku-jw/moby that referenced this issue May 16, 2019
@thaJeztah @kiku-jw
bump google.golang.org/grpc v1.12.2
239b700 
full diff: grpc/grpc-go@v1.12.0...v1.12.2

- grpc/grpc-go#2074 transport/server: fix race between writing status and header
  - fix grpc/grpc-go#1972 Possible race sending headers from server while receiving message over size limit
- grpc/grpc-go#2074 transport: account for user configured small io write buffer
  - fix grpc/grpc-go#2089 Server abruptly terminates connections if write buffer is small enough

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/docker that referenced this issue Aug 27, 2019
@thaJeztah
bump google.golang.org/grpc v1.12.2
e6f02e4 
full diff: grpc/grpc-go@v1.12.0...v1.12.2

- grpc/grpc-go#2074 transport/server: fix race between writing status and header
  - fix grpc/grpc-go#1972 Possible race sending headers from server while receiving message over size limit
- grpc/grpc-go#2074 transport: account for user configured small io write buffer
  - fix grpc/grpc-go#2089 Server abruptly terminates connections if write buffer is small enough

Signed-off-by: Sebastiaan van Stijn <[email protected]>
(cherry picked from commit 6f572c8)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
tussennet pushed a commit to tussennet/containerd that referenced this issue Sep 11, 2020
@thaJeztah @87654testtesttesttesttesttest
bump google.golang.org/grpc v1.12.2
386a547 
full diff: grpc/grpc-go@v1.12.0...v1.12.2

- grpc/grpc-go#2074 transport/server: fix race between writing status and header
  - fix grpc/grpc-go#1972 Possible race sending headers from server while receiving message over size limit
- grpc/grpc-go#2074 transport: account for user configured small io write buffer
  - fix grpc/grpc-go#2089 Server abruptly terminates connections if write buffer is small enough

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@MakMukhi MakMukhi

Labels
P2 Type: Bug
Projects
None yet
Milestone
No milestone
3 participants
@a-robinson @MakMukhi @dfawley