Skip to content

Commit

Permalink
Rebase fix memory leak (#153)
Browse files Browse the repository at this point in the history
* Fix memory leak in op_send_initial_metadata

There is no corresponding `free` for this `malloc` call in
`op_send_initial_metadata`, as `op`s are not recursively `free`d.

Fix it by referring directly to the metadata instead of copying it, as the
metadata will remain alive for the lifetime of the `Op`, see
`withOpArrayAndCtxts`.

Source: #137

* Fix memory leak in op_send_message

There is no corresponding `grpc_byte_buffer_destroy` for this
`grpc_byte_buffer_copy` call in `op_send_message`, as `op`s are not recursively
`free`d.

Fix it by referring directly to the byte buffer instead of copying it, as the
byte buffer will remain alive for the lifetime of the `Op`, see
`withOpArrayAndCtxts`.

Source: #137

* Work around memory leak in `createOpContext`

According to valgrind, a `grpc_slice` created by `grpc_slice_malloc_` (called
via `C.grpcSliceMalloc` in the `OpRecvStatusOnClient` case of `createOpContext`)
is leaked:

```
72 bytes in 1 blocks are definitely lost in loss record 8 of 11
   at 0x484079B: malloc (in /nix/store/73if83n79h4ml8rcb473ym47zmb4vi5x-valgrind-3.19.0/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x4BD2ACD: gpr_malloc (in /nix/store/kfbhv9kid1dzblf16w9g1i10x48xkyrr-grpc-1.34.1/lib/libgpr.so.14.0.0)
   by 0x4AC3864: grpc_core::UnmanagedMemorySlice::HeapInit(unsigned long) (in /nix/store/kfbhv9kid1dzblf16w9g1i10x48xkyrr-grpc-1.34.1/lib/libgrpc.so.14.0.0)
   by 0x4AC3AC0: grpc_slice_malloc (in /nix/store/kfbhv9kid1dzblf16w9g1i10x48xkyrr-grpc-1.34.1/lib/libgrpc.so.14.0.0)
   by 0x41DCD1: grpc_slice_malloc_ (in ..)
   by 0x48D758: grpczmleakzm0zi0zi1zminplace_NetworkziGRPCziLowLevelziOp_createOpContext1_info (in ..)
```

However, we actually try to free this slice using `free_slice` in
`freeOpContext`, so this is unexpected.

Note that `grpc_slice` stores its payload on the heap and maintains its own
reference count, unless the payload is small enough to be stored inline. Perhaps
there is still an unknown reference to it somewhere, which keeps the
`grpc_slice` alive. I haven't found out why.

I did discover that decreasing `defaultStatusStringLen` and thus the size of the
slice made the memory leak disappear. I believe because a smaller size means
that no extra heap allocation is necessary and the payload can be stored inline.

As the comment on `defaultStatusStringLen` says that "gRPC actually ignores this
length and reallocates a longer string if necessary", decreasing its size should
be safe.

* Fix memory leak in op_send_status_server

There is no corresponding `free` for this `malloc` call in
`op_send_status_server`, as `op`s are not recursively `free`d.

Fix it by referring directly to the metadata instead of copying it, as the
metadata will remain alive for the lifetime of the `Op`, see
`withOpArrayAndCtxts`.

Source: #137

---------

Co-authored-by: Thomas Winant <[email protected]>
  • Loading branch information
fumieval and mrBliss authored Jul 18, 2023
1 parent 414ae8e commit d20c20d
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 10 deletions.
12 changes: 3 additions & 9 deletions core/cbits/grpc_haskell.c
Original file line number Diff line number Diff line change
Expand Up @@ -220,10 +220,7 @@ void op_send_initial_metadata(grpc_op *op_array, size_t i,
grpc_op *op = op_array + i;
op->op = GRPC_OP_SEND_INITIAL_METADATA;
op->data.send_initial_metadata.count = n_metadata;
op->data.send_initial_metadata.metadata
= malloc(n_metadata*sizeof(grpc_metadata));
memcpy(op->data.send_initial_metadata.metadata, arr,
n_metadata*sizeof(grpc_metadata));
op->data.send_initial_metadata.metadata = arr;
op->flags = 0;
op->reserved = NULL;
}
Expand All @@ -241,7 +238,7 @@ void op_send_message(grpc_op *op_array, size_t i,
grpc_byte_buffer *payload){
grpc_op *op = op_array + i;
op->op = GRPC_OP_SEND_MESSAGE;
op->data.send_message.send_message = grpc_byte_buffer_copy(payload);
op->data.send_message.send_message = payload;
op->flags = 0;
op->reserved = NULL;
}
Expand Down Expand Up @@ -298,10 +295,7 @@ void op_send_status_server(grpc_op *op_array, size_t i,
grpc_op *op = op_array + i;
op->op = GRPC_OP_SEND_STATUS_FROM_SERVER;
op->data.send_status_from_server.trailing_metadata_count = metadata_count;
op->data.send_status_from_server.trailing_metadata
= malloc(sizeof(grpc_metadata)*metadata_count);
memcpy(op->data.send_status_from_server.trailing_metadata, m,
metadata_count*sizeof(grpc_metadata));
op->data.send_status_from_server.trailing_metadata = m;
op->data.send_status_from_server.status = status;
op->data.send_status_from_server.status_details = details;
op->flags = 0;
Expand Down
2 changes: 1 addition & 1 deletion core/src/Network/GRPC/LowLevel/Op.hs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ data OpContext =
-- when processing 'OpRecvStatusOnClient'. It appears that gRPC actually ignores
-- this length and reallocates a longer string if necessary.
defaultStatusStringLen :: Int
defaultStatusStringLen = 128
defaultStatusStringLen = 20

-- | Allocates and initializes the 'Opcontext' corresponding to the given 'Op'.
createOpContext :: Op -> IO OpContext
Expand Down

0 comments on commit d20c20d

Please sign in to comment.