Fix ZF-11921: Race condition in plugin loader include file cache #164

Merged
merged 3 commits into from Mar 28, 2014

Conversation

Projects
None yet
8 participants
Contributor

nicolas-grekas commented Jul 11, 2013

This fixes the race condition described at:
http://framework.zend.com/issues/browse/ZF-11921

For the race to disappear each and every line is made completely independent from the others. This is the reason why every line includes its own php open and close tag, and no file_get_content() is made, and the file is opened in append only mode.

To prevent the insertion of duplicates lines, a fast non blocking lock is made and kept until the end of the request, thus the static $h.

Tested on a highly concurrent server without a problem.

@weierophinney weierophinney commented on an outdated diff Jul 17, 2013

library/Zend/Loader/PluginLoader.php
@@ -471,14 +471,18 @@ public static function getIncludeFileCache()
*/
protected static function _appendIncFile($incFile)
{
- if (!file_exists(self::$_includeFileCache)) {
- $file = '<?php';
- } else {
- $file = file_get_contents(self::$_includeFileCache);
+ static $h = null;
@weierophinney

weierophinney Jul 17, 2013

Owner

Can you name this more sematically, please? $handle, , $cacheFile, etc...

We just hit this issue on a recent deployment. Although it happens very rarely, it is a pretty serious issue when it does occur since it spits out all the included files to the top of the HTML page.

It would be good to get this fix into ZF if the code change is OK. Is ZF1 still getting security fixes for things like this? Do you need any assistance testing this to get it approved?

Contributor

mhujer commented Jan 23, 2014

@nicolas-grekas it would be great, if you can rebase it on latest master (and eventually squash it). Travis passes for master, so it would be easy to see, if this does not break anything.

nicolas-grekas added some commits Jul 11, 2013

@nicolas-grekas nicolas-grekas Fix ZF-11921: Race condition in plugin loader include file cache
This fixes the race condition described at:
http://framework.zend.com/issues/browse/ZF-11921

For the race to disappear each and every line is made completely independent from the others. This is the reason why every line includes its own php open and close tag, and no file_get_content() is made, and the file is opened in append only mode.

To prevent the insertion of duplicates lines, a fast non blocking lock is made and kept until the end of the request, thus the static $h.

Tested on a highly concurrent server without a problem.
1688f3d
@nicolas-grekas nicolas-grekas Code cleanup 8d2a2af
@nicolas-grekas nicolas-grekas Minor fix in PluginLoader.php c7d7fd7
Contributor

nicolas-grekas commented Jan 24, 2014

Rebase done, tests pass!

Member

froschdesign commented Jan 25, 2014

@nicolas-grekas
Sorry, but I can not find a signed CLA. Please send me your e-mail address and I'll check again.

froschdesign was assigned Jan 25, 2014

Contributor

nicolas-grekas commented Jan 25, 2014

firstname.lastname at gmail
com

Member

froschdesign commented Jan 27, 2014

Hi Nicolas,
I can not find any signed CLA under your e-mail address. Sorry!
Have you ever sent a CLA?

Contributor

nicolas-grekas commented Jan 27, 2014

Never, I don't know anything about your process, I just do PHP ;)

Is this update stuck because of the CLA? If so, I have signed one for ZF1 if it's any help...

@froschdesign froschdesign modified the milestone: 1.12.5, 1.12.4, 1.12.6 Mar 7, 2014

Member

akrabat commented Mar 22, 2014

Ideally, we need @nicolas-grekas to sign the CLA (PDF download) and email it to cla@zend.com.

Contributor

nicolas-grekas commented Mar 22, 2014

I'll do that. Thanks for the links @akrabat

Member

akrabat commented Mar 27, 2014

@nicolas-grekas Did you get the CLA sent?

Contributor

nicolas-grekas commented Mar 27, 2014

Just now!

akrabat removed the No signed CLA label Mar 28, 2014

akrabat merged commit c7d7fd7 into zendframework:master Mar 28, 2014

1 check passed

default The Travis CI build passed
Details
Member

akrabat commented Mar 28, 2014

Thanks!

nuutco commented Apr 17, 2014

I just wanted to note that this change caused our zend caching to no longer work since we were externally creating the cache file with <?php appended to it before calling Zend_Loader_PluginLoader::setIncludeFileCache($cachePath);. This additional <?php would then trigger this error:

Parse error: syntax error, unexpected '<' in .../zendCache.php on line 2

I don't know if you'd consider that breaking backwards compatibility but I wanted to make mention of it in case anyone else has a similar issue. I can't comment as to why our legacy code was prepending <?php outside of setIncludeFileCache() but that's just how it was setup.

mhawk0 commented May 22, 2014

The plugin cache does not check for duplicates, but simply adding new require_onces at the end.
Before the change there was at least a check for

if (!strstr($file, $incFile)) {

but it's gone now, and it became useless as the file grows to hundreds of megabytes or more!

Member

froschdesign commented May 22, 2014

@marcing or @mhawk0
Please open a new bug report. Thanks!

mhawk0 commented May 22, 2014

Sorry for an alert too early.
Problem was on my side, as I misinterpreted the docs.
I thought that

    Zend_Loader_PluginLoader::setIncludeFileCache('a path to file');

is enough, but the include file cache was supposed to be manually included first:

    $classFileIncCache = APPLICATION_PATH . '/../data/pluginLoaderCache.php';
    if (file_exists($classFileIncCache)) {
        include_once $classFileIncCache;
    }

I assumed that the file is included automatically in Zend_Loader_PluginLoader::setIncludeFileCache() method.

After all, the cache never worked properly for us, as the include cache file was only written but not read. Before the change duplicates were not added because the whole file was read by file_get_contents and checked. After the change all includes were just appended to the end of file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment