Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cwd: different return values between pure perl and XS variants #16338

Closed
p5pRT opened this issue Dec 23, 2017 · 22 comments
Closed

Cwd: different return values between pure perl and XS variants #16338

p5pRT opened this issue Dec 23, 2017 · 22 comments
Labels

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Dec 23, 2017

Migrated from rt.perl.org#132648 (status was 'resolved')

Searchable as RT132648$

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 23, 2017

From @eserte

This is a bug report for perl from slaven@​rezic.de,
generated with the help of perlbug 1.40 running under perl 5.26.1.


The functions getcwd and abs_path return different values for the
XS and pure perl variants if a directory is non-existent. The XS
variant returns undef, while the PP variant returns an empty string.

A test program​:

#!/usr/bin/perl

use strict;
use warnings;
use Test​::More 'no_plan';
use File​::Temp 'tempdir';
use Cwd 'getcwd', 'abs_path';

my $tmp = tempdir(CLEANUP => 1);
mkdir "$tmp/testdir" or die $!;
chdir "$tmp/testdir" or die $!;
rmdir "$tmp/testdir" or die $!;
is getcwd, undef, 'XS getcwd on non-existent directory';
is abs_path("."), undef, 'XS abs_path on non-existent directory';
{
  no warnings 'redefine';
  # force pure perl variants
  local *Cwd​::abs_path = \&Cwd​::_perl_abs_path;
  local *Cwd​::getcwd = \&Cwd​::_perl_getcwd;
  is Cwd​::getcwd, undef, 'PP getcwd on non-existent directory';
  is Cwd​::abs_path("."), undef, 'PP abs_path on non-existent directory';
}
chdir "/";

__END__

Output​:

ok 1 - XS getcwd on non-existent directory
ok 2 - XS abs_path on non-existent directory
readdir(./..)​: No such file or directory at /tmp/cwd.pl line 20.
not ok 3 - PP getcwd on non-existent directory
# Failed test 'PP getcwd on non-existent directory'
# at /tmp/cwd.pl line 20.
# got​: ''
# expected​: undef
readdir(./..)​: No such file or directory at /tmp/cwd.pl line 21.
not ok 4 - PP abs_path on non-existent directory
# Failed test 'PP abs_path on non-existent directory'
# at /tmp/cwd.pl line 21.
# got​: ''
# expected​: undef
1..4
# Looks like you failed 2 tests of 4.

Additionally, the PP variants are quite loud while the XS variant is
silent in this situation. But this might be acceptable.

Probably the fix is to change all "return ''" statements in _perl_abs_path
to "return undef".

Regards,
  Slaven



Flags​:
  category=library
  severity=low
  module=Cwd


Site configuration information for perl 5.26.1​:

Configured by eserte at Sat Sep 23 09​:36​:11 CEST 2017.

Summary of my perl5 (revision 5 version 26 subversion 1) configuration​:
 
  Platform​:
  osname=linux
  osvers=3.16.0-4-amd64
  archname=x86_64-linux
  uname='linux cabulja 3.16.0-4-amd64 #1 smp debian 3.16.39-1+deb8u2 (2017-03-07) x86_64 gnulinux '
  config_args='-ds -e -Dprefix=/opt/perl-5.26.1 -Dcf_email=srezic@​cpan.org'
  hint=recommended
  useposix=true
  d_sigaction=define
  useithreads=undef
  usemultiplicity=undef
  use64bitint=define
  use64bitall=define
  uselongdouble=undef
  usemymalloc=n
  default_inc_excludes_dot=define
  bincompat5005=undef
  Compiler​:
  cc='cc'
  ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2'
  optimize='-O2'
  cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
  ccversion=''
  gccversion='4.9.2'
  gccosandvers=''
  intsize=4
  longsize=8
  ptrsize=8
  doublesize=8
  byteorder=12345678
  doublekind=3
  d_longlong=define
  longlongsize=8
  d_longdbl=define
  longdblsize=16
  longdblkind=3
  ivtype='long'
  ivsize=8
  nvtype='double'
  nvsize=8
  Off_t='off_t'
  lseeksize=8
  alignbytes=8
  prototype=define
  Linker and Libraries​:
  ld='cc'
  ldflags =' -fstack-protector-strong -L/usr/local/lib'
  libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/4.9/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib
  libs=-lpthread -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
  perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  libc=libc-2.19.so
  so=so
  useshrplib=false
  libperl=libperl.a
  gnulibc_version='2.19'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs
  dlext=so
  d_dlsymun=undef
  ccdlflags='-Wl,-E'
  cccdlflags='-fPIC'
  lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector-strong'


