Re: Interesting APC behaviour

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.