Problem/Motivation

On windows without SSL enabled there is an PHP deprecation message:

Deprecated function: strlen(): Passing null to parameter #1 ($string) of type string is deprecated in drupal_random_bytes() (line 2351 of /includes/bootstrap.inc).

PHP: 8.1
Database: PostgreSQL 11 / MySQL 8 (problem is present regardless of database type)
Drupal: 7.91

The problem appears to be in this part, where $bytes variable is going to the strlen() function:

if (strlen($bytes) < $count) {
  // Initialize on the first call. The contents of $_SERVER includes a mix of
  // user-specific and system information that varies a little with each page.

Something similar was fixed in the drupal_random_bytes() in #3241422: [PHP 8.1] Passing `null` to internal functions deprecation fixes, but this same usage of $bytes variable was missed.

Steps to reproduce

Install Drupal 7 on windows and disable openssl.

Proposed resolution

Cast $bytes to string as it was done few lines above.

$missing_bytes = $count - strlen((string) $bytes);

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#2 3299271-2.patch720 bytespoker10
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

poker10 created an issue. See original summary.

poker10’s picture

poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented
Status: Active » Needs review
FileSize
3299271-2.patch720 bytes

This patch should fix the problem.

poker10’s picture

poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented

Just to clarify the problem little bit more. There are two ways to fill the $bytes in that code:

1. 

if ($has_openssl) {
  $bytes .= openssl_random_pseudo_bytes($missing_bytes);
}

2. 

// Else, read directly from /dev/urandom, which is available on many *nix
// systems and is considered cryptographically secure.
elseif ($fh = @fopen('/dev/urandom', 'rb')) {

Neither of these will run on windows without openssl, so the $bytes variable will be still empty on the line 2351.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

Drupal 8.0.0 did something similar to D7.x in a method called randomBytes and they had also introduced randomBytesBase64

however Drupal 9.4.x they removed randomBytes and are only using randomBytesBase64 that was present in 8.0.0 also :

  public static function randomBytesBase64($count = 32) {
    return str_replace(['+', '/', '='], ['-', '_', ''], base64_encode(random_bytes($count)));
  }

The above mentioned code for D8/D9 is found in core/lib/Drupal/Component/Utility/Crypt.php

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

randomBytes() is deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use PHP\'s built-in random_bytes() function instead. See
https://drupal.org/node/3057191

hmm, so PHPs random_bytes function, maybe that exists in PHP 8.1, says to use that instead

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

PHPs random_bytes was added in PHP 7.0

so it would be fairly easy to check the PHP version, return the result from random_bytes if the version of PHP is >=7.0 otherwise use the Drupal 7 code compatible with PHP 5.2, 5.3, 5.4, 5.5, 5.6

and put this logic inside drupal_random_bytes

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Status: Needs review » Reviewed & tested by the community

RTBC patch #2

it's the exact same strategy already used by D7.x core , see line number 2318 of includes/bootstrap.inc

function drupal_random_bytes($count)  {
  // $random_state does not use drupal_static as it stores random bytes.
  static $random_state, $bytes, $has_openssl;

  $missing_bytes = $count - strlen((string) $bytes);
joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

see previous changes made by @mcdruid

#3241422: [PHP 8.1] Passing `null` to internal functions deprecation fixes

I double down on RTBC

commit 71626aac0465960774bfca7e1f741934e5218185
Author: mcdruid <drew@mcdruid.co.uk>
Date:   Thu Nov 11 21:28:17 2021 +0000

    Issue #3241422 by mcdruid, Ayesh: [PHP 8.1] Passing null to internal functions deprecation fixes
mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid as a volunteer and at Acquia commented
Issue tags: +RTBM

Yup, looks like we missed this one (that hopefully doesn't get used much!)

Thanks, ready for commit.

  • poker10 committed 9fd49e3 on 7.x 
    Issue #3299271 by poker10: [D7 PHP 8.1] strlen(): Passing null to...
poker10’s picture

poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -RTBM

Thanks everyone!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.