Skip to content

refactor: extract _check_open_perms, fix dead elsif in sysopen #353

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.

Sign up for GitHub

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

Draft
Koan-Bot wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

refactor: extract _check_open_perms, fix dead elsif in sysopen #353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 42 additions & 62 deletions lib/Test/MockFile.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -783,6 +783,38 @@ sub _check_parent_perms {
return _check_perms( $parent_mock, $access );
}

# _check_open_perms($mock_file, $abs_path, $rw, $is_new, $func_name, @orig_args)
# Unified permission check for open/sysopen.
# $is_new: true when the file is being created (contents was undef before O_CREAT).
# Returns 1 if allowed, 0 if denied (with $! and autodie set).
sub _check_open_perms {
my ( $mock_file, $abs_path, $rw, $is_new, $func_name, @orig_args ) = @_;

return 1 unless defined $_mock_uid;

if ( !$is_new ) {
# Existing file: check file permissions
my $need = 0;
$need |= 4 if $rw =~ /r/;
$need |= 2 if $rw =~ /w/;
if ( !_check_perms( $mock_file, $need ) ) {
$! = EACCES;
_maybe_throw_autodie( $func_name, @orig_args );
return 0;
}
}
elsif ( $rw =~ /w/ ) {
# Creating new file: check parent dir write+execute
if ( !_check_parent_perms( $abs_path, 2 | 1 ) ) {
$! = EACCES;
_maybe_throw_autodie( $func_name, @orig_args );
return 0;
}
}

return 1;
}

my @_tmf_callers;

# Packages where autodie was active when T::MF was imported.
Expand Down Expand Up @@ -2815,27 +2847,7 @@ sub _io_file_mock_open {
$rw .= 'a' if grep { $_ eq $mode } qw/>> +>>/;

# Permission check (GH #3)
if ( defined $_mock_uid ) {
if ( defined $contents ) {
# Existing file: check file permissions
my $need = 0;
$need |= 4 if $rw =~ /r/;
$need |= 2 if $rw =~ /w/;
if ( !_check_perms( $mock_file, $need ) ) {
$! = EACCES;
_maybe_throw_autodie( 'open', @_ );
return undef;
}
}
elsif ( $rw =~ /w/ ) {
# Creating new file: check parent dir write+execute
if ( !_check_parent_perms( $abs_path, 2 | 1 ) ) {
$! = EACCES;
_maybe_throw_autodie( 'open', @_ );
return undef;
}
}
}
return undef unless _check_open_perms( $mock_file, $abs_path, $rw, !defined $contents, 'open', @_ );

# Tie the existing IO::File glob directly (don't create a new one)
tie *{$fh}, 'Test::MockFile::FileHandle', $abs_path, $rw;
Expand Down Expand Up @@ -3166,26 +3178,8 @@ sub __open (*;$@) {
$rw .= 'w' if grep { $_ eq $mode } qw/+< +> +>> > >>/;
$rw .= 'a' if grep { $_ eq $mode } qw/>> +>>/;

# Permission check (GH #3) — IO::File path must match __open
if ( defined $_mock_uid ) {
if ( defined $contents ) {
my $need = 0;
$need |= 4 if $rw =~ /r/;
$need |= 2 if $rw =~ /w/;
if ( !_check_perms( $mock_file, $need ) ) {
$! = EACCES;
_maybe_throw_autodie( 'open', @_ );
return undef;
}
}
elsif ( $rw =~ /w/ ) {
if ( !_check_parent_perms( $abs_path, 2 | 1 ) ) {
$! = EACCES;
_maybe_throw_autodie( 'open', @_ );
return undef;
}
}
}
# Permission check (GH #3)
return undef unless _check_open_perms( $mock_file, $abs_path, $rw, !defined $contents, 'open', @_ );

my $filefh = IO::File->new;
tie *{$filefh}, 'Test::MockFile::FileHandle', $abs_path, $rw;
Expand Down Expand Up @@ -3326,8 +3320,12 @@ sub __sysopen (*$$;$) {
return undef;
}

# Track whether we're creating a new file (before O_CREAT populates contents).
# Used by the permission check below — without this, the elsif branch is dead.
my $is_new = !defined $mock_file->{'contents'};

# O_CREAT — POSIX open(2): creating a new file sets atime, mtime, and ctime.
if ( $sysopen_mode & O_CREAT && !defined $mock_file->{'contents'} ) {
if ( $sysopen_mode & O_CREAT && $is_new ) {
$mock_file->{'contents'} = '';
my $now = time;
$mock_file->{'atime'} = $now;
Expand Down Expand Up @@ -3370,26 +3368,8 @@ sub __sysopen (*$$;$) {
return undef;
}

# Permission check (GH #3)
if ( defined $_mock_uid ) {
if ( defined $mock_file->{'contents'} ) {
my $need = 0;
$need |= 4 if $rw =~ /r/;
$need |= 2 if $rw =~ /w/;
if ( !_check_perms( $mock_file, $need ) ) {
$! = EACCES;
_maybe_throw_autodie( 'sysopen', @_ );
return undef;
}
}
elsif ( $rw =~ /w/ ) {
if ( !_check_parent_perms( $mock_file->{'path'}, 2 | 1 ) ) {
$! = EACCES;
_maybe_throw_autodie( 'sysopen', @_ );
return undef;
}
}
}
# Permission check (GH #3) — uses $is_new captured before O_CREAT
return undef unless _check_open_perms( $mock_file, $mock_file->{'path'}, $rw, $is_new, 'sysopen', @_ );

$abs_path //= $mock_file->{'path'};

Expand Down
35 changes: 35 additions & 0 deletions t/perms.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,4 +336,39 @@ subtest 'open > on new file checks parent directory perms' => sub {
} 1000, 1000;
};

# =========================================================================
# sysopen O_CREAT — parent-dir permission check (GH #329)
# Before the fix, the elsif branch checking parent-dir perms was dead code
# in __sysopen, so sysopen(FH, '/restricted/new', O_WRONLY|O_CREAT) would
# succeed even in a read-only parent directory.
# =========================================================================

subtest 'sysopen O_CREAT checks parent dir permissions' => sub {
my $parent = Test::MockFile->new_dir( '/perms/restricted', { mode => 0555, uid => 1000, gid => 1000 } );
my $child = Test::MockFile->file('/perms/restricted/newfile');

with_user {
ok( !sysopen( my $fh, '/perms/restricted/newfile', O_WRONLY | O_CREAT ),
'sysopen O_CREAT fails in read-only parent dir' );
is( $! + 0, EACCES, 'sysopen errno is EACCES for restricted parent' );
} 2000, 2000;

# Owner also cannot create in a 0555 directory (no write bit for anyone)
with_user {
ok( !sysopen( my $fh, '/perms/restricted/newfile', O_WRONLY | O_CREAT ),
'sysopen O_CREAT fails for owner too when parent has no write bit' );
is( $! + 0, EACCES, 'sysopen errno is EACCES for owner too' );
} 1000, 1000;

# With write permission on parent, O_CREAT should succeed
my $parent2 = Test::MockFile->new_dir( '/perms/writable', { mode => 0755, uid => 1000, gid => 1000 } );
my $child2 = Test::MockFile->file('/perms/writable/newfile2');

with_user {
ok( sysopen( my $fh, '/perms/writable/newfile2', O_WRONLY | O_CREAT ),
'sysopen O_CREAT succeeds in writable parent dir' );
close $fh if $fh;
} 1000, 1000;
};

done_testing();
Loading