Skip to content

Conversation

lt
Copy link
Contributor

@lt lt commented Sep 24, 2014

The existing gmp_random() function doesn't allow the range of numbers to be fine-tuned, as the range is determined by a multiple of LIMB_BITS, which varies by platform.

This patch introduces:

  • gmp_random_bits(int bits) - generates a number between 0 and (2 ** bits) - 1
  • gmp_random_range(mixed min, mixed max) - generates a number between min and max

Outstanding issues with this patch:

  1. I'm not sure how to create deterministic tests for random functions. I have created tests for error conditions, but not sure what more I can do. Suggestions welcome.

  2. Attempting to generate a sufficiently large number with gmp_random_bits() has two error scenarios. A moderately large number of bits can cause a PHP FATAL because the memory limit is breached, I think this is fine. An insanely large number of bits can cause GMP itself to abort, and I'm not sure how to catch this, although I think it's also fine if it's documented (the same as you can cause PCRE to abort by recursing it out of stack space - it's not PHPs fault)

@lt
Copy link
Contributor Author

lt commented Sep 24, 2014

Oh, it also switches the default RNG from LCG to MT on platforms that support it. We currently don't allow seeding of the GMP RNG, so it doesn't break BC.

@@ -1784,6 +1795,22 @@ ZEND_FUNCTION(gmp_sign)
}
/* }}} */

void gmp_init_random()
Copy link
Member

Choose a reason for hiding this comment

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

Needs a TSRMLS_D parameter.

 - Thread safety on rand init function.
 - Ret false on validation failure
 - Add _dep of temp_a to temp_b
 - Special case int sized min values
 - More tests!
@nikic
Copy link
Member

nikic commented Oct 10, 2014

@smalyshev Do these APIs look okay to you? Any objections from your side to adding them?

@smalyshev
Copy link
Contributor

I wonder if it'd be also not good to add ability to initialize with different seed. This can serve two purposes: 1. test mocking and 2. our seed function had some complaints for not providing enough randomness. Custom seed allows people to use something like /dev/urandom or even /dev/random or any other source to their liking.
Otherwise it looks good on the quick look, I'll look in detail a bit later.

@nikic
Copy link
Member

nikic commented Oct 10, 2014

@smalyshev We can't add the ability to explicitly seed the RNG currently, because we support pre-4.2 libgmp versions, which do not support the MT generator (so you'll get different results depending on which GMP version you use). We could however add the ability to seed in PHP 7 (assuming that we get a stable RNG sequence there - hopefully GMP and MPIR generate the same).

@lt
Copy link
Contributor Author

lt commented Oct 11, 2014

I see @nikic already dropped support for 4.1 in master, I'm happy to add seeding functions (and if you feel appropriate algorithm selection (mt/lcg) functions) for master. I think from 4.2 onwards the random function is also hot-pluggable with a custom generator, so we could also provide our own generators that allow /dev/*random if desired.

@lt
Copy link
Contributor Author

lt commented Oct 13, 2014

@smalyshev @nikic If there are no objections to this patch, it would be nice to get it in before 5.6.2 is tagged (tomorrow I believe is the schedule)

@smalyshev
Copy link
Contributor

@lt I don't have any objection to it. However, I don't think we can put it in 5.6.2 day before the release - I think that would be 5.6.3 within the regular RC/release process.

@lt
Copy link
Contributor Author

lt commented Oct 13, 2014

@smalyshev can you merge for me? I don't have karma.

@smalyshev
Copy link
Contributor

@lt ok, I'll do it next couple of days.

@php-pulls php-pulls merged commit 106fab0 into php:PHP-5.6 Oct 14, 2014
@lt
Copy link
Contributor Author

lt commented Oct 18, 2014

I've written a rand seeding function for master, any preference between gmp_srandom and gmp_random_seed or something else? (we obviously have rand and srand, and mt_rand and mt_srand already), my patch is already using gmp_srandom

Edit: Nikita is +1 on gmp_random_seed

@lt lt mentioned this pull request Oct 19, 2014
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.

4 participants