[SPL] Fix FilesystemIterator::FOLLOW_SYMLINKS bug #6695
Conversation
FilesystemIterator::FOLLOW_SYMLINKS is currently treated as a directory key mode flag, even though it does not change the way that the key during iteration is set. To address this, SPL_FILE_DIR_KEY has been changed to exclude masking FOLLOW_SYMLINKS for comparison. Addresses bug 80724.
ext/spl/spl_directory.h
Outdated
| @@ -138,7 +138,7 @@ static inline spl_filesystem_object* spl_filesystem_iterator_to_object(spl_files | |||
| #define SPL_FILE_DIR_KEY_AS_FILENAME 0x00000100 /* make RecursiveDirectoryTree::key() return getFilename() */ | |||
| #define SPL_FILE_DIR_FOLLOW_SYMLINKS 0x00000200 /* make RecursiveDirectoryTree::hasChildren() follow symlinks */ | |||
| #define SPL_FILE_DIR_KEY_MODE_MASK 0x00000F00 /* mask RecursiveDirectoryTree::key() */ | |||
| #define SPL_FILE_DIR_KEY(intern,mode) ((intern->flags&SPL_FILE_DIR_KEY_MODE_MASK)==mode) | |||
| #define SPL_FILE_DIR_KEY(intern,mode) ((intern->flags&SPL_FILE_DIR_KEY_AS_FILENAME)==mode) | |||
There was a problem hiding this comment.
I'd suggest to move SPL_FILE_DIR_FOLLOW_SYMLINKS down into the SPL_FILE_DIR_OTHERS_MASK category instead. As written, this macro doesn't really make sense.
There was a problem hiding this comment.
As in changing the value? I wasn't sure if that would be ok to do, unless it's fine since it's non BC anyway?
Currently SPL_FILE_DIR_KEY_MODE_MASK doesn't make sense, so changing the value would allow it to make sense again...
I can move it down too.
There was a problem hiding this comment.
At least for master, it's okay to change the value.
|
Looks like there's a memory leak in the added test: Most likely a pre-existing issue. |
|
This is enough to cause a leak: |
|
The leak should be fixed with 44a80b6. |
|
Thank you, I didn't spend much time looking into the leak, glad it's fixed 👍 |
makes sense again
ext/spl/spl_directory.h
Outdated
| @@ -136,12 +136,12 @@ static inline spl_filesystem_object* spl_filesystem_iterator_to_object(spl_files | |||
|
|
|||
| #define SPL_FILE_DIR_KEY_AS_PATHNAME 0x00000000 /* make RecursiveDirectoryTree::key() return getPathname() */ | |||
| #define SPL_FILE_DIR_KEY_AS_FILENAME 0x00000100 /* make RecursiveDirectoryTree::key() return getFilename() */ | |||
| #define SPL_FILE_DIR_FOLLOW_SYMLINKS 0x00000200 /* make RecursiveDirectoryTree::hasChildren() follow symlinks */ | |||
| #define SPL_FILE_DIR_KEY_MODE_MASK 0x00000F00 /* mask RecursiveDirectoryTree::key() */ | |||
| #define SPL_FILE_DIR_KEY(intern,mode) ((intern->flags&SPL_FILE_DIR_KEY_MODE_MASK)==mode) | |||
|
|
|||
| #define SPL_FILE_DIR_SKIPDOTS 0x00001000 /* Tells whether it should skip dots or not */ | |||
| #define SPL_FILE_DIR_UNIXPATHS 0x00002000 /* Whether to unixify path separators */ | |||
| #define SPL_FILE_DIR_OTHERS_MASK 0x00003000 /* mask used for get/setFlags */ | |||
There was a problem hiding this comment.
The OTHERS_MASK should now be 0x00007000 to include the FOLLOW_SYMLINKS flag as well.
There was a problem hiding this comment.
👍 Changed and updated tests.
FilesystemIterator::FOLLOW_SYMLINKS is currently treated as a directory
key mode flag, even though it does not change the way that the key
during iteration is determined. To address this, SPL_FILE_DIR_KEY has been
changed to exclude masking FOLLOW_SYMLINKS for comparison.
Addresses bug 80724, though there might be disagreement about the intent of the FOLLOW_SYMLINKS flag.
Submitted against master because this is technically a BC break.