@​INC for perl 5.26.1​:
  /opt/perl-5.26.1/lib/site_perl/5.26.1/x86_64-linux
  /opt/perl-5.26.1/lib/site_perl/5.26.1
  /opt/perl-5.26.1/lib/5.26.1/x86_64-linux
  /opt/perl-5.26.1/lib/5.26.1


Environment for perl 5.26.1​:
  HOME=/home/eserte
  LANG=en_US.UTF-8
  LANGUAGE (unset)
  LD_LIBRARY_PATH (unset)
  LOGDIR (unset)
  PATH=/usr/local/bin​:/usr/bin​:/bin​:/usr/local/sbin​:/usr/sbin​:/sbin​:/home/eserte/bin/linux-gnu​:/home/eserte/bin/sh​:/home/eserte/bin​:/home/eserte/bin/pistachio-perl/bin​:/usr/games​:/home/eserte/devel
  PERLDOC=-MPod​::Perldoc​::ToTextOverstrike
  PERL_BADLANG (unset)
  SHELL=/bin/zsh

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 24, 2017

From zefram@fysh.org

Fixed in commit d2e38af.

-zefram

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 24, 2017

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 26, 2017

From @jkeenan

On Sun, 24 Dec 2017 11​:16​:06 GMT, zefram@​fysh.org wrote​:

Fixed in commit d2e38af.

-zefram

This does appear to be fixed on FreeBSD, but we've got a smoke test report from Cygwin explicitly FAILing on dist/PathTools/t/cwd_enoent.t. Please see​:

http​://perl.develop-help.com/raw/?id=204663

Thank you very much.
--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 26, 2017

From @jkeenan

On Tue, 26 Dec 2017 16​:59​:16 GMT, jkeenan wrote​:

On Sun, 24 Dec 2017 11​:16​:06 GMT, zefram@​fysh.org wrote​:

Fixed in commit d2e38af.

-zefram

This does appear to be fixed on FreeBSD, but we've got a smoke test
report from Cygwin explicitly FAILing on
dist/PathTools/t/cwd_enoent.t. Please see​:

http​://perl.develop-help.com/raw/?id=204663

Thank you very much.

Please ignore the above. Wrong ticket.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 28, 2017

@iabyn - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 15, 2018

From @tonycoz

On Sun, Dec 24, 2017 at 11​:15​:47AM +0000, Zefram wrote​:

Fixed in commit d2e38af.

This new test has been failing on cygwin since it was introduced.

tony@​saturn ~/dev/perl/git/perl/t
$ ./perl harness -v ../dist/PathTools/t/cwd_enoent.t
../dist/PathTools/t/cwd_enoent.t ..
1..8
not ok 1 - regular getcwd result on non-existent directory
# Failed test 'regular getcwd result on non-existent directory'
# at t/cwd_enoent.t line 30.
# got​: '/tmp/4WuHD3C4IY/testdir'
# expected​: undef
not ok 2 - regular getcwd errno on non-existent directory
# Failed test 'regular getcwd errno on non-existent directory'
# at t/cwd_enoent.t line 31.
# got​: '0'
# expected​: '2'
.​: No such file or directory at t/cwd_enoent.t line 33.
# Looks like your test exited with 2 just after 2.
.​: No such file or directory at ../../lib/File/Temp.pm line 777.
END failed--call queue aborted.
Dubious, test returned 2 (wstat 512, 0x200)
Failed 8/8 subtests

Test Summary Report


../dist/PathTools/t/cwd_enoent.t (Wstat​: 512 Tests​: 2 Failed​: 2)
  Failed tests​: 1-2
  Non-zero exit status​: 2
  Parse errors​: Bad plan. You planned 8 tests but ran 2.
Files=1, Tests=2, 0 wallclock secs ( 0.00 usr 0.00 sys + 0.09 cusr 0.00 csys = 0.09 CPU)
Result​: FAIL

