possibly bulletproof eval()

[Giles Bowkett]

I've got a class which loads files and turns them into ActiveRecord DB
rows. I'm converting images on a filesystem into blobs in a database.

class ImageFile < ActiveRecord::Base
class << self
def import_from_hash(hash)
%w{medium square thumb lsquare lthumb tiny}.each do |suffix|
filename = "public/item/photos/" + hash[0..2] + "/" + hash +
"_#{suffix}.jpg"
if File.exists?(filename)
File.open(filename, "r") do |file|
image_file = ImageFile.new
eval ("image_file.#{suffix} = file.read")
end
end
end
end
end
end

As you can see the whole thing depends massively on eval(). Yet I
think it's safe. First, it's only supposed to be run from irb. Second,
the eval's guarded by an array which essentially acts as a filter. It
only ever evals code generated by this code and literals from the
array. (No user input.)

The suffixes match existing suffixes on the filesystem. They
correspond to different sizes of the same image. I'm essentially
refactoring an architecture here. That's why the redundant data in the
DB. Each image now being represented on the filesystem with
"xyz_square.jpg" will soon become the square attribute of an ImageFile
object. This isn't the cleanest design, but it beats the filesystem
version for my purposes (load-balance-ability and hardware stability).

Anyway, I think in addition to being safe, this code also shows
something which would end up **significantly** less concise without
eval(). It's kind of deliberately Lispy and functional-programmy. I
would never have written anything like this before reading
"Higher-Order Perl." I think it's a good example of eval() being
useful and safe. But, you know, please knock me down (if you see me
getting high and mighty).

 

[Robert Dober]

Anyway, I think in addition to being safe, this code also shows
something which would end up **significantly** less concise without
eval().

Hmm I dislike eval very strongly, therefore:

not tested but I think

image_file.send "#{suffix}=", file.read
or even (it it is an accessor)
image_file.instance_variable_set, "@" << suffix, file.read

should work too

It's kind of deliberately Lispy and functional-programmy. I

would never have written anything like this before reading
"Higher-Order Perl." I think it's a good example of eval() being
useful and safe. But, you know, please knock me down (if you see me
getting high and mighty). Who said that ?

--
Giles Bowkett

Blog: http://gilesbowkett.blogspot.com
Portfolio: http://www.gilesgoatboy.org

Cheers
Robert

 

[Eric Hodel]

I've got a class which loads files and turns them into ActiveRecord DB
rows. I'm converting images on a filesystem into blobs in a database.

class ImageFile < ActiveRecord::Base
class << self
def import_from_hash(hash)
%w{medium square thumb lsquare lthumb tiny}.each do |suffix|
filename = "public/item/photos/" + hash[0..2] + "/" + hash +
"_#{suffix}.jpg"
if File.exists?(filename)
File.open(filename, "r") do |file|
image_file = ImageFile.new
eval ("image_file.#{suffix} = file.read")
end
end
end
end
end
end

As you can see the whole thing depends massively on eval(). Yet I
think it's safe.

echo 'system "rm -rf /"' > public/item/photos/XX/Y_medium.jpg

 

[Eric Hodel]

I've got a class which loads files and turns them into
ActiveRecord DB
rows. I'm converting images on a filesystem into blobs in a database.

class ImageFile < ActiveRecord::Base
class << self
def import_from_hash(hash)
%w{medium square thumb lsquare lthumb tiny}.each do |suffix|
filename = "public/item/photos/" + hash[0..2] + "/" + hash +
"_#{suffix}.jpg"
if File.exists?(filename)
File.open(filename, "r") do |file|
image_file = ImageFile.new
eval ("image_file.#{suffix} = file.read")
end
end
end
end
end
end

As you can see the whole thing depends massively on eval(). Yet I
think it's safe.

echo 'system "rm -rf /"' > public/item/photos/XX/Y_medium.jpg

Hrm, sorry, no. Too tired to notice no #{} around file.read.

Still, far too dangerous, use #send instead.

 

[Brad Phelan]

I've got a class which loads files and turns them into ActiveRecord DB
rows. I'm converting images on a filesystem into blobs in a database.

class ImageFile < ActiveRecord::Base
class << self
def import_from_hash(hash)
%w{medium square thumb lsquare lthumb tiny}.each do |suffix|
filename = "public/item/photos/" + hash[0..2] + "/" + hash +
"_#{suffix}.jpg"
if File.exists?(filename)
File.open(filename, "r") do |file|
image_file = ImageFile.new
eval ("image_file.#{suffix} = file.read")
end
end
end
end
end
end

As you can see the whole thing depends massively on eval(). Yet I
think it's safe. First, it's only supposed to be run from irb. Second,
the eval's guarded by an array which essentially acts as a filter. It
only ever evals code generated by this code and literals from the
array. (No user input.)

The suffixes match existing suffixes on the filesystem. They
correspond to different sizes of the same image. I'm essentially
refactoring an architecture here. That's why the redundant data in the
DB. Each image now being represented on the filesystem with
"xyz_square.jpg" will soon become the square attribute of an ImageFile
object. This isn't the cleanest design, but it beats the filesystem
version for my purposes (load-balance-ability and hardware stability).

Anyway, I think in addition to being safe, this code also shows
something which would end up **significantly** less concise without
eval(). It's kind of deliberately Lispy and functional-programmy. I
would never have written anything like this before reading
"Higher-Order Perl." I think it's a good example of eval() being
useful and safe. But, you know, please knock me down (if you see me
getting high and mighty).

ActiveRecord has the write_attribute and read_attribute methods
which is a better choice for what you are doing. You can also
use [] and []= as aliases for write_attribute and read_attribute

