BBC: perl-5.34.0 broke Path::Class (Windows only, AFAICT)

[sisyphus]

Module:
Path-Class-0.37

Description

On MS Windows with perl-5.34.0 or later, we find that during the the running of the test suite. t/01-basic.t fails with:

t/01-basic.t .......... 1/78 Can't call method "relative" on an undefined value
at C:\sisyphusion\Path-Class-0.37\blib\lib/Path/Class/Entity.pm line 76.
t/01-basic.t .......... Dubious, test returned 22 (wstat 5632, 0x1600)

Steps to Reproduce

On MS Windows with perl-5.34.0 or later, run:
cpan -i Path::Class

I believe it's probably up to Path::Class to fix this issue and there's a bug report filed against Path::Class at:
kenahoo/Path-Class#55

t/01-basic.t innovatively tries to get File::Spec to provide 'Unix' path formatting on Windows by resetting $^O to 'Unix', in a BEGIN{} block at the start of the file.
This is actually quite successful until we get to perl-5.34.0. (The breakage, of course, probably occurred somewhere in the 5.33 devel cycle.)

I'm assuming that if File::Spec had really wanted to provide that capability, then it would have come up with something better than clobbering $^O.
If someone can confirm that there's nothing here for the perl developers to attend to, then we can close this issue and move on.

There's also some additional discussion and observations at https://www.perlmonks.org/?node_id=11145885

Perl configuration

Summary of my perl5 (revision 5 version 34 subversion 0) configuration:
   
  Platform:
    osname=MSWin32
    osvers=6.1.7601
    archname=MSWin32-x64-multi-thread
    uname=''
    config_args='undef'
    hint=recommended
    useposix=true
    d_sigaction=undef
    useithreads=define
    usemultiplicity=define
    use64bitint=define
    use64bitall=undef
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
  Compiler:
    cc='gcc'
    ccflags =' -DWIN32 -DWIN64 -fdiagnostics-color=never -DPERL_TEXTMODE_SCRIPTS -DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -DUSE_PERLIO -D__USE_MINGW_ANSI_STDIO -fwrapv -fno-strict-aliasing -mms-bitfields'
    optimize='-s -O2'
    cppflags='-DWIN32'
    ccversion=''
    gccversion='10.2.0'
    gccosandvers=''
    intsize=4
    longsize=4
    ptrsize=8
    doublesize=8
    byteorder=12345678
    doublekind=3
    d_longlong=define
    longlongsize=8
    d_longdbl=define
    longdblsize=16
    longdblkind=3
    ivtype='long long'
    ivsize=8
    nvtype='double'
    nvsize=8
    Off_t='long long'
    lseeksize=8
    alignbytes=8
    prototype=define
  Linker and Libraries:
    ld='g++'
    ldflags ='-s -L"C:\perl-5.34.0\lib\MSWin32-x64-multi-thread\CORE" -L"C:\_64\winlibs-gcc-1020\mingw64\lib" -L"C:\_64\winlibs-gcc-1020\mingw64\x86_64-w64-mingw32\lib" -L"C:\_64\winlibs-gcc-1020\mingw64\lib\gcc\x86_64-w64-mingw32\10.2.0"'
    libpth=C:\_64\winlibs-gcc-1020\mingw64\lib C:\_64\winlibs-gcc-1020\mingw64\x86_64-w64-mingw32\lib C:\_64\winlibs-gcc-1020\mingw64\lib\gcc\x86_64-w64-mingw32\10.2.0 C:\_64\msys_1020\1.0\local\lib
    libs= -lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 -ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr -lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32
    perllibs= -lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 -ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr -lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32
    libc=
    so=dll
    useshrplib=true
    libperl=libperl534.a
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_win32.xs
    dlext=dll
    d_dlsymun=undef
    ccdlflags=' '
    cccdlflags=' '
    lddlflags='-mdll -s -L"C:\perl-5.34.0\lib\MSWin32-x64-multi-thread\CORE" -L"C:\_64\winlibs-gcc-1020\mingw64\lib" -L"C:\_64\winlibs-gcc-1020\mingw64\x86_64-w64-mingw32\lib" -L"C:\_64\winlibs-gcc-1020\mingw64\lib\gcc\x86_64-w64-mingw32\10.2.0"'