tony@​saturn ~/dev/perl/git/perl/t
$ git describe
v5.27.7-132-g22803c6a19

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 15, 2018

From @eserte

Dana Sun, 14 Jan 2018 18​:32​:28 -0800, tonyc reče​:

On Sun, Dec 24, 2017 at 11​:15​:47AM +0000, Zefram wrote​:

Fixed in commit d2e38af.

This new test has been failing on cygwin since it was introduced.

tony@​saturn ~/dev/perl/git/perl/t
$ ./perl harness -v ../dist/PathTools/t/cwd_enoent.t
../dist/PathTools/t/cwd_enoent.t ..
1..8
not ok 1 - regular getcwd result on non-existent directory
# Failed test 'regular getcwd result on non-existent directory'
# at t/cwd_enoent.t line 30.
# got​: '/tmp/4WuHD3C4IY/testdir'
# expected​: undef
not ok 2 - regular getcwd errno on non-existent directory
# Failed test 'regular getcwd errno on non-existent directory'
# at t/cwd_enoent.t line 31.
# got​: '0'
# expected​: '2'
.​: No such file or directory at t/cwd_enoent.t line 33.
# Looks like your test exited with 2 just after 2.
.​: No such file or directory at ../../lib/File/Temp.pm line 777.
END failed--call queue aborted.
Dubious, test returned 2 (wstat 512, 0x200)
Failed 8/8 subtests

Test Summary Report
-------------------
../dist/PathTools/t/cwd_enoent.t (Wstat​: 512 Tests​: 2 Failed​: 2)
Failed tests​: 1-2
Non-zero exit status​: 2
Parse errors​: Bad plan. You planned 8 tests but ran 2.
Files=1, Tests=2, 0 wallclock secs ( 0.00 usr 0.00 sys + 0.09 cusr
0.00 csys = 0.09 CPU)
Result​: FAIL

tony@​saturn ~/dev/perl/git/perl/t
$ git describe
v5.27.7-132-g22803c6a19

I guess that's because Cygwin's getcwd() is implemented with _backtick_pwd(), and Cygwin's pwd command behaves differently than on Unix --- in this situation it returns the non-existent directory.

Regards,
  Slaven

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 15, 2018

From zefram@fysh.org

Tony Cook wrote​:

# Failed test 'regular getcwd result on non-existent directory'
# at t/cwd_enoent.t line 30.
# got​: '/tmp/4WuHD3C4IY/testdir'
# expected​: undef

For getcwd() to yield a non-error return for a non-existent directory
seems like a bug. Cygwin seems to prefer _backtick_pwd() as its
implementation of getcwd(), so maybe this is a bug in Cygwin's pwd(1).
Would someone on Cygwin please check, firstly, whether pwd(1) actually
exhibits this behaviour for a removed directory, and secondly, whether
the removed directory can actually be addressed by this name. (If it
can be addressed by name, that would seem to be a bug in rmdir().)

.​: No such file or directory at t/cwd_enoent.t line 33.
# Looks like your test exited with 2 just after 2.

This is another problem. That's abs_path() dying, which it shouldn't​:
it's documented to return errors as undef+$!. Cygwin actually uses
fast_abs_path() as its abs_path(), and fast_abs_path() is coded to die
rather than yield undef+$!, so that's a clear bug in fast_abs_path().

-zefram

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 16, 2018

From zefram@fysh.org

I wrote​:

fast_abs_path() as its abs_path(), and fast_abs_path() is coded to die
rather than yield undef+$!, so that's a clear bug in fast_abs_path().

This bit fixed in commit a97021b.

-zefram

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 16, 2018

From @tonycoz

On Mon, Jan 15, 2018 at 11​:28​:06PM +0000, Zefram wrote​:

Tony Cook wrote​:

# Failed test 'regular getcwd result on non-existent directory'
# at t/cwd_enoent.t line 30.
# got​: '/tmp/4WuHD3C4IY/testdir'
# expected​: undef

For getcwd() to yield a non-error return for a non-existent directory
seems like a bug. Cygwin seems to prefer _backtick_pwd() as its
implementation of getcwd(), so maybe this is a bug in Cygwin's pwd(1).
Would someone on Cygwin please check, firstly, whether pwd(1) actually
exhibits this behaviour for a removed directory, and secondly, whether
the removed directory can actually be addressed by this name. (If it
can be addressed by name, that would seem to be a bug in rmdir().)

