- Status changed to Needs review
almost 2 years ago 6:18pm 23 January 2023 - πΊπΈUnited States ednark
I wanted to try a different approach than the 3221065-7 patch.
Problem: This type of check cannot rely on Drupal's internal dynamically built queries. Drupal Core Database's StatementPrefetch call relies on a full download of all result data from the database. This means there will always be a resource limit imposed by the Drupal host from this one call in core via:
$this->data = $statement->fetchAll(\PDO::FETCH_ASSOC);
Possible Solutions: In this type of situation you either need to run separate small queries per item you want to check, or push the storage of the result set into the database itself and only ask to download one result at at time.
Solution: Running a separate query to load each node in the system seems like a waste so I will try generating one query per field instead and attempt leave the data on the storage server as long as possible. To do this, I will use a static query instead of a dynamic one. A drawback to this method will be that since the table and column names are dynamic we will have to construct a new query for every field we want to check. Normally you should avoid string concatenation when dealing with queries, but in this specific situation all column names are being pulled directly from Drupal in the first place and so they should be safe in a practical sense from any possible corruption, even if we have to use a bad practice.
This patch reorders the loops so the id and data columns are calculated first which allows us to generate a new query for each field needing to be checked. We then also need to be sure to unset the $query variable each loop to free any temporary memory reserved by the underlying libraries.
- πΊπΈUnited States smustgrave
Add to tweak the patch in #8 as it wasn't picking up on nodes that I added a script tag to.
- Status changed to RTBC
almost 2 years ago 10:22pm 23 January 2023 - πΊπΈUnited States ednark
I have tested this and it functions as expected without errors.
Tested on 10.1.x-dev with a standard install
module drupal/security_review:2.0.x-dev@dev
patch 3221065-9.patchTest procedure:
start with base 2.0.x module code
add an unreasonable amount of content items via devel
drush devel-generate-content --bundles=page 200001
run test to see Memory error generated
apply patch
run test to see it completes and has no Fields errors
add script tag to any piece of content
run test to see a Field error in the piece of content
add the errored content's reported Hash to the ignore list
run test to see no Field errors - @smustgrave opened merge request.
-
smustgrave β
committed 5e113717 on 2.0.x
Issue #3221065: Field check out of memory
-
smustgrave β
committed 5e113717 on 2.0.x
- Status changed to Fixed
almost 2 years ago 10:28pm 23 January 2023 Automatically closed - issue fixed for 2 weeks with no activity.