karupanerura / TOML-Parser

simple toml parser
Other
15 stars 6 forks source link

Uses File::Slurp, known to be buggy and vulnerable #1

Closed karenetheridge closed 10 years ago

karenetheridge commented 10 years ago

e.g. look at https://rt.cpan.org/Ticket/Display.html?id=83126 and be dismayed

File::Slurp::Tiny and Path::Tiny are both excellent alternatives.

karupanerura commented 10 years ago

Thank you for issued. Maybe I understand. I want to solve it too. But, I don't want to use slurp. Because, I want to keep parse_fh interface. I maybe think this patch solve this problem. What do you think?

diff --git a/lib/TOML/Parser.pm b/lib/TOML/Parser.pm
index dae98e8..0383ffc 100644
--- a/lib/TOML/Parser.pm
+++ b/lib/TOML/Parser.pm
@@ -11,6 +11,8 @@ use TOML::Parser::Tokenizer qw/:constant/;
 use TOML::Parser::Util qw/unescape_str/;
 use Types::Serialiser;

+use constant IO_LAYER => $^O eq 'MSWin32' ? ':raw:encoding(utf-8):crlf' : ':raw:encoding(utf-8):unix';
+
 sub new {
     my $class = shift;
     my $args  = (@_ == 1 and ref $_[0] eq 'HASH') ? +shift : +{ @_ };
@@ -23,7 +25,7 @@ sub new {

 sub parse_file {
     my ($self, $file) = @_;
-    open my $fh, '<:encoding(utf-8)', $file or die $!;
+    open my $fh, IO_LAYER, $file or die $!;
     return $self->parse_fh($fh);
 }

If there is already test case, please tell me.

shadowcat-mst commented 10 years ago

It's author/test-case-maker.pl that's using it, and it's already using slurp().

We're just suggesting you adjust it to use a less buggy slurp().

No change to lib/ is required.

karenetheridge commented 10 years ago

Sorry for the confusion! I found this dist in https://metacpan.org/requires/distribution/File-Slurp?sort=[[2,1]]. Your code in TOML::Parser::parse_file is fine the way it is.

It looks like the file you're reading in author/test-case-maker.pl contains JSON, so I think you probably need slurp_raw from Path::Tiny.

karupanerura commented 10 years ago

Oops!! Sorry for wrong understanding. I fixed it.