diff --git a/FileCheck.xs b/FileCheck.xs index df1918c..2883984 100644 --- a/FileCheck.xs +++ b/FileCheck.xs @@ -119,14 +119,25 @@ int _overload_ft_ops(pTHX) { return check_status; } -SV* _overload_ft_ops_sv(pTHX) { +/* +* NV-specific helper for -M, -C, -A ops. +* +* _check() returns a (status, value) pair for NV ops to avoid the -1 +* sentinel collision: FALLBACK_TO_REAL_OP is -1, but -1.0 is a valid +* NV result (file modified exactly 1 day in the future). +* +* Returns: +* *status_out = -1 -> FALLBACK_TO_REAL_OP (nv_out is unused) +* *status_out = -2 -> CHECK_IS_NULL / undef (nv_out is unused) +* *status_out = 1 -> success, *nv_out has the value +*/ +void _overload_ft_ops_nv(pTHX_ int *status_out, NV *nv_out) { dMY_CXT; SV *const arg = *PL_stack_sp; - int optype = PL_op->op_type; /* this is the current op_type we are mocking */ - SV *status; /* 1 -> YES ; 0 -> FALSE ; -1 -> delegate */ + int optype = PL_op->op_type; + int count; dSP; - int count; ENTER; SAVETMPS; @@ -138,21 +149,39 @@ SV* _overload_ft_ops_sv(pTHX) { PUTBACK; - count = call_pv("Overload::FileCheck::_check", G_SCALAR); + count = call_pv("Overload::FileCheck::_check", G_ARRAY); SPAGAIN; - if (count != 1) + if (count < 1) croak("No return value from Overload::FileCheck::_check for OP #%d\n", optype); - status = POPs; - SvREFCNT_inc( status ); + if (count == 1) { + /* Single return: FALLBACK_TO_REAL_OP or CHECK_IS_NULL */ + SV *sv = POPs; + if (!SvOK(sv)) + *status_out = -2; /* undef => CHECK_IS_NULL */ + else + *status_out = SvIV(sv); /* -1 for FALLBACK, -2 for NULL */ + *nv_out = 0; + } + else if (count == 2) { + /* Pair return: (status_code, nv_value) */ + SV *value_sv = POPs; + SV *status_sv = POPs; + *status_out = SvIV(status_sv); + *nv_out = SvNV(value_sv); + } + else { + /* Pop excess values to avoid stack corruption */ + int orig_count = count; + while (count-- > 0) (void)POPs; + croak("Overload::FileCheck::_check returned %d values for NV OP #%d, expected 1 or 2\n", orig_count, optype); + } - OFC_DEBUG("_overload_ft_ops_sv: optype=%d\n", optype); + OFC_DEBUG("_overload_ft_ops_nv: status=%d optype=%d\n", *status_out, optype); LEAVE_PRESERVING_ERRNO(); - - return status; } /* @@ -326,7 +355,8 @@ PP(pp_overload_ft_int) { PP(pp_overload_ft_nv) { dMY_CXT; - SV *status; + int check_status; + NV nv_value; int saved_errno; if (!gl_overload_ft) @@ -336,36 +366,28 @@ PP(pp_overload_ft_nv) { RETURN_CALL_REAL_OP_IF_UNMOCK(); RETURN_CALL_REAL_OP_IF_CALL_WITH_DEFGV(); - status = _overload_ft_ops_sv(aTHX); + /* _overload_ft_ops_nv uses G_ARRAY and a status code to avoid the -1 + * sentinel collision: FALLBACK_TO_REAL_OP is -1, but -1.0 is a valid + * NV result (e.g. file modified exactly 1 day in the future). */ + _overload_ft_ops_nv(aTHX_ &check_status, &nv_value); - if ( !SvOK(status) ) { /* CHECK_IS_NULL — undef */ - SvREFCNT_dec(status); + if ( check_status == -1 ) + return CALL_REAL_OP(); + + if ( check_status == -2 ) { /* CHECK_IS_NULL */ FT_SETUP_dSP_IF_NEEDED; FT_RETURNUNDEF; } - /* Save errno — sv_setnv()/sv_setiv() and FT_RETURN_TARG can trigger - * allocations or other Perl internals that clobber errno. */ + /* Save errno — sv_setnv() and FT_RETURN_TARG can trigger allocations + * or other Perl internals that clobber errno. */ saved_errno = errno; - /* Check for FALLBACK_TO_REAL_OP sentinel (-1) across all SV types */ - if ( SvNV(status) == -1 ) { - SvREFCNT_dec(status); - return CALL_REAL_OP(); - } - { dTARGET; FT_SETUP_dSP_IF_NEEDED; - if ( SvNOK(status) ) - sv_setnv(TARG, (NV) SvNV(status) ); - else if ( SvIOK(status) ) - sv_setiv(TARG, (IV) SvIV(status) ); - else - sv_setnv(TARG, (NV) SvNV(status) ); - - SvREFCNT_dec(status); + sv_setnv(TARG, nv_value); errno = saved_errno; FT_RETURN_TARG; } diff --git a/lib/Overload/FileCheck.pm b/lib/Overload/FileCheck.pm index 34acb3c..9d34ddc 100644 --- a/lib/Overload/FileCheck.pm +++ b/lib/Overload/FileCheck.pm @@ -142,7 +142,8 @@ my %MAP_STAT_T_IX = ( # op_type_id => check my %REVERSE_MAP = reverse %MAP_FC_OP; -my %OP_CAN_RETURN_INT = map { $MAP_FC_OP{$_} => 1 } qw{ s M C A }; +my %OP_CAN_RETURN_INT = map { $MAP_FC_OP{$_} => 1 } qw{ s }; +my %OP_RETURNS_NV = map { $MAP_FC_OP{$_} => 1 } qw{ M C A }; my %OP_IS_STAT_OR_LSTAT = map { $MAP_FC_OP{$_} => 1 } qw{ stat lstat }; # # This is listing the default ERRNO codes @@ -363,17 +364,19 @@ sub _check_from_stat { c => sub { _check_mode_type( $lstat[ST_MODE], S_IFCHR ) }, # character special file # Age calculations: (basetime - timestamp) / seconds_per_day + # Returns scalar ref to distinguish real NV values from the + # FALLBACK_TO_REAL_OP sentinel (-1) — see _check() NV handling. M => sub { return CHECK_IS_NULL unless scalar @stat && defined $stat[ST_MTIME]; - ( get_basetime() - $stat[ST_MTIME] ) / 86400.0; # days since modification + \( ( get_basetime() - $stat[ST_MTIME] ) / 86400.0 ); # days since modification }, A => sub { return CHECK_IS_NULL unless scalar @stat && defined $stat[ST_ATIME]; - ( get_basetime() - $stat[ST_ATIME] ) / 86400.0; # days since access + \( ( get_basetime() - $stat[ST_ATIME] ) / 86400.0 ); # days since access }, C => sub { return CHECK_IS_NULL unless scalar @stat && defined $stat[ST_CTIME]; - ( get_basetime() - $stat[ST_CTIME] ) / 86400.0; # days since inode change + \( ( get_basetime() - $stat[ST_CTIME] ) / 86400.0 ); # days since inode change }, ); @@ -479,6 +482,22 @@ sub _check { return $out; } + # NV ops (-M, -C, -A) return a (status, value) pair to avoid the -1 + # sentinel collision: FALLBACK_TO_REAL_OP is -1, but -1.0 is a valid + # NV result (file modified exactly 1 day in the future). The XS + # handler uses the status code instead of checking value == -1. + if ( $OP_RETURNS_NV{$optype} ) { + # Scalar ref: from _check_from_stat — always a real value, never FALLBACK + if ( ref $out eq 'SCALAR' ) { + return ( CHECK_IS_TRUE, $$out ); + } + # Bare scalar: from direct mock_file_check callbacks + if ( !ref $out && $out == FALLBACK_TO_REAL_OP ) { + return (FALLBACK_TO_REAL_OP); + } + return ( CHECK_IS_TRUE, $out ); + } + if ( !$out ) { # Set a default ERRNO when the user didn't provide one, diff --git a/t/nv-sentinel-collision.t b/t/nv-sentinel-collision.t new file mode 100644 index 0000000..95c1052 --- /dev/null +++ b/t/nv-sentinel-collision.t @@ -0,0 +1,139 @@ +#!/usr/bin/perl -w + +# Test that -M/-C/-A correctly return -1.0 when a file's timestamp is +# exactly basetime + 86400 (1 day in the future), instead of silently +# falling back to the real OP. +# +# The FALLBACK_TO_REAL_OP sentinel is -1. Before the fix, the XS handler +# pp_overload_ft_nv checked `SvNV(status) == -1` which collided with the +# legitimate NV value -1.0. + +use strict; +use warnings; + +use Test2::Bundle::Extended; +use Test2::Tools::Explain; +use Test2::Plugin::NoWarnings; + +use Overload::FileCheck q{:all}; + +my $basetime = Overload::FileCheck::get_basetime(); + +subtest 'mock_all_from_stat: -M returns -1.0 for mtime 1 day in future' => sub { + my $mtime_future = $basetime + 86400; + + mock_all_from_stat(sub { + my ($op, $file) = @_; + return stat_as_file( mtime => $mtime_future ) if $file eq '/test/future'; + return FALLBACK_TO_REAL_OP; + }); + + my $age = -M '/test/future'; + ok( defined $age, '-M returns a defined value (not undef from fallback)' ); + ok( abs($age - (-1.0)) < 0.01, "-M returns approximately -1.0 (got: $age)" ) + if defined $age; + + unmock_all_file_checks(); + unmock_stat(); +}; + +subtest 'mock_all_from_stat: -A returns -1.0 for atime 1 day in future' => sub { + my $atime_future = $basetime + 86400; + + mock_all_from_stat(sub { + my ($op, $file) = @_; + return stat_as_file( atime => $atime_future ) if $file eq '/test/future'; + return FALLBACK_TO_REAL_OP; + }); + + my $age = -A '/test/future'; + ok( defined $age, '-A returns a defined value (not undef from fallback)' ); + ok( abs($age - (-1.0)) < 0.01, "-A returns approximately -1.0 (got: $age)" ) + if defined $age; + + unmock_all_file_checks(); + unmock_stat(); +}; + +subtest 'mock_all_from_stat: -C returns -1.0 for ctime 1 day in future' => sub { + my $ctime_future = $basetime + 86400; + + mock_all_from_stat(sub { + my ($op, $file) = @_; + return stat_as_file( ctime => $ctime_future ) if $file eq '/test/future'; + return FALLBACK_TO_REAL_OP; + }); + + my $age = -C '/test/future'; + ok( defined $age, '-C returns a defined value (not undef from fallback)' ); + ok( abs($age - (-1.0)) < 0.01, "-C returns approximately -1.0 (got: $age)" ) + if defined $age; + + unmock_all_file_checks(); + unmock_stat(); +}; + +subtest 'mock_all_from_stat: FALLBACK still works for non-mocked files' => sub { + mock_all_from_stat(sub { + my ($op, $file) = @_; + return stat_as_file( mtime => $basetime ) if $file eq '/test/now'; + return FALLBACK_TO_REAL_OP; + }); + + # /nonexistent should fall back to real stat => undef + my $age = -M '/nonexistent/path/should/not/exist'; + ok( !defined $age, 'FALLBACK_TO_REAL_OP still delegates to real OP' ); + + # mocked file should return ~0 days + my $now_age = -M '/test/now'; + ok( defined $now_age, '-M returns defined for mocked file' ); + ok( abs($now_age) < 0.01, "-M returns approximately 0 (got: $now_age)" ) + if defined $now_age; + + unmock_all_file_checks(); + unmock_stat(); +}; + +subtest 'direct mock_file_check: -M FALLBACK_TO_REAL_OP still works' => sub { + mock_file_check( '-M' => sub { + my ($file) = @_; + return 42.5 if $file eq '/test/custom'; + return FALLBACK_TO_REAL_OP; + }); + + my $age = -M '/test/custom'; + ok( defined $age, '-M returns defined for mocked file' ); + ok( abs($age - 42.5) < 0.01, "-M returns 42.5 (got: $age)" ) + if defined $age; + + my $real = -M '/nonexistent/direct/mock/fallback'; + ok( !defined $real, 'FALLBACK_TO_REAL_OP works for direct -M mock' ); + + unmock_file_check('-M'); +}; + +subtest 'mock_all_from_stat: other NV values pass through correctly' => sub { + mock_all_from_stat(sub { + my ($op, $file) = @_; + if ( $file eq '/test/past' ) { + return stat_as_file( mtime => $basetime - 2 * 86400 ); # 2 days ago + } + if ( $file eq '/test/far_future' ) { + return stat_as_file( mtime => $basetime + 5 * 86400 ); # 5 days future + } + return FALLBACK_TO_REAL_OP; + }); + + my $past = -M '/test/past'; + ok( defined $past, '-M defined for 2 days ago' ); + ok( abs($past - 2.0) < 0.01, "-M returns ~2.0 (got: $past)" ) if defined $past; + + my $far = -M '/test/far_future'; + ok( defined $far, '-M defined for 5 days in future' ); + ok( abs($far - (-5.0)) < 0.01, "-M returns ~-5.0 (got: $far)" ) if defined $far; + + unmock_all_file_checks(); + unmock_stat(); +}; + +done_testing;