search

cowsql / raft

Asynchronous C implementation of the Raft consensus protocol
https://raft.readthedocs.io
Other
45 stars 6 forks source link

[atlesn] Fix misc. usage of uninitialized memory #180

Closed atlesn closed 5 months ago

atlesn commented 5 months ago

I identified three places where uninitialized memory was used for outgoing traffic using valgrind. This seemed to be caused by misc. struct members not being set explicitly although I have not investigated this in detail. I replaced some mallocs with callocs which seemed to solve the problem.

codecov[bot] commented 5 months ago

Codecov Report

Attention: Patch coverage is 88.23529% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 74.19%. Comparing base (bee6c36) to head (e99dde7).

Files Patch % Lines
src/uv_encoding.c 75.00% 0 Missing and 1 partial :warning:
src/uv_tcp_connect.c 85.71% 0 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #180 +/- ## ========================================== + Coverage 74.16% 74.19% +0.03% ========================================== Files 52 52 Lines 10358 10367 +9 Branches 2465 2465 ========================================== + Hits 7682 7692 +10 Misses 1321 1321 + Partials 1355 1354 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

freeekanayaka commented 5 months ago

I identified three places where uninitialized memory was used for outgoing traffic using valgrind. This seemed to be caused by misc. struct members not being set explicitly although I have not investigated this in detail. I replaced some mallocs with callocs which seemed to solve the problem.

Thanks!

I left some comments. The barrier case looks fine. The other two I'm not sure, if possible I'd like to get to the bottom of this and explicitly initialize the memory without resorting to calloc.

atlesn commented 5 months ago

Changed all three callocs to explicit initialization. Also tried to follow styles used elsewhere in the project, please have a look if it feels OK.

atlesn commented 5 months ago

I found another one where padding of encoded configuration struct was not initialized. Sorry about force push, I did the end pointer calculation twice initially. But I guess the commits should be squashed probably anyway?

freeekanayaka commented 5 months ago

I found another one where padding of encoded configuration struct was not initialized. Sorry about force push, I did the end pointer calculation twice initially. But I guess the commits should be squashed probably anyway?

Thanks! I think this is good to go as it is now. It seems to fix the problem in a good away with explicit memory initialization instead of blind calloc calls.

I might tweak the code a bit after merging this, but it would be purely cosmetics/nits.