class and module attribute accessor refactoring

[plashchynski]

class and module accessors are not safe for an user input.
Here an example:

1.9.2p318 :001 > class HoleyClass; end
=> nil
1.9.2p318 :002 > HoleyClass.cattr_writer("abc; echo 1 > ~/1; @@some_var")
=> ["abc; echo 1 > ~/1; @@some_var"]
1.9.2p318 :003 > HoleyClass.cattr_reader("abc; echo 2 > ~/2; @@some_var")
=> ["abc; echo 2 > ~/2; @@some_var"]
1.9.2p318 :004 > HoleyClass.cattr_accessor("abc; echo 3 > ~/3; @@some_var")
=> ["abc; echo 3 > ~/3; @@some_var"]
1.9.2p318 :005 > HoleyClass.send(:attr_accessor, "abc; echo 3 > ~/3; @@some_var") # attr_accessor is safe
NameError: invalid attribute name abc;echo 3 > ~/3`; @@some_var'

Of course this is not a dangerous vulnerability. I hope no one passes an user input to these methods.
But if it is possible to make them safer, so why not?

[josevalim]

IIRC, previous Ruby versions had some memory leak related to define_method and their usage were reduced within Rails code base. Also, those versions are certainly slower than the current ones, but we need benchmarks to show if the performance matters at this point or not.

With this in mind, couldn't we simply go with a regexp based approach, that will simply check if the given input is a valid identifier (aka /\w+/) or not?

[plashchynski]

Yes, this implementation is much slower. Please take a look at the regex variant: #5668

[josevalim]

👍 for the regexp variant. What do you think @fxn?

[fxn]

The regexp approach seems OK to me: 👍.