I tested with​:

tony@​saturn ~/dev/perl/git
$ cat 132648.c
#include <stdio.h>
#include <unistd.h>
#include <sys/stat.h>
#include <sys/types.h>

int main(void) {
  char buf[1000]; /* keeping this simple */
  if (mkdir("foo", 0700) < 0) {
  perror("mkdir");
  return 1;
  }
  if (chdir("foo") < 0) {
  perror("chdir");
  return 1;
  }
  if (rmdir("../foo") < 0) {
  perror("rmdir");
  return 1;
  }

  if (!getcwd(buf, sizeof(buf))) {
  perror("getcwd");
  return 1;
  }
  puts(buf);

  return 0;
}

tony@​saturn ~/dev/perl/git
$ cc -o132648.exe 132648.c

tony@​saturn ~/dev/perl/git
$ ./132648
/home/tony/dev/perl/git/foo

I've asked about it on the cygwin mailing list.

I suspect this will be WONTFIX at the cygwin level.

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 16, 2018

From zefram@fysh.org

Tony Cook wrote​:

I suspect this will be WONTFIX at the cygwin level.

I'd still like to know which bug it is that they won't fix.

-zefram

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 16, 2018

From @tonycoz

On Tue, Jan 16, 2018 at 05​:50​:47AM +0000, Zefram wrote​:

Tony Cook wrote​:

I suspect this will be WONTFIX at the cygwin level.

I'd still like to know which bug it is that they won't fix.

Well, remembering cygwin is built on top of Win32, and if I'm reading
the code correctly, rather than calling the RemoveDirectory() API,
which fails on "open" directories, cygwin calls NtSetInformationFile()
to mark the directory to be deleted once it's closed.

Having to record that the directory (or file) is marked deleted and
not make it available to getcwd(), open(), opendir(), unlink() etc
might be considered too much work for too little return.

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 16, 2018

From zefram@fysh.org

Tony Cook wrote​:

Having to record that the directory (or file) is marked deleted and
not make it available to getcwd(), open(), opendir(), unlink() etc

That's not what I'm proposing, and would be totally unworkable.

If rmdir(2) doesn't actually succeed in removing the directory
(immediately), then I'll add a check for that in cwd_enoent.t, skipping
the tests if the directory didn't go away. If rmdir() does remove the
directory and pwd(1) emits a name that can't be used, then I'll add a
check for that in _backtick_pwd(), to prevent it returning an unusable
name.

-zefram

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 16, 2018

From @tonycoz

On Tue, Jan 16, 2018 at 08​:23​:58AM +0000, Zefram wrote​:

Tony Cook wrote​:

Having to record that the directory (or file) is marked deleted and
not make it available to getcwd(), open(), opendir(), unlink() etc

That's not what I'm proposing, and would be totally unworkable.

If rmdir(2) doesn't actually succeed in removing the directory
(immediately), then I'll add a check for that in cwd_enoent.t, skipping
the tests if the directory didn't go away. If rmdir() does remove the
directory and pwd(1) emits a name that can't be used, then I'll add a
check for that in _backtick_pwd(), to prevent it returning an unusable
name.

If this what you need?

tony@​saturn ~/dev/perl/git
$ perl 132648.pl
/home/tony/dev/perl/git/foo

tony@​saturn ~/dev/perl/git
$ cat 132648.pl
#!perl
use Cwd;

mkdir "foo", 0700
  or die "mkdir $!";
chdir "foo"
  or die "chdir $!";
rmdir "../foo"
  or die "rmdir $!";
my $cwd = getcwd
  or die "getcwd $!";
print "$cwd\n";
print "$cwd is a dir\n" if -d $cwd;

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 16, 2018

From zefram@fysh.org

Tony Cook wrote​:

If this what you need?

Nearly. Instead of the -d line, please do

  print stat($cwd) ? join",",stat(_) : $!, "\n";

and please show the output of Cwd​::_backtick_pwd(), not just getcwd().
For good measure, please do

  print $_, " ", \&{"Cwd​::$_"}, "\n"
  foreach qw(cwd getcwd fastgetcwd fastcwd _backtick_pwd _perl_getcwd);

