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.

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

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

Merged
merged 3 commits into from
Mar 23, 2024

Conversation

atlesn
Copy link
Contributor

@atlesn atlesn commented Mar 22, 2024

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.

Copy link

codecov bot commented Mar 22, 2024

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 ⚠️
src/uv_tcp_connect.c 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/convert.c Outdated Show resolved Hide resolved
src/uv_encoding.c Outdated Show resolved Hide resolved
src/uv_tcp_connect.c Outdated Show resolved Hide resolved
@freeekanayaka
Copy link
Member

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
Copy link
Contributor Author

atlesn commented Mar 23, 2024

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
Copy link
Contributor Author

atlesn commented Mar 23, 2024

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
Copy link
Member

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.

@freeekanayaka freeekanayaka merged commit 421cffc into cowsql:main Mar 23, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants