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

Disallow strings as ops in cmp-ok in Test.pm6 #5347

Closed
p6rt opened this issue May 29, 2016 · 12 comments
Closed

Disallow strings as ops in cmp-ok in Test.pm6 #5347

p6rt opened this issue May 29, 2016 · 12 comments
Labels
RFC Request For Comments

Comments

@p6rt
Copy link

p6rt commented May 29, 2016

Migrated from rt.perl.org#128283 (status was 'rejected')

Searchable as RT128283$

@p6rt
Copy link
Author

p6rt commented May 29, 2016

From @zoffixznet

The operators for `cmp-ok` subroutine can be given as strings or as Callables. The problem with strings is the subroutine evals them as EVAL "&infix​:<$op>" and the current implementation makes it impossible to use, say '<' as an operator​:

  <Zoffix> m​: use Test; cmp-ok 2, '<', 5, 'seems sane';
  <camelia> rakudo-moar beb3c9​: OUTPUT«not ok 1 - seems sane␤␤# Failed test 'seems sane'␤# at /tmp/kqui3siS7u line 1␤# Could not use '<' as a comparator␤»

It does work with &[<] instead of '<' and considering that's only 1 character longer and there's no 100% clean way to allow any op in a string, I propose we ditch the string version entirely.

While I don't know anything about SETTINGS, the current implementation also has a security hole, at least in camelia where `run`, while restricted in general code, is given a free pass when injected inside the string comparator passed to cmp-ok​:

  <Zoffix> m​: run "ls"
  <camelia> rakudo-moar beb3c9​: OUTPUT«run is disallowed in restricted setting␤ in sub restricted at src/RESTRICTED.setting line 1␤ in sub run at src/RESTRICTED.setting line 14␤ in block <unit> at /tmp/wWcCMBi30n line 1␤␤»
  <Zoffix> m​: use Test; cmp-ok '', '~~>;warn run "ls"; <z', '', '';
  <camelia> rakudo-moar beb3c9​: OUTPUT«Perlito␤dalek-queue␤evalbot␤evalbot.log␤foo␤lib␤log␤mbox␤nqp-js␤p1␤p2␤p6eval-token␤perl5␤rakudo-j-1␤rakudo-j-2␤rakudo-j-inst␤rakudo-j-inst-1␤rakudo-j-inst-2␤rakudo-m-1␤rakudo-m-2␤rakudo-m-inst␤rakudo-m-inst-1␤rak…»

@p6rt
Copy link
Author

p6rt commented May 29, 2016

From @smls

Sounds like throwing out the baby with the bath water, to me.

there's no 100% clean way to allow any op in a string

What makes you so sure?
EVAL is not the only way to this this, nor arguably the correct way.

For example, the following works​:

  my $name = "<";

  my &op = CORE​::.first(*.key eq "\&infix​:<$name>" | "\&infix​:«$name»").value;

  if &op {
  say op 5, 42; # True
  say op 42, 5; # False
  }

This should be immune to injection attacks too.

Using low-level NQP code to do this, might make it even more robust (i.e. avoid having to check both <$name> and «$name» against the string representation, and instead check for the routine name directly).

@p6rt
Copy link
Author

p6rt commented May 29, 2016

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

@p6rt
Copy link
Author

p6rt commented May 29, 2016

From @smls

Ok, admittedly using `.first` to index the PseudoStash is a little silly.
Something like this is probably saner​:

  my &op = CORE​::{"\&infix​:<$name>"} //
  CORE​::{"\&infix​:«$name»"};

@p6rt
Copy link
Author

p6rt commented May 29, 2016

From @smls

Or this​:

  my &op = (try &​::("infix​:<$name>")) //
  (try &​::("infix​:«$name»"));

@p6rt
Copy link
Author

p6rt commented May 30, 2016

From @zoffixznet

On Sun May 29 08​:51​:45 2016, smls75@​gmail.com wrote​:

What makes you so sure?

I asked the channel about this before and no one gave a satisfying answer. I go off my knowledge. If you know a way, state it; there's no need to attack my knowledge of the language.