-zefram

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 16, 2018

From @tonycoz

On Tue, Jan 16, 2018 at 10​:45​:38AM +0000, Zefram wrote​:

Tony Cook wrote​:

If this what you need?

Nearly. Instead of the -d line, please do

print stat\($cwd\) ? join"\,"\,stat\(\_\) : $\!\, "\\n";

and please show the output of Cwd​::_backtick_pwd(), not just getcwd().
For good measure, please do

print $\_\, " "\, \\&\{"Cwd&#8203;::$\_"\}\, "\\n"
foreach qw\(cwd getcwd fastgetcwd fastcwd \_backtick\_pwd \_perl\_getcwd\);

tony@​saturn ~/dev/perl/git
$ perl 132648.pl
/home/tony/dev/perl/git/foo
No such file or directory
cwd /home/tony/dev/perl/git/foo
getcwd /home/tony/dev/perl/git/foo
fastgetcwd /home/tony/dev/perl/git/foo
fastcwd /home/tony/dev/perl/git/foo
_backtick_pwd /home/tony/dev/perl/git/foo
.​: No such file or directory at 132648.pl line 15.

tony@​saturn ~/dev/perl/git
$ cat 132648.pl
#!perl
use Cwd;

mkdir "foo", 0700
  or die "mkdir $!";
chdir "foo"
  or die "chdir $!";
rmdir "../foo"
  or die "rmdir $!";
my $cwd = getcwd
  or die "getcwd $!";
print "$cwd\n";
print stat($cwd) ? join",",stat(_) : $!, "\n";
print $_, " ", &{"Cwd​::$_"}, "\n"
  foreach qw(cwd getcwd fastgetcwd fastcwd _backtick_pwd _perl_getcwd);

I assumed you wanted the Cwd​::foo functions called rather than a bunch
of CODE(...) output.

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 16, 2018

From zefram@fysh.org

Tony Cook wrote​:

I assumed you wanted the Cwd​::foo functions called rather than a bunch
of CODE(...) output.

I wanted the CODE(...) output. I want to see which names are aliased,
and hence which implementations are hiding behind each name. Though the
outputs of the functions are also interesting data points.

-zefram

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 16, 2018

From @tonycoz

On Tue, Jan 16, 2018 at 11​:44​:00PM +0000, Zefram wrote​:

Tony Cook wrote​:

I assumed you wanted the Cwd​::foo functions called rather than a bunch
of CODE(...) output.

I wanted the CODE(...) output. I want to see which names are aliased,
and hence which implementations are hiding behind each name. Though the
outputs of the functions are also interesting data points.

$ perl 132648.pl
/home/tony/dev/perl/git/foo
cwd CODE(0x800f9630)
getcwd CODE(0x800f9630)
fastgetcwd CODE(0x800f9630)
fastcwd CODE(0x800f9630)
_backtick_pwd CODE(0x800a3a20)
_perl_getcwd CODE(0x800a35d0)
No such file or directory

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 17, 2018

From zefram@fysh.org

Findings from debugging output provided by Tony​:

* rmdir() succeeds in making the directory's former name unresolvable.

* Cygwin uses the XS implementation for its getcwd(), not _backtick_pwd()
  as the code seems to prefer.

* both the XS getcwd() and _backtick_pwd() produce the erroneous result.

* behind the XS getcwd(), the core's getcwd_sv() and libc's getcwd(3)
  produce the erroneous result.

This is fixable, but it's got more complicated than I'm willing to
squeeze into the present ticket. I've therefore made cwd_enoent.t just
skip on Cygwin, and opened [perl #132733] regarding getcwd() erroneously
producing a non-error result.

-zefram

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 23, 2018

From @khwilliamson

Thank you for filing this report. You have helped make Perl better.

With the release yesterday of Perl 5.28.0, this and 185 other issues have been
resolved.

Perl 5.28.0 may be downloaded via​:
https://metacpan.org/release/XSAWYERX/perl-5.28.0

If you find that the problem persists, feel free to reopen this ticket.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 23, 2018

@khwilliamson - Status changed from 'pending release' to 'resolved'

@p5pRT p5pRT closed this Jun 23, 2018
@p5pRT p5pRT added the Severity Low label Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.