wren-lang / wren

The Wren Programming Language. Wren is a small, fast, class-based concurrent scripting language.
http://wren.io
MIT License
6.86k stars 550 forks source link

Long duplicate method names (even under 64 characters) leads to stack overflow in parser #1036

Open graphitemaster opened 3 years ago

graphitemaster commented 3 years ago

This is similar to issue #1034 except the method names are not actually longer than 64 characters, they're at 58 characters in length. The body of the method is empty too, and there is no invalid tokens necessary.

It looks like length management is messed up under duplicate names and that alone leads to stack overflow.

class Foo {
  iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii {}
  iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii {}
}

Like the other issue, adding more is if it does not crash will make it crash too if it isn't already. This one I think is more interesting from a user perspective because the identifier is less than 64 characters, adding more to exceed it is not surprising either, though it shouldn't crash in this fashion. Without stack protector this leads to a segfault in my case.

Output

[graphite@graphite bin]# ./wren_test a.wren 
[./a line 3] Error at 'iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii': Class Foo already defines a method 'iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii'.
*** stack smashing detected ***: terminated
Aborted (core dumped)
joshgoebel commented 2 years ago

Can we fix this with a simple:

#define ERROR_MESSAGE_SIZE (80 + MAX_VARIABLE_NAME + MAX_VARIABLE_NAME + 15)

The problem is we need to allow for MAX_VARIABLE_NAME to be included TWICE - since that's what we do in some error messages - or else change the error message to not include the function name.

Thoughts?