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

Do not rely on calloc zeroing out allocations #474

Merged
merged 3 commits into from
Aug 7, 2024

Conversation

jtraglia
Copy link
Member

@jtraglia jtraglia commented Aug 6, 2024

There were two spots which relied on calloc zeroing out values. I don't like this.

I left the MEMSET_TO_FF macro option to test this. Could be useful in the future. Open to removing it.

And yeah, malloc doesn't zero out allocations, but wanted to be consistent.

Also fix a random comment's wrap that I noticed.

@jtraglia jtraglia requested a review from asn-d6 August 6, 2024 22:48
@jtraglia jtraglia mentioned this pull request Aug 6, 2024
@asn-d6
Copy link
Contributor

asn-d6 commented Aug 7, 2024

Just curious, why you dislike relying on calloc to zero out values?

Also, why do you loop and manually set arrays to FR_ZERO, over a memset? Do you dislike relying on FR_ZERO being all zeroes?

@jtraglia
Copy link
Member Author

jtraglia commented Aug 7, 2024

Just curious, why you dislike relying on calloc to zero out values?

So calloc is required by the C standard to zero out the data, so it should be perfectly safe to rely on this. But it kind of tells me that we're depending on a feature that isn't exactly clear to everyone. It's possible someone unfamiliar with C doesn't know that calloc behaves that way.

Also, why do you loop and manually set arrays to FR_ZERO, over a memset?

I think the loop is more readable than memset. I think performance difference is negligible & probably optimized away.

Do you dislike relying on FR_ZERO being all zeroes?

To small degree, yes 😄

src/eip7594/eip7594.c Outdated Show resolved Hide resolved
Co-authored-by: George Kadianakis <desnacked@riseup.net>
Copy link
Contributor

@asn-d6 asn-d6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

@asn-d6 asn-d6 merged commit 96d51ad into ethereum:main Aug 7, 2024
38 checks passed
@jtraglia jtraglia deleted the dont-rely-on-calloc-zeroize branch August 7, 2024 16:32
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