Mask body of runOps to avoid a heap corruption #89
Conversation
| -- 1. We allocate an OpContext, e.g., OpRecvMessageContext and the corresponding ByteBuffer. | ||
| -- 2. We pass the buffer to gRPC in startBatch. | ||
| -- 3. If we now get an exception we will free the ByteBuffer. | ||
| -- 4. gRPC can now end up writing to the freed ByteBuffer and we get a heap corruption. |
There was a problem hiding this comment.
Where would gRPC write to the freed ByteBuffer? The reason I ask is that I don't see opArray used outside of startBatch
There was a problem hiding this comment.
startBatch is asynchronous. The actual write happens somewhere in the gRPC internals. I haven’t tracked it down to the exact location.
There was a problem hiding this comment.
I took a brief look and it looks like at least call->receiving_buffer retains a reference to the buffer https://github.com/grpc/grpc/blob/master/src/core/lib/surface/call.cc#L1804
Probably also worth pointing out that this fix is based on an actual issue that we encountered in our tests.
|
I can confirm that this patch fixes a segfault that we started to get on an internal project after upgrading to grpc-1.22. |
|
@cocreature: Thank you! |
|
Update: this branch works for our project but the |
|
@neongreen I’m confused, this branch is identical to the current master now that the PR is merged. |
|
I believe it's not. $ git diff cocreature/fix-heap-corruption origin/master
diff --git a/core/src/Network/GRPC/LowLevel/Op.hs b/core/src/Network/GRPC/LowLevel/Op.hs
index ed8cd8e..bde3997 100644
--- a/core/src/Network/GRPC/LowLevel/Op.hs
+++ b/core/src/Network/GRPC/LowLevel/Op.hs
@@ -108,8 +108,8 @@ freeOpContext (OpSendInitialMetadataContext m _) = C.metadataFree m
freeOpContext (OpSendMessageContext (bb, s)) =
C.grpcByteBufferDestroy bb >> C.freeSlice s
freeOpContext OpSendCloseFromClientContext = return ()
-freeOpContext (OpSendStatusFromServerContext metadata _ _ _) =
- C.metadataFree metadata
+freeOpContext (OpSendStatusFromServerContext metadata _ _ s) =
+ C.metadataFree metadata >> C.freeSlice s
freeOpContext (OpRecvInitialMetadataContext metadata) =
C.metadataArrayDestroy metadata
freeOpContext (OpRecvMessageContext pbb) = |
|
Latest commits in this branch: Latest commits in master: |
|
I'm starting to suspect that this branch is a red herring and the only reason it "fixed" our segfault is that our segfault is caused by #90, which happened not to be present in this branch. I will see if I can share the code. |
* change: expose `Proto3.Wire.Reverse.Internal` * change: export `smallChunkSize`, `metaDataAlign`, and `metaDataSize` * change: export sealBuffer * change: export BuildRState * change: export `readTotal`, `readState`, `writeState`, `readSpace`, and `writeSpace`
No description provided.