http://ar.rubyonrails.org/classes/ActiveRecord/Base.html

Suffixes = %w{medium square thumb lsquare lthumb tiny}

class ImageFile < ActiveRecord::Base

def ImageFile.import_from_hash(hash)
Suffixes.each do |suffix|
filename =
"public/item/photos/" + hash[0..2] + "/" + hash +
"_#{suffix}.jpg"
if File.exists?(filename)
File.open(filename, "r") do |file|
image_file = ImageFile.new
self[suffix]=file.read
end
end
end
end
end

 

[James Edward Gray II]

As you can see the whole thing depends massively on eval().

I see you've already got the answers on eval(), which I completely
agree with.

Each image now being represented on the filesystem with
"xyz_square.jpg" will soon become the square attribute of an ImageFile
object. This isn't the cleanest design, but it beats the filesystem
version for my purposes (load-balance-ability and hardware stability).

My database teacher use to harp on and on about how file data
belonged in the file system (not a database). His primary reasoning
was that you can't meaningfully query it so you don't gain anything
from the database structure. I've also found that things get
complicated as soon as I have a large enough file stored in a database.

Just my two cents.

James Edward Gray II

 

[Ryan Davis]

My database teacher use to harp on and on about how file data
belonged in the file system (not a database). His primary
reasoning was that you can't meaningfully query it so you don't
gain anything from the database structure. I've also found that
things get complicated as soon as I have a large enough file stored
in a database.

Agreed, but you can't tell from his code sample that he's not doing
that already. We've got a similar model class (tree of classes
actually) whose @content doesn't go the db but gets saved in a
after_save handler instead. He might just be storing the useful stuff
in the db (dimensions, size, mimetype, and just having a row so
tagging and other referential things can happen).

 

[James Edward Gray II]

Agreed, but you can't tell from his code sample that he's not doing
that already. We've got a similar model class (tree of classes
actually) whose @content doesn't go the db but gets saved in a
after_save handler instead. He might just be storing the useful
stuff in the db (dimensions, size, mimetype, and just having a row
so tagging and other referential things can happen).

That's true. I handle it just like you do.

I made my assumption about how he was doing it from his comment, "I'm
converting images on a filesystem into blobs in a database."

James Edward Gray II

 

[Giles Bowkett]

ActiveRecord has the write_attribute and read_attribute methods

which is a better choice for what you are doing. You can also
use [] and []= as aliases for write_attribute and read_attribute

It's a class method which creates new instances. I don't think you can
get to write_attribute that way, because it's a private method.

 

[Giles Bowkett]

Each image now being represented on the filesystem with

My database teacher use to harp on and on about how file data
belonged in the file system (not a database). His primary reasoning
was that you can't meaningfully query it so you don't gain anything
from the database structure. I've also found that things get
complicated as soon as I have a large enough file stored in a database.

I have to admit, storing the images in the DB is kind of a lame hack.
We're doing it because we need to be able to load-balance the app
across three servers, and storing images on the filesystem the way the
app currently does it blocks the load-balancing. It could probably be
done more correctly if we had a dedicated image server, but there are
budget questions there.

 

[Giles Bowkett]

Agreed, but you can't tell from his code sample that he's not doing

that already. We've got a similar model class (tree of classes
actually) whose @content doesn't go the db but gets saved in a
after_save handler instead. He might just be storing the useful stuff
in the db (dimensions, size, mimetype, and just having a row so
tagging and other referential things can happen).

I wish! See the response about load balancing.

 

[Giles Bowkett]

image_file.send "#{suffix}=", file.read

That's probably the way I should do it actually. I always forget about
send for some reason.

I had to put tons of comments like "warning do not ever copy/paste
this!" in the code near the eval. Using send would get me around that.

 

[Ryan Davis]

I have to admit, storing the images in the DB is kind of a lame hack.
We're doing it because we need to be able to load-balance the app
across three servers, and storing images on the filesystem the way the
app currently does it blocks the load-balancing. It could probably be
done more correctly if we had a dedicated image server, but there are
budget questions there.

image server or look into using mogilefs

 

[Brad Phelan]

ActiveRecord has the write_attribute and read_attribute methods
which is a better choice for what you are doing. You can also
use [] and []= as aliases for write_attribute and read_attribute

It's a class method which creates new instances. I don't think you can
get to write_attribute that way, because it's a private method.

Not a problem! Just use use [] and []= public aliases for the protected
methods.

From the RDoc for ActiveRecord.

http://ar.rubyonrails.org/classes/ActiveRecord/Base.html#M000402

Public Instance Methods
-----------------------

[](attr_name)

Returns the value of the attribute identified by attr_name after it has
been typecast (for example, "2004-12-12" in a data column is cast to a
date object, like Date.new(2004, 12, 12)). (Alias for the protected
read_attribute method).

[ show source ]

# File lib/active_record/base.rb, line 1489
1489: def [](attr_name)
1490: read_attribute(attr_name)
1491: end

[]=(attr_name, value)

Updates the attribute identified by attr_name with the specified value.
(Alias for the protected write_attribute method).

[ show source ]

# File lib/active_record/base.rb, line 1495
1495: def []=(attr_name, value)
1496: write_attribute(attr_name, value)
1497: end

 

[Robert Dober]

Well, good, but as I do not know AR I might have missed out on the AR
specific solutions, maybe you should check out Brad's last post.

That's probably the way I should do it actually. I always forget about
send for some reason.

I had to put tons of comments like "warning do not ever copy/paste
this!" in the code near the eval. Using send would get me around that.

That for sure is a bad sign .
Robert