janet-lang / janet

A dynamic language and bytecode vm
https://janet-lang.org
MIT License
3.48k stars 225 forks source link

Potential arity check bug in peg.c #1335

Closed nrser closed 10 months ago

nrser commented 10 months ago

https://github.com/janet-lang/janet/blob/772f4c26e82ae0fd111451390c45bbf4763e9ae9/src/core/peg.c#L1655

It looks like this may be using get_replace as min for janet_arity when it should be using the min variable computed on the previous line?

The way I read the current logic is

  1. For peg/match, peg/find and peg/find-all check that arity is zero or greater.
  2. For peg/replace and peg/replace-all check that arity is one or greater.

This looks like a bug because the next line uses argv[0], which is not guaranteed to be there by for match, find and find-all. You can see from calling match with no arguments that it clears the arity check and dies trying to handle a peg that isn't there:

repl:1:> (peg/match)
error: grammar error in nil, unexpected peg source
  in peg/match [src/core/peg.c] on line 1704
  in _thunk [repl] (tailcall) on line 1, column 1

if you try peg/replace with no arguments you'll hit the arity check as get_replace will be 1:

repl:2:> (peg/replace)
error: arity mismatch, expected at least 1, got 0
  in peg/replace [src/core/peg.c] on line 1779
  in _thunk [repl] (tailcall) on line 2, column 1

I don't know if this has any actual effect other than less useful error messages. I've just started looking at the peg source regarding adapting it for another use case so I can't say I understand it in any grader sense.

pepe commented 10 months ago

I got the same impression. I will compile Janet with your proposed variant and see what happens. Thanks for the heads up.

pepe commented 10 months ago

I can confirm that the behavior with the fix is indeed correct.

pepe commented 10 months ago

I created PR in #1336

pepe commented 10 months ago

Thank you for reporting this error! Please check the fixed version and close this issue, if you find it satisfactory.