Fix a few memory leaks #140
Conversation
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: awakesecurity#137
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: awakesecurity#137
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.
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: awakesecurity#137
|
@mrBliss thank you for submitting this PR! We've been really busy but should have some time soon to review your PR. |
|
@ixmatus Did you have a chance to take a look at this? |
|
@fumieval I took a look, theres a few things I need to check but I plan on approving or leaving suggestions today. |
|
@fumieval I took a cursory look but I'm just not familiar enough with this C-code to do a good job reviewing it, generally it seems fine though. A more experienced C-programmer looked at it as well and concluded they would need to swap in a lot more context about how the C-code works before being able to leave comments. I unfortunately don't have time to do that (and I don't think they do either). Hopefully @riz0id does have the time but I do think we should be careful, especially with workarounds for a memory leak the original author didn't fully understand in 23b114e. |
|
FWIW, we switched to this version in production and it seems to be working fine so far |
|
@fumieval is it fine if I merge |
|
@fumieval Thank you, I'm sorry I've been so slow with this. I've been really busy and theres still some CI issues on OSX we've been dealing with lately. I'm trying my best to find some time to review this. Thanks for being patient. |
|
@riz0id No problem as we can always use a forked version. There's a problem on OSX environment indeed cachix/install-nix-action#183 |
Yeah, I've been dealing with that a lot lately. Waiting to see if #154 resolves the issue. |
|
Okay seems like upgrading to |
|
@fumieval I'm closing this and merging the rebased PR. |
This fixes the memory leaks identified in #137 and adds a workaround for another memory leak I don't fully understand. See the commit messages for more details.