The current core/rebuild.php script has

// Clear the APC cache to ensure APC class loader is reset.
if (function_exists('apc_fetch')) {
  apc_clear_cache('user');
}

near the top of the file OUTSIDE the access check. This can lead to a DOS situation, where constant GETs of core/rebuild.php will clear the user cache.

Quoting @alexpott

the class map doesn't need clearing in rebuild.php btw
if a class moves it's PSR namespace changes... you get a cache miss - which is not a problem.

CommentFileSizeAuthor
#8 2476247.8.patch960 bytesalexpott
#2 2476247.2.patch449 bytesalexpott
#1 rebuild_php_clears_apu-2476247-1.patch998 bytesmpdonadio
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Status: Active » Needs review
FileSize
rebuild_php_clears_apu-2476247-1.patch998 bytes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2476247.2.patch449 bytes

The run tests part of this part will not work - run-tests.sh runs from cli and can not clear the web server cache. The other part of the patch is critical because of the potential DOS.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

the class map doesn't need clearing in rebuild.php btw
if a class moves it's PSR namespace changes... you get a cache miss - which is not a problem.

Not so sure about that.

Yes, if the namespace changes, new classes or deleted classes, we're fine.

But our module system allows to move modules around, for example, you might have some_module in profiles/something/modules, and then add a site specific override at /modules/some_module. Then all those classes of that module would now point to the wrong folder.

I'd suggest to move this inside drupal_flush_all_caches()...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

We can't move it into drupal_flush_all_caches() because that will remove the benefits of caching YAML files in APC user cache.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

I am happy to remove it from core/rebuild.php again or put it after the access check, but what the patch does, does not fly as:

cmdline APC != web APC

That is why it was in rebuild.php in the first place ...

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

#2: Can we just move it after the access check in rebuild.php?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Related issues: +#2474909: Allow Simpletest to use the same APC user cache prefix so that tests can share the classmap and other cache objects
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2476247.8.patch960 bytes

Okay let's move it and also fix php 5.5+

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
Status: Needs review » Reviewed & tested by the community

RTBC, thanks!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed ac85428 on 8.0.x 
    Issue #2476247 by alexpott, mpdonadio: rebuild.php clears APU user cache...
mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Title: rebuild.php clears APU user cache w/o access check » rebuild.php clears APC user cache w/o access check

Just fixing the type in the title...

Status: Fixed » Closed (fixed)

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