Characteristics of this binary (from libperl): 
  Compile-time options:
    HAS_TIMES
    HAVE_INTERP_INTERN
    MULTIPLICITY
    PERLIO_LAYERS
    PERL_COPY_ON_WRITE
    PERL_DONT_CREATE_GVSV
    PERL_IMPLICIT_CONTEXT
    PERL_IMPLICIT_SYS
    PERL_MALLOC_WRAP
    PERL_OP_PARENT
    PERL_PRESERVE_IVUV
    USE_64_BIT_INT
    USE_ITHREADS
    USE_LARGE_FILES
    USE_LOCALE
    USE_LOCALE_COLLATE
    USE_LOCALE_CTYPE
    USE_LOCALE_NUMERIC
    USE_LOCALE_TIME
    USE_PERLIO
    USE_PERL_ATOF
  Built under MSWin32
  Compiled at May 21 2021 17:06:16
  @INC:
    C:/perl-5.34.0/site/lib/MSWin32-x64-multi-thread
    C:/perl-5.34.0/site/lib
    C:/perl-5.34.0/lib/MSWin32-x64-multi-thread
    C:/perl-5.34.0/lib

[kenahoo]

Do we know what actual change in perl or File::Spec caused the difference in behavior?

[Leont]

Do we know what actual change in perl or File::Spec caused the difference in behavior?

It kind of has to be Cwd::realpath, my best guess would be c8c3675 but I can't confirm that as I don't run Windows.

[sisyphus]

my best guess would be c8c3675 but I can't confirm that as I don't run Windows

I probably won't be able to test this until Monday.

[sisyphus]

I've taken a clone of current blead (commit c870f3e) and reverted c8c3675 by manually editing Cwd.xs.

I then built this perl and found that the perl test suite still passed all tests.
I then installed that perl, but Path-Class-0.37 still fails in exactly the same way.

One thing I don't understand is that when I build Path-Class-0.37 on perl-5.34 or 5.36 by running perl Makefile.PL followed by make then t/01-basic.t fails in different ways - depending upon whether I run make test or perl -Mblib t/01-basic.t.
When I run the former, Cwd.pm sees $^O as "Unix", but when I run the latter Cwd.pm sees $^O as "MSWin32".

Anyway - Path::Class is fine with PathTools-3.78, but is broken byPathTools-3.80.
Cwd.pm did not alter between those 2 versions (except for version numbers), so if the problem is in Cwd, then it must be in the XS.
Here's the diff of Cwd.xs between 3.78 and 3.80.

--- \comp-830\perl-5.32.1\dist\PathTools\Cwd.xs	2020-12-18 21:04:36 +1100
+++ \comp-1020\perl-5.34.0\dist\PathTools\Cwd.xs	2021-01-21 10:04:44 +1100
@@ -84,6 +84,9 @@
 	unsigned symlinks;
 	int serrno;
 	char remaining[MAXPATHLEN], next_token[MAXPATHLEN];
+#ifdef PERL_IMPLICIT_SYS
+        dTHX;
+#endif
 
 	serrno = errno;
 	symlinks = 0;
@@ -119,15 +122,24 @@
 
             p = strchr(remaining, '/');
             s = p ? p : remaining + remaining_len;
+
             if ((STRLEN)(s - remaining) >= (STRLEN)sizeof(next_token)) {
                 errno = ENAMETOOLONG;
                 return (NULL);
             }
             memcpy(next_token, remaining, s - remaining);
             next_token[s - remaining] = '\0';
