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

Jump to bottom

Use arc4random_buf instead of rand on NetBSD #3540

Merged
merged 3 commits into from Aug 13, 2020
Merged

Use arc4random_buf instead of rand on NetBSD #3540

merged 3 commits into from Aug 13, 2020

Conversation

gufe44
Copy link
Contributor

@gufe44 gufe44 commented Aug 3, 2020 •
edited by mpg

This older implementation of rand generates the same small set of random numbers (or rather their lower bits) over and over again. A number of tests do not complete because of this. There exists already a conditional for OpenBSD making use of arc4random_buf instead which is also included in the C standard Library of NetBSD.

Backports: #3559 #3560

@gufe44
Use arc4random_buf instead of rand on NetBSD
c2620da 
Avoid old implementation of rand returning numbers with cyclical lower bits. Allow tests to pass.

Signed-off-by: gufe44 <gu981@protonmail.com>
@danh-arm danh-arm assigned danh-arm and unassigned danh-arm Aug 6, 2020
@danh-arm danh-arm added this to To do in Mbed TLS PRs via automation Aug 6, 2020
danh-arm
danh-arm requested changes Aug 6, 2020
View reviewed changes
Copy link
Contributor

@danh-arm danh-arm left a comment

Choose a reason for hiding this comment

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

Thanks. The change looks good but could you add a change log entry for this please?

Mbed TLS PRs automation moved this from To do to In progress Aug 6, 2020
danh-arm
danh-arm previously approved these changes Aug 6, 2020
View reviewed changes
Copy link
Contributor

@danh-arm danh-arm left a comment

Choose a reason for hiding this comment

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

Looks good but it would be good to squash the last 2 commits together.

@danh-arm danh-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Aug 6, 2020
@danh-arm danh-arm moved this from In progress to 1 Approval in Mbed TLS PRs Aug 6, 2020
@gufe44
Add changelog entry
ed06d2f 
Signed-off-by: gufe44 <gu981@protonmail.com>
Mbed TLS PRs automation moved this from 1 Approval to In progress Aug 6, 2020
@danh-arm danh-arm self-assigned this Aug 10, 2020
danh-arm
danh-arm previously approved these changes Aug 10, 2020
View reviewed changes
@danh-arm danh-arm moved this from In progress to 1 Approval in Mbed TLS PRs Aug 10, 2020
mpg
mpg previously approved these changes Aug 12, 2020
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Mbed TLS PRs automation moved this from 1 Approval to Approved Aug 12, 2020
@mpg mpg added bug and removed enhancement labels Aug 12, 2020
@mpg
Copy link
Contributor

mpg commented Aug 12, 2020

Thanks for your contribution! One thing though: if the issue prevented some test from passing, I'd call that a bug fix, not just an enhancement.

As a general rule, we backport bug fixes to our LTS branches (currently 2.7 and 2.16). Would you be willing to create those backports? It would involve raising two PRs, one based on mbedtls-2.16, one on mbedtls-2.7, with the same patches as this one (usually git cherry-pick is useful in creating backports).

Otherwise we could handle the backports ourselves, but we do appreciate when contributors can take care of them as well.

@mpg mpg added needs-backports Backports are missing or are pending review and approval. and removed needs-review Every commit must be reviewed by at least two team members, labels Aug 12, 2020
@gufe44
Log change as bugfix
29fcac3 
Signed-off-by: gufe44 <gu981@protonmail.com>
@gufe44 gufe44 dismissed stale reviews from mpg and danh-arm via 29fcac3 August 13, 2020 07:17
Mbed TLS PRs automation moved this from Approved to In progress Aug 13, 2020
mpg
mpg approved these changes Aug 13, 2020
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM

@mpg mpg moved this from In progress to 1 Approval in Mbed TLS PRs Aug 13, 2020
Mbed TLS PRs automation moved this from 1 Approval to Approved Aug 13, 2020
danh-arm
danh-arm approved these changes Aug 13, 2020
View reviewed changes
Copy link
Contributor

@danh-arm danh-arm left a comment

Choose a reason for hiding this comment

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

Hi @gufe44. This is approved but I'm holding off merging until you answer @mpg 's question about backports.

@gufe44
Copy link
Contributor Author

gufe44 commented Aug 13, 2020

Thank you very much. I have prepared the backport branches.

This was referenced Aug 13, 2020
Merged
Merged
@gilles-peskine-arm gilles-peskine-arm added component-crypto Crypto primitives and low-level interfaces and removed needs-backports Backports are missing or are pending review and approval. labels Aug 13, 2020
@gilles-peskine-arm gilles-peskine-arm merged commit 3890f7c into Mbed-TLS:development Aug 13, 2020
Mbed TLS PRs automation moved this from Approved to Done Aug 13, 2020
@gufe44 gufe44 mentioned this pull request Aug 14, 2020
Merged
@daverodgman daverodgman removed this from Done in Mbed TLS PRs Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpg mpg mpg approved these changes

@danh-arm danh-arm danh-arm approved these changes

Assignees

@danh-arm danh-arm

Labels
bug component-crypto Crypto primitives and low-level interfaces
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@gufe44 @mpg @danh-arm @gilles-peskine-arm