Make WordPress Core

Opened 5 years ago

Last modified 3 years ago

#48289 reviewing defect (bug)

wp_normalize_path() breaks path_is_absolute() in Windows.

Reported by: paultgoodchild's profile paultgoodchild Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 5.2.3
Component: Filesystem API Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

Apologies in advance if this is considered correct behaviour, but it's strange that this is the case.

On Windows, when you take an absolute file system path and use wp_normalize_path() and then pass this result to path_is_absolute(), you get false instead of true.

Super simple demo:

<?php
var_dump( path_is_absolute( __FILE__ ) );
var_dump( path_is_absolute( wp_normalize_path( __FILE__ ) ) );

*nix Result:

bool(true)
bool(true)

Windows Result:

bool(true)
bool(false)

Attachments (4)

48289.diff (872 bytes) - added by paultgoodchild 5 years ago.
a git diff to fix the problem
48289.1.diff (447 bytes) - added by itowhid06 5 years ago.
updated patch using DIRECTORY_SEPARATOR
48289.2.diff (449 bytes) - added by itowhid06 5 years ago.
48289.3.diff (457 bytes) - added by itowhid06 5 years ago.
updated the regex

Download all attachments as: .zip

Change History (15)

#1 @paultgoodchild
5 years ago

A potential fix for this is to adjust the regex in the function, so instead of:

preg_match( '#^[a-zA-Z]:\\\\#', $path )

you could have

preg_match( '#^[a-zA-Z]:(\\\\|/)#', $path )

@paultgoodchild
5 years ago

a git diff to fix the problem

#2 @SergeyBiryukov
5 years ago

  • Component changed from General to Filesystem API
  • Keywords has-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 5.4

#3 @jrf
5 years ago

Had a quick look at the patch.
Suggestion: look into using the PHP constant DIRECTORY_SEPARATOR.

@itowhid06
5 years ago

updated patch using DIRECTORY_SEPARATOR

#4 @itowhid06
5 years ago

Uploaded an updated patch with suggestion of @jrf. Works for me.

#5 @jrf
5 years ago

@itowhid06 Your patch doesn't address the problem as it only checks for the separator, but no longer checks whether something is an absolute path starting with C: (as in drive letter).

Last edited 5 years ago by jrf (previous) (diff)

@itowhid06
5 years ago

@itowhid06
5 years ago

updated the regex

#6 @paultgoodchild
5 years ago

I'd need to take a much closer look at the diffs, but how does such a complex IF solve this, unless I'm missing something.

I know the DIRECTORY SEPARATOR constant certainly cannot fix this problem, because of the way normalize works.

#7 @paultgoodchild
5 years ago

This ticket has the risk of being waylaid as no-one has looked at it in a while.

The suggestions around the use of DIRECTORY_SEPARATOR don't work as they don't understand the underlying problem. It's got nothing to do with DIRECTORY_SEPARATOR because the the issue stems from the use of wp_normalize_path() function, which has nothing to do with DIRECTORY_SEPARATOR.

I believe my original patch solves this problem and is simple, effective and doesn't affect any of the other processing in the problem-function.

Would be good if someone could take a deeper look at this and verify the simple patch either way.
Thanks!

#8 @SergeyBiryukov
5 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#9 @apedog
5 years ago

Function path_is_absolute() should handle paths with mixed slashes when installed on Windows.
PHP constant DIRECTORY_SEPARATOR is not relevant to this issue.

#10 @audrasjb
5 years ago

  • Milestone changed from 5.4 to Future Release

Hi,

Looks like the patch still needs unit tests.
With 5.4 Beta 3 approaching and the Beta period reserved for bugs introduced during the cycle, this is being moved to Future Release. If any maintainer or committer feels this should be included or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#11 @nlpro
3 years ago

Perhaps the description of the path_is_absolute() function as provided in the Code Reference on developer.wordpress.org:

Test if a given filesystem path is absolute.

should be changed to something like:

Test if a given (non normalized) filesystem path is absolute.

It seems WordPress Core distinguishes between functional (as is) paths and (normalized) display/comparable paths.

Note: See TracTickets for help on using tickets.