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

Bug: random_string() Implicit conversion from float 4.5 to int loses precision #6330

Closed
turbopixel opened this issue Aug 2, 2022 · 4 comments · Fixed by #6334
Closed

Bug: random_string() Implicit conversion from float 4.5 to int loses precision #6330

turbopixel opened this issue Aug 2, 2022 · 4 comments · Fixed by #6334
Assignees
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@turbopixel
Copy link

turbopixel commented Aug 2, 2022

PHP Version

8.1

CodeIgniter4 Version

4.2.1

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

Linux Debian

Which server did you use?

fpm-fcgi

Database

MariaDB

What happened?

I'm using the codeigniter build in helper helper('text'); and want to create a random string called the function random_string("crypto", 9); With an odd number I always get the error: Implicit conversion from float 4.5 to int loses precision. So the numbers 3, 5, 7, 9, 11 (...) are not usable. With even numbers I always get a random string.

What function is called?

  • system/Helpers/text_helper.php
if (!function_exists('random_string')) {
  /**
   * Create a Random String
   *
   * Useful for generating passwords or hashes.
   *
   * @param string $type Type of random string.  basic, alpha, alnum, numeric, nozero, md5, sha1, and crypto
   * @param int $len Number of characters
   */
  function random_string(string $type = 'alnum', int $len = 8) : string {}
}

This case I call with random_string("crypto", 9):

return bin2hex(random_bytes($len / 2));

Steps to Reproduce

Didn't work:

helper('text');
$pw = random_string("crypto", 9);

Works fine:

helper('text');
$pw = random_string("crypto", 8);

Expected Output

A random string like b3da1087d or something else.

Anything else?

Last two Backtrace rows:

  1. {PHP internal code} — CodeIgniter\Debug\Exceptions->errorHandler ()
  2. SYSTEMPATH/Helpers/text_helper.php : 576 — random_bytes()

grafik

Solution maybe

Cast the result to an integer:

return bin2hex(random_bytes((int)($len / 2)));
@turbopixel turbopixel added the bug Verified issues on the current code behavior or pull requests that will fix them label Aug 2, 2022
@kenjis
Copy link
Member

kenjis commented Aug 2, 2022

Thank you for reporting!

It seems random_string("crypto", 9) is a misuse.
Because it is impossible to generate strings with the length of 9.
The output length is 8 if you use PHP 8.0.
You must set an even number in the second parameter.

So we should throw an InvalidArgumentException?

@turbopixel
Copy link
Author

turbopixel commented Aug 3, 2022

I see it differently. The problem is that the function random_bytes() expects an integer, but gets a float. So it would be right (In my opinion) to round up the number or cast it as int.

@paulbalandan
Copy link
Member

@turbopixel You're correct in a way that the float to int conversion is the immediate problem which can be patched by simply rounding up the float or discarding the decimal thru casting to int. But, if you'll look at the design deeper, the $len is first halved before being passed to random_bytes which accepts an int. Since dividing an integer by 2 gives either an integer (even) or float (odd) and the consuming function needs an integer, I think it should be right to assume that we should pass only even numbers in the $len in the first place.

On the other hand, another way is to just pass the $len argument as-is. Why does it need to be halved, anyway?

@turbopixel
Copy link
Author

@paulbalandan The secret will probably be the PHP function itself. In the example you can see that a length of 5 is specified and the return value is 10 characters long. This will probably be the reason why the originally specified length is divided by 2.

https://www.php.net/manual/en/function.random-bytes.php#refsect1-function.random-bytes-examples

Now, there are two options:

  1. The specified length is given directly (without / 2) and the return value of bin2hex() is shortened afterwards.
  2. @kenjis idea will be implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants