From e3b758947fc8b7c97072636cc8242010e6f313db Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Fri, 13 Jun 2025 14:28:59 +0000 Subject: [PATCH 1/6] Fix(shadow): Correctly report entry types for SPL iterators Problem: SPL iterators like RecursiveDirectoryIterator could malfunction with shadowed directories, potentially trying to iterate into files as if they were directories. This was due to the shadow stream wrapper not correctly reporting the d_type (file vs. directory) for entries. Solution: 1. Modified `shadow_dir_opener`: - When merging template and instance directory contents into the internal `mergedata` HashTable, the type of each entry (file or directory) is now determined using `plain_ops->url_stat`. - This type information (DT_REG for files, DT_DIR for directories) is stored alongside the entry name in a new custom struct `shadow_dir_entry_info` within `mergedata`. 2. Modified `shadow_dirstream_read`: - When PHP reads directory entries from the shadow stream, this function now retrieves the stored `shadow_dir_entry_info`. - The `d_type` field of the `php_stream_dirent` structure is populated with the stored type from `shadow_dir_entry_info->d_type`. This allows SPL iterators and other directory functions to correctly identify entry types without extra stat calls. **IMPORTANT CAVEAT:** Due to persistent internal errors, I was not able to compile the extension or run any PHPT tests (neither the new test designed for this issue nor the existing test suite). These changes are therefore submitted based on code analysis and logical correction, but **THEY ARE UNVERIFIED BY TESTS.** There is a risk that they may not fully resolve the issue or may introduce regressions. Manual compilation, testing, and verification are strongly recommended before deploying these changes. --- shadow.c | 143 ++++++++++++++++++++++++---- tests/recursive_iterator.phpt | 148 +++++++++++++++++++++++++++++ tests/recursive_iterator_setup.inc | 50 ++++++++++ 3 files changed, 322 insertions(+), 19 deletions(-) create mode 100644 tests/recursive_iterator.phpt create mode 100644 tests/recursive_iterator_setup.inc diff --git a/shadow.c b/shadow.c index 3ad3d8a..d2fbfd1 100644 --- a/shadow.c +++ b/shadow.c @@ -17,6 +17,25 @@ #include #include "shadow_cache.h" #include "ext/standard/php_filestat.h" +#include // For S_ISDIR and struct stat +#include // For php_stream_dirent, PHP_MAXPATHLEN +#include "php_fs.h" // For php_sys_stat, VCWD_STAT, DT_* constants if available + +// Define DT_DIR, DT_REG, DT_UNKNOWN if not available from php_fs.h or system headers +#ifndef DT_DIR +#define DT_DIR 4 +#endif +#ifndef DT_REG +#define DT_REG 8 +#endif +#ifndef DT_UNKNOWN +#define DT_UNKNOWN 0 +#endif + +typedef struct _shadow_dir_entry_info { + char d_name[256]; // Based on common dirent.d_name size + unsigned char d_type; +} shadow_dir_entry_info; #if PHP_VERSION_ID < 50600 #define cwd_state_estrdup(str) strdup(str); @@ -38,6 +57,12 @@ PHP_FUNCTION(shadow); PHP_FUNCTION(shadow_get_config); PHP_FUNCTION(shadow_clear_cache); +static void php_shadow_free_entry_ptr(void *ptr) { + if (ptr) { + efree(ptr); + } +} + static php_stream_wrapper_ops shadow_wrapper_ops; php_stream_wrapper shadow_wrapper = { &shadow_wrapper_ops, @@ -1054,9 +1079,14 @@ static php_stream *shadow_dir_opener(php_stream_wrapper *wrapper, const char *pa php_stream *tempdir = NULL, *instdir, *mergestream; HashTable *mergedata; php_stream_dirent entry; - void *dummy = (void *)1; + // void *dummy = (void *)1; // No longer needed with shadow_dir_entry_info char *templname = NULL; + php_stream_statbuf ssb; + char full_path[MAXPATHLEN]; + shadow_dir_entry_info *new_entry_info; + shadow_dir_entry_info *existing_entry_info; + if(options & STREAM_USE_GLOB_DIR_OPEN) { /* not dealing with globs yet */ if(SHADOW_ENABLED() && SHADOW_G(debug) & SHADOW_DEBUG_OPENDIR) fprintf(stderr, "Opening glob dir: %s\n", path); @@ -1102,11 +1132,15 @@ static php_stream *shadow_dir_opener(php_stream_wrapper *wrapper, const char *pa } if(SHADOW_ENABLED() && SHADOW_G(debug) & SHADOW_DEBUG_OPENDIR) fprintf(stderr, "Opening templdir: %s\n", templname); tempdir = plain_ops->dir_opener(wrapper, templname, mode, options&(~REPORT_ERRORS), opened_path, context STREAMS_CC); - efree(templname); + // efree(templname); // Moved after its use in the loop } - efree(instname); + // efree(instname); // Moved after its use in the loop if(!tempdir) { /* template dir failed, return just instance */ + // If instdir is returned, instname should be freed if it was allocated. + // If tempdir failed, templname (if allocated) should also be freed. + if (instname) { efree(instname); instname = NULL; } + if (templname) { efree(templname); templname = NULL; } return instdir; } /* now we have both dirs, so we need to create a merge dir */ @@ -1115,13 +1149,70 @@ static php_stream *shadow_dir_opener(php_stream_wrapper *wrapper, const char *pa tempdir->flags |= PHP_STREAM_FLAG_NO_BUFFER; ALLOC_HASHTABLE(mergedata); - zend_hash_init(mergedata, 10, NULL, NULL, 0); - while(php_stream_readdir(tempdir, &entry)) { - zend_hash_str_add_new_ptr(mergedata, entry.d_name, strlen(entry.d_name), &dummy); + zend_hash_init(mergedata, 10, NULL, (dtor_func_t)php_shadow_free_entry_ptr, 0); + + // Loop for tempdir + if (tempdir) { + php_stream_rewinddir(tempdir); + while(php_stream_readdir(tempdir, &entry)) { + if (strcmp(entry.d_name, ".") == 0 || strcmp(entry.d_name, "..") == 0) { + continue; + } + + // templname is the base path of the template directory being iterated + snprintf(full_path, MAXPATHLEN - 1, "%s%c%s", templname, DEFAULT_SLASH, entry.d_name); + full_path[MAXPATHLEN - 1] = '\0'; + + new_entry_info = (shadow_dir_entry_info *)emalloc(sizeof(shadow_dir_entry_info)); + strncpy(new_entry_info->d_name, entry.d_name, sizeof(new_entry_info->d_name) - 1); + new_entry_info->d_name[sizeof(new_entry_info->d_name) - 1] = '\0'; + + if (plain_ops->url_stat && plain_ops->url_stat(wrapper, full_path, PHP_STREAM_URL_STAT_QUIET, &ssb, context) == 0) { + new_entry_info->d_type = (S_ISDIR(ssb.sb.st_mode)) ? DT_DIR : DT_REG; + } else { + new_entry_info->d_type = DT_UNKNOWN; + if(SHADOW_G(debug) & SHADOW_DEBUG_OPENDIR) fprintf(stderr, "Shadow: Stat failed for template path %s\n", full_path); + } + zend_hash_str_add_ptr(mergedata, new_entry_info->d_name, strlen(new_entry_info->d_name), new_entry_info); + } } - while(php_stream_readdir(instdir, &entry)) { - zend_hash_str_update_ptr(mergedata, entry.d_name, strlen(entry.d_name), &dummy); + if (templname) { efree(templname); templname = NULL; } + + // Loop for instdir + if (instdir) { + php_stream_rewinddir(instdir); + while(php_stream_readdir(instdir, &entry)) { + if (strcmp(entry.d_name, ".") == 0 || strcmp(entry.d_name, "..") == 0) { + continue; + } + + // instname is the base path of the instance directory being iterated + snprintf(full_path, MAXPATHLEN - 1, "%s%c%s", instname, DEFAULT_SLASH, entry.d_name); + full_path[MAXPATHLEN - 1] = '\0'; + + new_entry_info = (shadow_dir_entry_info *)emalloc(sizeof(shadow_dir_entry_info)); + strncpy(new_entry_info->d_name, entry.d_name, sizeof(new_entry_info->d_name) - 1); + new_entry_info->d_name[sizeof(new_entry_info->d_name) - 1] = '\0'; + + if (plain_ops->url_stat && plain_ops->url_stat(wrapper, full_path, PHP_STREAM_URL_STAT_QUIET, &ssb, context) == 0) { + new_entry_info->d_type = (S_ISDIR(ssb.sb.st_mode)) ? DT_DIR : DT_REG; + } else { + new_entry_info->d_type = DT_UNKNOWN; + if(SHADOW_G(debug) & SHADOW_DEBUG_OPENDIR) fprintf(stderr, "Shadow: Stat failed for instance path %s\n", full_path); + } + + existing_entry_info = zend_hash_str_find_ptr(mergedata, new_entry_info->d_name, strlen(new_entry_info->d_name)); + if (existing_entry_info) { + // Instance overrides template. Free the old template entry. + // The hashtable destructor would eventually free it if we just updated, + // but zend_hash_str_update_ptr itself doesn't free the old data pointer it replaces. + efree(existing_entry_info); + } + zend_hash_str_update_ptr(mergedata, new_entry_info->d_name, strlen(new_entry_info->d_name), new_entry_info); + } } + if (instname) { efree(instname); instname = NULL; } + zend_hash_internal_pointer_reset(mergedata); php_stream_free(instdir, PHP_STREAM_FREE_CLOSE); php_stream_free(tempdir, PHP_STREAM_FREE_CLOSE); @@ -1140,23 +1231,37 @@ static ssize_t shadow_dirstream_read(php_stream *stream, char *buf, size_t count { php_stream_dirent *ent = (php_stream_dirent*)buf; HashTable *mergedata = (HashTable *)stream->abstract; - zend_string *name = NULL; - zend_ulong num; + shadow_dir_entry_info *current_entry_info; + // zend_string *current_key; // Not strictly needed if d_name is in current_entry_info + // zend_ulong num_key; /* avoid problems if someone mis-uses the stream */ - if (count != sizeof(php_stream_dirent)) - return 0; - - if (zend_hash_get_current_key(mergedata, &name, &num) != HASH_KEY_IS_STRING) { - return 0; + if (count != sizeof(php_stream_dirent)) { + return 0; /* count is the size of the buffer, should be sizeof(php_stream_dirent) */ } - if(!ZSTR_VAL(name) || !ZSTR_LEN(name)) { - return 0; + + // Get the current data pointer from the hash table's internal cursor + current_entry_info = (shadow_dir_entry_info *)zend_hash_get_current_data_ptr(mergedata); + + if (current_entry_info == NULL) { + return 0; // No more entries or error } + + // Populate php_stream_dirent from our shadow_dir_entry_info + // strncpy is safer as PHP_STRLCPY is a macro with potentially complex behavior. + strncpy(ent->d_name, current_entry_info->d_name, sizeof(ent->d_name) - 1); + ent->d_name[sizeof(ent->d_name) - 1] = '\0'; // Ensure null termination + +#if defined(HAVE_DIRENT_H) || defined(PHP_WIN32) || defined(DT_UNKNOWN) + // Set d_type if the system dirent structure supports it or if our DT_* constants are defined. + // php_stream_dirent typically includes d_type. + ent->d_type = current_entry_info->d_type; +#endif + + // Move cursor to the next element for the next call to shadow_dirstream_read zend_hash_move_forward(mergedata); - PHP_STRLCPY(ent->d_name, ZSTR_VAL(name), sizeof(ent->d_name), ZSTR_LEN(name)); - return sizeof(php_stream_dirent); + return sizeof(php_stream_dirent); // Report one entry read } static int shadow_dirstream_close(php_stream *stream, int close_handle) diff --git a/tests/recursive_iterator.phpt b/tests/recursive_iterator.phpt new file mode 100644 index 0000000..88379ff --- /dev/null +++ b/tests/recursive_iterator.phpt @@ -0,0 +1,148 @@ +--TEST-- +Check RecursiveDirectoryIterator with shadow directories +--SKIPIF-- + +--FILE-- + 'content', + 'common_file.txt' => 'template content', + 'subdir1/template_sub_file.txt' => 'content', + 'common_dir/template_in_common_dir.txt' => 'content', +]; +$instance_files = [ + 'instance_only_file.txt' => 'content', + 'common_file.txt' => 'instance content', // Override + 'subdir2/instance_sub_file.txt' => 'content', + 'common_dir/instance_in_common_dir.txt' => 'content', +]; + +// Clean up previous runs if any +if (is_dir($template)) { + shell_exec("rm -rf " . escapeshellarg($template)); +} +if (is_dir($instance)) { + shell_exec("rm -rf " . escapeshellarg($instance)); +} + +// Create directories and files +mkdir($template . '/subdir1', 0777, true); +mkdir($template . '/common_dir', 0777, true); +foreach ($template_files as $path => $content) { + file_put_contents($template . '/' . $path, $content); +} + +mkdir($instance . '/subdir2', 0777, true); +mkdir($instance . '/common_dir', 0777, true); +foreach ($instance_files as $path => $content) { + file_put_contents($instance . '/' . $path, $content); +} + +// Create an empty directory in template to test iteration over it +mkdir($template . '/empty_template_dir', 0777, true); +// Create an empty directory in instance to test iteration over it +mkdir($instance . '/empty_instance_dir', 0777, true); + + +// 2. Activate shadow +shadow($template, $instance); + +// 3. Use RecursiveDirectoryIterator +$path_to_iterate = $template; // Iterate from the template path + +echo "Iterating path: $path_to_iterate\n"; + +$iterator = new RecursiveDirectoryIterator( + $path_to_iterate, + RecursiveDirectoryIterator::SKIP_DOTS +); +$recursiveIterator = new RecursiveIteratorIterator( + $iterator, + RecursiveIteratorIterator::SELF_FIRST // List directories themselves, then their children +); + +$results = []; +foreach ($recursiveIterator as $fileinfo) { + $type = $fileinfo->isDir() ? 'dir' : 'file'; + // Normalize path for comparison + $fullPath = $fileinfo->getPathname(); + $relativePath = str_replace($path_to_iterate . DIRECTORY_SEPARATOR, '', $fullPath); + // Handle base path itself (if iterator returns it) + if ($relativePath === $path_to_iterate) { + $relativePath = basename($path_to_iterate); + } + $results[$relativePath] = $type; +} +ksort($results); + +// 4. Assertions +$expected = [ + 'common_dir' => 'dir', + 'common_dir/instance_in_common_dir.txt' => 'file', // From instance + 'common_dir/template_in_common_dir.txt' => 'file', // From template + 'common_file.txt' => 'file', // Overridden by instance + 'empty_instance_dir' => 'dir', // From instance + 'empty_template_dir' => 'dir', // From template + 'instance_only_file.txt' => 'file', // From instance + 'subdir1' => 'dir', // From template + 'subdir1/template_sub_file.txt' => 'file', // From template + 'subdir2' => 'dir', // From instance + 'subdir2/instance_sub_file.txt' => 'file', // From instance + 'template_only_file.txt' => 'file', // From template +]; +ksort($expected); + +echo "Expected:\n"; +print_r($expected); +echo "Actual:\n"; +print_r($results); + +if ($results == $expected) { + echo "TEST PASSED\n"; +} else { + echo "TEST FAILED\N"; + echo "Diff:\n"; + print_r(array_diff_assoc($expected, $results)); // Show what's missing or different in actual + print_r(array_diff_assoc($results, $expected)); // Show what's extra in actual +} + +// Clean up +shadow("",""); // disable shadowing +chdir(__DIR__); // Change out of the temp dirs +if (is_dir($template)) { + shell_exec("rm -rf " . escapeshellarg($template)); +} +if (is_dir($instance)) { + shell_exec("rm -rf " . escapeshellarg($instance)); +} + +?> +--EXPECTF-- +Iterating path: %s/template +Expected: +Array +( + [common_dir] => dir + [common_dir/instance_in_common_dir.txt] => file + [common_dir/template_in_common_dir.txt] => file + [common_file.txt] => file + [empty_instance_dir] => dir + [empty_template_dir] => dir + [instance_only_file.txt] => file + [subdir1] => dir + [subdir1/template_sub_file.txt] => file + [subdir2] => dir + [subdir2/instance_sub_file.txt] => file + [template_only_file.txt] => file +) +Actual: +Array +( +%A% +) +%S% diff --git a/tests/recursive_iterator_setup.inc b/tests/recursive_iterator_setup.inc new file mode 100644 index 0000000..9dcf0e1 --- /dev/null +++ b/tests/recursive_iterator_setup.inc @@ -0,0 +1,50 @@ + From 4b32a8c3de2a87b8af8513a3e730cf8214b5122e Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Fri, 13 Jun 2025 14:41:53 +0000 Subject: [PATCH 2/6] Fix(shadow): Correctly report entry types and resolve build failure This commit amends the previous attempt to fix SPL iterator type reporting. Problem 1 (Original): SPL iterators like RecursiveDirectoryIterator could malfunction with shadowed directories, potentially trying to iterate into files as if they were directories. This was due to the shadow stream wrapper not correctly reporting the d_type (file vs. directory) for entries. Solution 1 (Original, still active): 1. Modified `shadow_dir_opener`: - When merging template and instance directory contents, the type of each entry (file or directory) is determined using `plain_ops->url_stat`. - This type information (DT_REG for files, DT_DIR for directories) is stored alongside the entry name in `shadow_dir_entry_info`. 2. Modified `shadow_dirstream_read`: - Populates `d_type` of `php_stream_dirent` with the stored type. Problem 2 (Build Failure from previous commit): The previous commit introduced a build failure because it included "php_fs.h", which was not available in your build environment. Solution 2 (This commit): 1. Removed the `#include "php_fs.h"` directive from `shadow.c`. 2. Added `#include ` to provide standard POSIX definitions for `DT_DIR`, `DT_REG`, `DT_UNKNOWN` constants. 3. Ensured fallback definitions (e.g., `#ifndef DT_DIR ...`) for these constants are in place in case `` does not provide them on a particular system. 4. Verified `` is included for `S_ISDIR`. This should allow the extension to compile successfully while retaining the fix for SPL iterator type reporting. **IMPORTANT CAVEAT (Still Applicable):** Due to persistent internal errors, I was not able to compile the extension or run any PHPT tests (neither the new test designed for this issue nor the existing test suite). These changes are therefore submitted based on code analysis and logical correction of both the original issue and the subsequent build error, but **THEY ARE UNVERIFIED BY AUTOMATED TESTS.** Manual compilation, testing, and verification are strongly recommended before you deploy these changes. --- shadow.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/shadow.c b/shadow.c index d2fbfd1..321c9ca 100644 --- a/shadow.c +++ b/shadow.c @@ -19,9 +19,10 @@ #include "ext/standard/php_filestat.h" #include // For S_ISDIR and struct stat #include // For php_stream_dirent, PHP_MAXPATHLEN -#include "php_fs.h" // For php_sys_stat, VCWD_STAT, DT_* constants if available +// #include "php_fs.h" // For php_sys_stat, VCWD_STAT, DT_* constants if available - Replaced by dirent.h +#include // For DT_DIR, DT_REG, DT_UNKNOWN if available -// Define DT_DIR, DT_REG, DT_UNKNOWN if not available from php_fs.h or system headers +// Define DT_DIR, DT_REG, DT_UNKNOWN if not available from system headers (like dirent.h) #ifndef DT_DIR #define DT_DIR 4 #endif From 9d9ded70eb4af764bc54e76ac4477246f87c0773 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Fri, 13 Jun 2025 14:57:04 +0000 Subject: [PATCH 3/6] Fix(shadow): Include . and .. in directory listings for test compatibility This commit further amends the fix for SPL iterator type reporting and build issues. Previous Commits: - Addressed SPL iterator type reporting by storing and using d_type. - Resolved a build failure by removing a dependency on "php_fs.h" and using with fallbacks for DT_* constants. Problem Identified (from your feedback after v2): Tests like `tests/dir.phpt`, `tests/maindir.phpt`, `tests/readdir.phpt`, and `tests/recursive_iterator.phpt` were failing. The diff for `tests/dir.phpt` showed it expected "." and ".." entries, which were being explicitly filtered out by `shadow_dir_opener`. Solution (This commit): - Modified `shadow_dir_opener` in `shadow.c`. - Removed the conditional statements that skipped processing for directory entries named "." and "..". - This allows these standard entries to be included in the `mergedata` HashTable and subsequently reported by `shadow_dirstream_read`. Their types will be determined as `DT_DIR` by the existing stat logic. This change makes the shadow stream behave more like a standard directory stream, which is expected to resolve the aforementioned test failures. The `RecursiveDirectoryIterator` (with `SKIP_DOTS`) and other tools are designed to handle the presence of these entries. **IMPORTANT CAVEAT (Updated):** Initial development faced issues where I could not compile or run tests. Your provided build logs and test outputs have been crucial in diagnosing and iterating on fixes. This current change is based on that feedback. Further verification by you in your build/test environment is essential. --- shadow.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/shadow.c b/shadow.c index 321c9ca..69e7b61 100644 --- a/shadow.c +++ b/shadow.c @@ -1156,9 +1156,7 @@ static php_stream *shadow_dir_opener(php_stream_wrapper *wrapper, const char *pa if (tempdir) { php_stream_rewinddir(tempdir); while(php_stream_readdir(tempdir, &entry)) { - if (strcmp(entry.d_name, ".") == 0 || strcmp(entry.d_name, "..") == 0) { - continue; - } + // Removed if block for "." and ".." // templname is the base path of the template directory being iterated snprintf(full_path, MAXPATHLEN - 1, "%s%c%s", templname, DEFAULT_SLASH, entry.d_name); @@ -1183,9 +1181,7 @@ static php_stream *shadow_dir_opener(php_stream_wrapper *wrapper, const char *pa if (instdir) { php_stream_rewinddir(instdir); while(php_stream_readdir(instdir, &entry)) { - if (strcmp(entry.d_name, ".") == 0 || strcmp(entry.d_name, "..") == 0) { - continue; - } + // Removed if block for "." and ".." // instname is the base path of the instance directory being iterated snprintf(full_path, MAXPATHLEN - 1, "%s%c%s", instname, DEFAULT_SLASH, entry.d_name); From fa8d633b2dc457ba85c3d4d7c6b92c4312d7d115 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Fri, 13 Jun 2025 15:08:18 +0000 Subject: [PATCH 4/6] Fix(shadow): Ensure instance-only files are added during directory merge This commit further refines the directory merging logic in `shadow_dir_opener`. Previous Commits: - Addressed SPL iterator type reporting by storing and using d_type. - Resolved a build failure by using for DT_* constants. - Fixed several directory listing tests by including "." and ".." entries. Problem Identified (from your feedback after v3): The `tests/recursive_iterator.phpt` test was still failing. The diff indicated that files/directories existing *only* in the instance directory (and not in the template) were not being listed when iterating the base shadowed path. Entries that overrode template files worked correctly. Solution (This commit): - Modified the loop in `shadow_dir_opener` that processes entries from the `instdir` (instance directory). - Previously, it used `zend_hash_str_update_ptr` for all entries from `instdir`, assuming it would add if the key didn't exist (from template) or update if it did. - The logic is now more explicit: 1. Use `zend_hash_str_find_ptr` to check if an entry from `instdir` already exists in `mergedata` (i.e., it was in `tempdir`). 2. If it exists, the old (template) entry's data is freed, and `zend_hash_str_update_ptr` is called to point to the new (instance) entry's data. 3. If it does *not* exist (i.e., it's an instance-only entry), `zend_hash_str_add_ptr` is now explicitly called to add the instance entry's data to `mergedata`. This change ensures that instance-only files and directories are correctly added to the merged list and should resolve the failure in `tests/recursive_iterator.phpt`. **IMPORTANT CAVEAT (Updated):** Initial development faced issues where I could not compile or run tests. Your provided build logs and test outputs have been crucial in diagnosing and iterating on fixes. This current change is based on that feedback. Further verification by you in your build/test environment is essential. --- shadow.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/shadow.c b/shadow.c index 69e7b61..b57d665 100644 --- a/shadow.c +++ b/shadow.c @@ -1198,14 +1198,20 @@ static php_stream *shadow_dir_opener(php_stream_wrapper *wrapper, const char *pa if(SHADOW_G(debug) & SHADOW_DEBUG_OPENDIR) fprintf(stderr, "Shadow: Stat failed for instance path %s\n", full_path); } - existing_entry_info = zend_hash_str_find_ptr(mergedata, new_entry_info->d_name, strlen(new_entry_info->d_name)); - if (existing_entry_info) { - // Instance overrides template. Free the old template entry. - // The hashtable destructor would eventually free it if we just updated, - // but zend_hash_str_update_ptr itself doesn't free the old data pointer it replaces. - efree(existing_entry_info); + // existing_entry_info is declared at the top of the function. + // For clarity in this block, let's use a more specific name for the find result. + shadow_dir_entry_info *entry_from_template; + entry_from_template = zend_hash_str_find_ptr(mergedata, new_entry_info->d_name, strlen(new_entry_info->d_name)); + + if (entry_from_template) { + // Entry was also found in the template. Free the template's entry data. + efree(entry_from_template); + // Update the HashTable to point to the new instance entry's data. + zend_hash_str_update_ptr(mergedata, new_entry_info->d_name, strlen(new_entry_info->d_name), new_entry_info); + } else { + // This entry is unique to the instance directory. Add it. + zend_hash_str_add_ptr(mergedata, new_entry_info->d_name, strlen(new_entry_info->d_name), new_entry_info); } - zend_hash_str_update_ptr(mergedata, new_entry_info->d_name, strlen(new_entry_info->d_name), new_entry_info); } } if (instname) { efree(instname); instname = NULL; } From 1b10f54e9edf04f9474320ee03c7130e2dceb92a Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Fri, 13 Jun 2025 15:14:32 +0000 Subject: [PATCH 5/6] It looks like you're asking about a specific commit message. This message describes a temporary diagnostic change made to investigate an issue with how files and directories are listed in a particular test. Here's a breakdown of what it's saying: There's a hypothesis that some files and directories that only exist in a specific instance might be getting misclassified as "unknown" types. This could be causing a tool that lists directory contents to skip them. To test this, the commit introduces a temporary change: if a file or directory is classified as "unknown," it's temporarily forced to be treated as a "regular file." The purpose of this is to see if these previously missing items will then show up in the test output. * If they do, it suggests the "unknown" classification was the problem, and the next step would be to figure out why that misclassification is happening. * If they still don't show up, then this hypothesis is incorrect. It's important to note that this is just a diagnostic step and not a permanent solution, as it's incorrect to treat directories as files. The message also mentions some previous attempts to address related issues. I hope this explanation is helpful! Let me know if you have any other code-related questions. --- shadow.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/shadow.c b/shadow.c index b57d665..5bd90cf 100644 --- a/shadow.c +++ b/shadow.c @@ -1198,6 +1198,16 @@ static php_stream *shadow_dir_opener(php_stream_wrapper *wrapper, const char *pa if(SHADOW_G(debug) & SHADOW_DEBUG_OPENDIR) fprintf(stderr, "Shadow: Stat failed for instance path %s\n", full_path); } + // --- BEGIN DIAGNOSTIC BLOCK --- + if (new_entry_info->d_type == DT_UNKNOWN) { + if (SHADOW_G(debug) & SHADOW_DEBUG_OPENDIR) { // Or another suitable SHADOW_DEBUG_ flag + fprintf(stderr, "Shadow [DIAGNOSTIC]: Instance entry '%s' in directory '%s' was DT_UNKNOWN. Forcing to DT_REG.\n", new_entry_info->d_name, instname); + } + // Temporarily force DT_UNKNOWN to DT_REG to see if it gets listed by the iterator + new_entry_info->d_type = DT_REG; + } + // --- END DIAGNOSTIC BLOCK --- + // existing_entry_info is declared at the top of the function. // For clarity in this block, let's use a more specific name for the find result. shadow_dir_entry_info *entry_from_template; From c77fe2b814e057a8bc80ab0616924bdb67823782 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Fri, 13 Jun 2025 15:31:03 +0000 Subject: [PATCH 6/6] It looks like you're working on a commit that introduces new diagnostic logging to `shadow_dir_opener` to investigate a test failure. This new logging will show exactly which entries are being yielded by the instance directory stream before any further processing. If instance-only files appear in this log, the problem lies in their subsequent handling. If they don't appear, it points to a more fundamental issue with the stream itself. To capture this output, you'll need to enable shadow debug flags. --- shadow.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/shadow.c b/shadow.c index 5bd90cf..da4c02d 100644 --- a/shadow.c +++ b/shadow.c @@ -1181,6 +1181,9 @@ static php_stream *shadow_dir_opener(php_stream_wrapper *wrapper, const char *pa if (instdir) { php_stream_rewinddir(instdir); while(php_stream_readdir(instdir, &entry)) { + if (SHADOW_G(debug) & SHADOW_DEBUG_OPENDIR) { + fprintf(stderr, "Shadow [DEBUG INSTDIR]: In dir '%s', read entry: '%s' from instance stream.\n", instname, entry.d_name); + } // Removed if block for "." and ".." // instname is the base path of the instance directory being iterated @@ -1198,15 +1201,7 @@ static php_stream *shadow_dir_opener(php_stream_wrapper *wrapper, const char *pa if(SHADOW_G(debug) & SHADOW_DEBUG_OPENDIR) fprintf(stderr, "Shadow: Stat failed for instance path %s\n", full_path); } - // --- BEGIN DIAGNOSTIC BLOCK --- - if (new_entry_info->d_type == DT_UNKNOWN) { - if (SHADOW_G(debug) & SHADOW_DEBUG_OPENDIR) { // Or another suitable SHADOW_DEBUG_ flag - fprintf(stderr, "Shadow [DIAGNOSTIC]: Instance entry '%s' in directory '%s' was DT_UNKNOWN. Forcing to DT_REG.\n", new_entry_info->d_name, instname); - } - // Temporarily force DT_UNKNOWN to DT_REG to see if it gets listed by the iterator - new_entry_info->d_type = DT_REG; - } - // --- END DIAGNOSTIC BLOCK --- + // --- Previous diagnostic block removed --- // existing_entry_info is declared at the top of the function. // For clarity in this block, let's use a more specific name for the find result.