-            remaining_len -= s - remaining;
-            if (p != NULL)
-                memmove(remaining, s + 1, remaining_len + 1);
+
+            /* shift first component off front of path, including '/' */
+            if (p) {
+                s++; /* skip '/' */
+                remaining_len -= s - remaining;
+                /* the +1 includes the trailing '\0' */
+                memmove(remaining, s, remaining_len + 1);
+            }
+            else
+                remaining_len = 0;
+
             if (resolved[resolved_len - 1] != '/') {
                 if (resolved_len + 1 >= MAXPATHLEN) {
                     errno = ENAMETOOLONG;
@@ -166,8 +178,8 @@
             }
 #if defined(HAS_LSTAT) && defined(HAS_READLINK) && defined(HAS_SYMLINK)
             {
-                struct stat sb;
-                if (lstat(resolved, &sb) != 0) {
+                Stat_t sb;
+                if (PerlLIO_lstat(resolved, &sb) != 0) {
                     if (errno == ENOENT && p == NULL) {
                         errno = serrno;
                         return (resolved);
@@ -182,7 +194,7 @@
                         errno = ELOOP;
                         return (NULL);
                     }
-                    slen = readlink(resolved, symlink, sizeof(symlink) - 1);
+                    slen = PerlLIO_readlink(resolved, symlink, sizeof(symlink) - 1);
                     if (slen < 0)
                         return (NULL);
                     symlink[slen] = '\0';

Apart from the section that I've already tried reverting, is there something else that looks suspicious in that ?
Interestingly #if defined(HAS_LSTAT) && defined(HAS_READLINK) && defined(HAS_SYMLINK) which was FALSE in perl-5.32.1 and earlier, is TRUE in 5.34.0 and later.

Cheers,
Rob

[kenahoo]

Interestingly #if defined(HAS_LSTAT) && defined(HAS_READLINK) && defined(HAS_SYMLINK) which was FALSE in perl-5.32.1 and earlier, is TRUE in 5.34.0 and later.

That might be the most interesting fact, actually. Perhaps this branch was never exercised on Windows before, and now it is, and causes the failures we're seeing.

[Leont]

Interestingly #if defined(HAS_LSTAT) && defined(HAS_READLINK) && defined(HAS_SYMLINK) which was FALSE in perl-5.32.1 and earlier, is TRUE in 5.34.0 and later.

I guess that means PerlLIO_lstat gets confused and returns a failure, in which case the whole function returns NULL.

[sisyphus]

I guess that means PerlLIO_lstat gets confused and returns a failure

I inserted some warn() statements in Cwd.xs and established that PerlLIO_lstat(resolved, &sb) returns a non-zero value, and the condition(errno == ENOENT && p == NULL) is FALSE - and we immediately fall through to the explicit return (NULL); (at line 193 of the current blead version of Cwd.xs).

I've no idea what needs to be done.

[Leont]

I've no idea what needs to be done.

TBH I think this needs to get fixed on the Path::Class side.

[kenahoo]

TBH I think this needs to get fixed on the Path::Class side.

Not IMO. I think the Path::Class test exposed a regression in either perl or Cwd that needs to be understood first.

[Leont]

I think the Path::Class test exposed a regression in either perl or Cwd that needs to be understood first.

I think you voided the warranty the moment you did BEGIN { $^O = 'Unix' }. The obvious solution is to change the tests to use File::Spec instead of hardcoding unix paths.

[kenahoo]

I think you voided the warranty the moment you did BEGIN { $^O = 'Unix' }. The obvious solution is to change the tests to use File::Spec instead of hardcoding unix paths.

That is indeed obvious, but it's not the end of the story.

I understand I can fix the Path::Class tests by doing that, and I will do so. If nobody on the perl core team wants to find out what actually broke in the new perl release, and how that could affect other code besides Path::Class, then I guess that's all we'll do. I'm hoping that's not the case.

So I agree that the Path::Class tests should change in this fashion. But when I find a trout in my milk, I don't just discard it and declare everything fine, I want to understand how the trout got in the milk.

To push the animals-in-odd-environments metaphors farther - for the moment I consider the Path::Class to be a canary in the coal mine, indicating that something interface-incompatible was done to perl that breaks code, even if such code is doing odd things. Things that previously worked should in general not stop working with a new perl release unless an announcement was made about backward compatibility, full stop.

[Leont]

I inserted some warn() statements in Cwd.xs and established that PerlLIO_lstat(resolved, &sb) returns a non-zero value, and the condition(errno == ENOENT && p == NULL) is FALSE - and we immediately fall through to the explicit return (NULL); (at line 193 of the current blead version of Cwd.xs).

What does that resolved that PerlLIO_lstat/win32_lstat trips over look like?

[xenu]

What does that resolved that PerlLIO_lstat/win32_lstat trips over look like?

For example C:\Users\xenu\Documents\git\Path-Class/C:\Users\xenu\Documents\git\Path-Class.

It seems the problem is that the unix version of Cwd::realpath started validating paths on Windows when lstat became available in Perl 5.34.

Perl 5.32:

> perl -e 'BEGIN{$^O="Unix"} use File::Spec; use Cwd; print Cwd::realpath(Cwd::cwd()) // "<undef>"'
C:\Users\xenu\Documents\git\Path-Class/C:\Users\xenu\Documents\git\Path-Class
> perl -e 'BEGIN{$^O="Unix"} use File::Spec; use Cwd; print File::Spec->abs2rel(Cwd::realpath(Cwd::cwd()))'
C:\Users\xenu\Documents\git\Path-Class

blead:

> perl -e 'BEGIN{$^O="Unix"} use File::Spec; use Cwd; print Cwd::realpath(Cwd::cwd()) // "<undef>"'
<undef>