Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use pseudo-random rand() instead of cryptographically secure random numbers #3152

Closed
wants to merge 1 commit into from
Closed

Use pseudo-random rand() instead of cryptographically secure random numbers #3152

wants to merge 1 commit into from

Conversation

benbucksch
Copy link

Some AMD CPUs seems to return a non-random number, but still claim success. This makes random_device throw an exception, which is passed to the host application, which then crashes.

To work around this, simply use pseudo-random numbers.

Fixes #3151 and probably sass/node-sass#3047

This is a simplistic fix, but is tested to work and fix the problem on a CPU where it was broken before. I can surely imagine more elaborate fixes, e.g. to use random_device first, catch the exception, and use rand() only as fallback.

However, I do wonder why libsass is using cryptographically secure random numbers in the first place, and what for. I don't see why CSS would need that. I would think that pseudo-random is sufficient. So, maybe this is even the right fix, as-is.

I am contributing the fix that I found. If you would like to improve this, please go ahead. My involvement stops here. My objectively was merely to continue to use the host application, which suddenly failed on the new computer. I hope to help others who run into the same problem.

Some AMD CPUs seems to return a non-random number, but still claim success. This makes `random_device` throw an exception, which is passed to the host application, which then crashes.

To work around this, simply use pseudo-random numbers.
@mgreter
Copy link
Contributor

mgreter commented May 10, 2021

As mentioned in the issue ticket, this approach will generate the same output over and over again for the following sass:

test { nr: random(); }

On my PC it always produces:

test { nr: 0.6805727266; }

See #3153 for a more robust implementation.
BTW. the failing CI test are an issue with sass-spec and its ruby runner which two new tests have triggered ...

@benbucksch
Copy link
Author

See #3153 for a more robust implementation.

That makes sense. Thank you!

@benbucksch benbucksch closed this May 10, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

random_device: rdrand failed - Crashes host application
2 participants