Playing with the solutions you offered, I see that they they don't work with custom user-defined operators and likely break with operators containing both > and » in them.

I think psch++ came up with a working solution somewhere here, but there are too many variants for me to follow​: http://irclog.perlgeek.de/perl6-dev/2016-05-29#i_12568075

@p6rt
Copy link
Author

p6rt commented May 30, 2016

From @smls

there's no need to attack my knowledge of the language

Sorry, wasn't meant as an attack.
It's just that you sounded very sure when you said there's no way to implement it cleanly, so I wondered if there had been a proper review leading to that conclusion.

Playing with the solutions you offered, I see that they they don't
work with custom user-defined operators and likely break with
operators containing both > and » in them.

The last one works with user-defined operators.

To make it also work with operators containing both > and », it can be extended as follows​:

  my &op = &​::("infix​:<$name>")
  // &​::("infix​:«$name»")
  // &​::("infix​:<{$name.subst(/<before <[<>]> >/, "\\", :g)}>")
  // die "Infix operator $name not found.";

@p6rt
Copy link
Author

p6rt commented May 30, 2016

From @zoffixznet

The last one works with user-defined operators.

It works only when the custom operators are in scope. Is there a way to make it work inside Test.pm6 code?

  zoffix@​leliana​:/tmp/tmp.avxBKNwr4B$ cat foo.p6
  sub cmp ($name, $lhs, $rhs) {
  my &op = &​::("infix​:<$name>")
  // &​::("infix​:«$name»")
  // &​::("infix​:<{$name.subst(/<before <[<>]> >/, "\\", :g)}>")
  // die "Infix operator $name not found.";
  op($lhs, $rhs);
  }
 
  {
  sub infix​:<foo> { $^a &lt; $^b };
  say 2 foo 3;
  say cmp 'foo', 2, 3;
  }
 
  zoffix@​leliana​:/tmp/tmp.avxBKNwr4B$ perl6 foo.p6
  True
  Infix operator foo not found.
  in sub cmp at foo.p6 line 2
  in block <unit> at foo.p6 line 13

@p6rt
Copy link
Author

p6rt commented May 30, 2016

From @zoffixznet

While wordy, this appears to work and removes all the issues raised in this ticket​:

  sub cmp ($name, $a, $b) {
  my &op = &​::("infix​:<$name>")
  // &​::("infix​:«$name»")
  // &​::("infix​:<$name.subst(/<before <[<>]> >/, "\\", :g)>")
  // &CALLER​::("infix​:<$name>")
  // &CALLER​::("infix​:«$name»")
  // &CALLER​::("infix​:<$name.subst(/<before <[<>]> >/, "\\", :g)>")
  // die "Infix operator $name not found.";

  op($a, $b);
  }

  {
  sub infix​:<< <» >> { $^a + $^b }
  sub infix​:<< «> >> { $^a + $^b }
  sub infix​:<< «» >> { $^a + $^b }
  say cmp '<', 3, 4;
  say cmp '>', 3, 4;
  say cmp '<»', 3, 4;
  say cmp '«>', 3, 4;
  say cmp '«»', 3, 4;
  }

I'll submit a PR and add extra tests for this to roast tonight or tomorrow.

@p6rt
Copy link
Author

p6rt commented May 30, 2016

From @smls

Unfortunately, `CALLER​::` only contains symbols that were lexically declared directly in the caller's scope, and not those inherited from its parent scopes.

I'm not sure if a proper run-time lookup of a lexical symbol as seen from the caller scope is possible. Maybe there's some NQP trickery to do it? Ask jnthn, he is the expert on scopes and callframes...

@p6rt
Copy link
Author

p6rt commented Jun 16, 2016

From @zoffixznet

Closing due to lack of interest.

Will address the mentioned security issue in a few days.

@p6rt
Copy link
Author

p6rt commented Jun 16, 2016

@zoffixznet - Status changed from 'open' to 'rejected'

@p6rt p6rt closed this as completed Jun 16, 2016
@p6rt p6rt added the RFC Request For Comments label Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request For Comments
Projects
None yet
Development

No branches or pull requests

1 participant