houseabsolute / DateTime-Locale

Localization support for DateTime.pm
https://metacpan.org/release/DateTime-Locale/
Other
3 stars 12 forks source link

accessor functions are vulnerable to caller malice #2

Closed FGasper closed 8 years ago

FGasper commented 8 years ago
> perl -MYAML::Syck -MDateTime::Locale -e'my $fr = DateTime::Locale->load("fr"); print Dump($fr->am_pm_abbreviated()); push @{$fr->am_pm_abbreviated()}, 123; print Dump($fr->am_pm_abbreviated());'
--- 
- AM
- PM
--- 
- AM
- PM
- 123

One call to a function should not be able to “poison” the results of a subsequent call.

autarch commented 8 years ago

This is done mostly because it makes the DateTime.pm internals much faster than having the DateTime::Locale accessors make copies for every call. There's already an RT issue for this from long ago. I'm not sure if there's a good fix that keeps things as fast as they can be.

FGasper commented 8 years ago

What is the speed difference?

I personally would favor robustness over speed by default, maybe creating am_pm_abbreviated_internal() or some such for applications where the copying makes a significant difference.

> perl test_array_copy.pl 
            (warning: too few iterations for a reliable count)
                Rate fresh_ref  dupe_ref  orig_ref
fresh_ref  2439024/s        --       -1%      -93%
dupe_ref   2469136/s        1%        --      -93%
orig_ref  33333333/s     1267%     1250%        --

Avoiding the dupe is about 12x the speed, but a difference between 2.5 million and 33 million calls per second for locale stuff would seem only to matter in exceptional circumstances.

FGasper commented 8 years ago
use Benchmark;

my $list_ar = [ 'foo', 'bar', 'baz', 'qux' ];

Benchmark::cmpthese(
    10000000,
    {
        fresh_ref => sub { return [ 'foo', 'bar', 'baz', 'qux' ] },
        dupe_ref => sub { return [ @$list_ar ] },
        orig_ref => sub { return $list_ar },
    },
);