On 10/27/2012 03:50 PM, Stas Malyshev wrote:
> Hi!
>
>> An APC-level fix might be to fix the include_once/require_once override
>> implementation to go to the cache first to see if the inode is there and
>> pull out the cached realpath and use that to check against the
>> included_files list. The downside is that we will likely end up with 2
>> cache lookups on every include_once.
>
> Comparing inodes sounds like a good idea for APC. Or, rather, comparing
> device,inode tuples. However, it should be able to fall back to
> comparing paths for files that have no inode.
>
> However, this may lead to BC break in (admittedly very unlikely)
> scenario of file hardlinked under two different names.
>
Ok, I have a working implementation which uses the normal APC cache
lookup mechanism which by default will use an inode lookup, so through
that we get the device+inode check, and it will then pull the realpath
instead of going through VCWD_REALPATH() to get a possibly incorrect
one. See the attached patch. This takes care of the hardlink scenario as
well because it checks both the inode and the cached path. Anyone see
any holes in this approach?
-Rasmus
Index: apc_main.c
===================================================================
--- apc_main.c (revision 328165)
+++ apc_main.c (working copy)
@@ -450,6 +450,27 @@
}
/* }}} */
+/* {{{ apc_get_cache_entry
+ Fetches the cache entry for a file */
+apc_cache_entry_t* apc_get_cache_entry(zend_file_handle* h TSRMLS_DC)
+{
+ apc_cache_key_t key;
+ time_t t;
+
+ if (!APCG(enabled) || apc_cache_busy(apc_cache)) {
+ return NULL;
+ }
+
+ t = apc_time();
+
+ if (!apc_cache_make_file_key(&key, h->filename, PG(include_path), t
TSRMLS_CC)) {
+ return NULL;
+ }
+ return apc_cache_find(apc_cache, key, t TSRMLS_CC);
+
+}
+/* }}} */
+
/* {{{ my_compile_file
Overrides zend_compile_file */
static zend_op_array* my_compile_file(zend_file_handle* h,
Index: apc_zend.c
===================================================================
--- apc_zend.c (revision 328165)
+++ apc_zend.c (working copy)
@@ -122,9 +122,11 @@
zval *freeop1 = NULL;
zval *inc_filename = NULL, tmp_inc_filename;
php_stream_wrapper *wrapper;
- char *path_for_open, realpath[MAXPATHLEN];
+ char *path_for_open, realpath_storage[MAXPATHLEN], *realpath;
int ret = 0;
apc_opflags_t* flags = NULL;
+ zend_file_handle file_handle;
+ apc_cache_entry_t* cache_entry;
#ifdef ZEND_ENGINE_2_4
if (opline->extended_value != ZEND_INCLUDE_ONCE &&
@@ -149,16 +151,41 @@
inc_filename = &tmp_inc_filename;
}
+ /* Check to see if this is a regular file and bail if it isn't */
wrapper = php_stream_locate_url_wrapper(Z_STRVAL_P(inc_filename),
&path_for_open, 0 TSRMLS_CC);
-
- if (wrapper != &php_plain_files_wrapper ||
!IS_ABSOLUTE_PATH(path_for_open, strlen(path_for_open)) ||
!VCWD_REALPATH(path_for_open, realpath)) {
- /* Fallback to original handler */
+ if (wrapper != &php_plain_files_wrapper) {
if (inc_filename == &tmp_inc_filename) {
zval_dtor(&tmp_inc_filename);
}
return
apc_original_opcode_handlers[APC_OPCODE_HANDLER_DECODE(opline)](ZEND_OPCODE_HANDLER_ARGS_PASSTHRU);
}
+ /*
+ Before we do anything else, we can check to see if we have a cached
version of this
+ file, in which case we can skip the realpath - this also means that we
will be checking
+ the correct realpath for this cache entry since it is possible that the
inode existed in
+ a different directory at the time it was cached. Some deploy systems
will do tricky things
+ where they rename directories and move a symlink around.
+ */
+ file_handle.filename = inc_filename->value.str.val;
+ file_handle.free_filename = 0;
+ file_handle.type = ZEND_HANDLE_FILENAME;
+ file_handle.opened_path = NULL;
+ file_handle.handle.fp = NULL;
+ cache_entry = apc_get_cache_entry(&file_handle);
+ if (cache_entry) {
+ realpath = cache_entry->data.file.filename;
+ } else {
+ if (!IS_ABSOLUTE_PATH(path_for_open, strlen(path_for_open)) ||
!VCWD_REALPATH(path_for_open, realpath_storage)) {
+ /* Fallback to original handler */
+ if (inc_filename == &tmp_inc_filename) {
+ zval_dtor(&tmp_inc_filename);
+ }
+ return
apc_original_opcode_handlers[APC_OPCODE_HANDLER_DECODE(opline)](ZEND_OPCODE_HANDLER_ARGS_PASSTHRU);
+ }
+ realpath = realpath_storage;
+ }
+
if (zend_hash_exists(&EG(included_files), realpath, strlen(realpath) + 1))
{
#ifdef ZEND_ENGINE_2_4
if (!(opline->result_type & EXT_TYPE_UNUSED)) {
Index: apc_cache.h
===================================================================
--- apc_cache.h (revision 328165)
+++ apc_cache.h (working copy)
@@ -207,6 +207,8 @@
extern int *apc_cache_insert_mult(apc_cache_t* cache, apc_cache_key_t* keys,
apc_cache_entry_t** values, apc_context_t *ctxt,
time_t t, int num_entries TSRMLS_DC);
+extern apc_cache_entry_t* apc_get_cache_entry(zend_file_handle* h TSRMLS_DC);
+
/*
* apc_cache_find searches for a cache entry by filename, and returns a
* pointer to the entry if found, NULL otherwise.
--
PECL development discussion Mailing List (http://pecl.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php
