Page MenuHomePhabricator
Create Task
Maniphest T4153

Filesystem::readRandomBytes does not work on Windows without php_openssl.dll
Closed, ResolvedPublic
Actions

Description

I had to explicitly enable extension=ext/php_openssl.dll in php.ini for it to work. The error message that was thrown was:

[2013-11-23 12:30:00] EXCEPTION: (FilesystemException) Failed to open /dev/urandom for reading! at [C:\Tools\libphutil\src\filesystem\Filesy
stem.php:400]
  #0 Filesystem::readRandomBytes(10) called at [C:\Tools\libphutil\src\filesystem\Filesystem.php:449]
  #1 Filesystem::readRandomCharacters(10) called at [C:\Tools\arcanist\src\unit\engine\XUnitTestEngine.php:318]
  #2 XUnitTestEngine::buildTestFuture(C:\Users\James\Documents\Projects\Tychaia\Tychaia.Tests/bin/Debug/Tychaia.Tests.dll) called at [C:\Too
ls\arcanist\src\unit\engine\XUnitTestEngine.php:353]
  #3 XUnitTestEngine::testAssemblies(Array of size 9 starting with: { 0 => Array of size 2 starting with: { project => C:\Users\James\Docume
nts\Projects\Tychaia\Tychaia.Tests\Tychaia.Tests.Windows.csproj } }) called at [C:\Tools\arcanist\src\unit\engine\XUnitTestEngine.php:171]
  #4 XUnitTestEngine::runAllTests(Array of size 9 starting with: { 0 => Array of size 2 starting with: { project => C:\Users\James\Documents
\Projects\Tychaia\Tychaia.Tests\Tychaia.Tests.Windows.csproj } }) called at [C:\Tools\arcanist\src\unit\engine\XUnitTestEngine.php:102]
  #5 XUnitTestEngine::run() called at [C:\Tools\arcanist\src\workflow\ArcanistUnitWorkflow.php:173]
  #6 ArcanistUnitWorkflow::run() called at [C:\Tools\arcanist\scripts\arcanist.php:321]

althrough from the code it looks like the Filesystem::readRandomBytes() requires at least PHP 5.3 or /dev/urandom was probably the more intended exception in this case.

For Windows it should probably make it clear what the user needs to do (enable php_openssl.dll in their configuration). In the default Windows binaries that are shipped for PHP, there are no extensions enabled by default, so this is something all Windows users will have to do.

Revisions and Commits

rPHU libphutil
D7650rPHU5fab43a1cccd Improve error messages from Filesystem::readRandomBytes()

Related Objects

Event Timeline

hach-que created this task.Nov 23 2013, 12:36 PM
hach-que assigned this task to epriestley.
hach-que raised the priority of this task from to Needs Triage.
hach-que updated the task description. (Show Details)
hach-que added a subscriber: hach-que.
epriestley added a comment.Nov 25 2013, 2:35 AM

Yeah, this error message could probably use some improvement after D7528.

I'm tempted to just fall back to calling mt_rand() a bunch of times if the more reasonable random sources don't exist. The big reason I'm hesitant to do this is that if you have a call like this:

do_stuff();
$x = Filesystem::readRandomBytes(12);

...then $x will be completely deterministic if do_stuff() calls mt_srand(3).

We'd have to call mt_srand() ourselves, and we don't have a ton of great entropy sources to seed it with. Offhand:

  • microtime() - Fine, but predictable.
  • getmypid() - Really weak entropy source. Does this even work on Windows?
  • uniqid() with $more_entropy = true? Not sure how good / predictable this is on Windows.
  • Sources like memory_get_usage() and spl_object_hash(), which might be very weak entropy sources.
  • tempnam(), which leaks and is probably really expensive?

Maybe there's enough entropy there to reasonably seed the generator. We don't have super strong randomness requirements -- it needs to be unpredictable, but it doesn't need to be cryptographic-quality randomness today. However, the method currently produces the best quality randomness it reasonably can, and I'm hesitant to silently degrade it.

Probably better to just fix the message.

epriestley edited this Maniphest Task.Nov 25 2013, 8:58 PM
epriestley edited this Maniphest Task.Nov 25 2013, 10:56 PM
epriestley closed this task as Resolved.Nov 25 2013, 10:56 PM

Closed by commit rPHU5fab43a1cccd.

igorgatis added a subscriber: igorgatis.Mar 24 2015, 5:03 PM

T7554: XUnitTestEngine: remove OpenSSL dependency (use uniqid) is a duplicate of this task. It seems you considered uniqid().

Mind explaining why predictability is an issue here?

epriestley added a comment.Mar 24 2015, 5:13 PM

Enabling OpenSSL is trivial and Filesystem::readRandomBytes() is used to generate cryptographically sensitive randomness.

The original report here focused on the next steps not being obvious, not on removing the OpenSSL dependency for the test engine.

It would not be acceptable to implement readRandomBytes() in terms of a predictable source.

It would be acceptable to implement the XUnitTest engine in terms of a weak random source, but the construction in XUnitTestEngine seems generally suspicious. For example: can we make TempFile() emit a path with DIRECTORY_SEPARATOR delimiters to fix this instead? Hacking around a hack around a bizarre behavior is almost certainly not the best fix.

Phabricator · Built by Phacility