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

Fix warning: 'arc4random_addrandom' is deprecated: use arc4random_stir #1631

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Coeur
Copy link
Contributor

@Coeur Coeur commented Apr 28, 2024

Ideally, arc4random_addrandom should never be used. It's deprecated in favor of arc4random_stir:
https://opensource.apple.com/source/Libc/Libc-1439.40.11/include/stdlib.h.auto.html

Copy link
Member

@azat azat left a comment

Choose a reason for hiding this comment

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

LGTM, but MacOS CI is broken now, and it should be fixed first (#1632)

Copy link
Member

@azat azat left a comment

Choose a reason for hiding this comment

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

.

@Coeur Coeur requested a review from azat May 4, 2024 14:56
Copy link
Contributor

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

Ideally, arc4random_addrandom should never be used. It's deprecated in favor of arc4random_stir.

Can you expand the PR description to explain what you're doing and why, with links to the supposed deprecations (or at least the relevant warning output). The original PR description had something about macOS, but this code is going to effect systems other than macOS, so that's not the only thing that should be considered.

From a quick look at one manpage, i.e https://man.archlinux.org/man/arc4random_stir.3bsd.en, arc4random_addrandom and arc4random_stir are similar in function, but not marked as deprecated?

I also see:

It is not necessary for an application to call arc4random_stir() or arc4random_addrandom() before calling other arc4random functions.
Additionally, arc4random_stir() and arc4random_addrandom() may abort the process in the highly unlikely event that the operating system fails to provide entropy.

So it could be considered as unnecessary to call arc4random_stir as use arc4random_addrandom, so it's not clear why swapping the usage of one, for the other, here, is an improvement, or the right change to make?

event-config.h.cmake Show resolved Hide resolved
@@ -193,9 +193,11 @@ evutil_secure_rng_get_bytes(void *buf, size_t n)
void
evutil_secure_rng_add_bytes(const char *buf, size_t n)
{
#if !defined(EVENT__HAVE_ARC4RANDOM) || defined(EVENT__HAVE_ARC4RANDOM_ADDRANDOM)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're removing the use of EVENT__HAVE_ARC4RANDOM_ADDRANDOM, then why is the check for arc4random_addrandom being retained in both build systems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean to do like that: c7adfa5
Right?

@Coeur Coeur requested a review from fanquake June 3, 2024 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants