Asp Forum - possibly bulletproof eval

Giles Bowkett

7/17/2007 5:52:00 AM

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).

--
Giles Bowkett

Blog: http://gilesbowkett.bl...
Portfolio: http://www.gilesg...

14 Answers

Robert Dober

7/17/2007 6:39:00 AM

On 7/17/07, Giles Bowkett <gilesb@gmail.com> wrote:
<snip>
> 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.bl...
> Portfolio: http://www.gilesg...
>
>
Cheers
Robert
--
I always knew that one day Smalltalk would replace Java.
I just didn't know it would be called Ruby
-- Kent Beck

Eric Hodel

7/17/2007 8:21:00 AM

On Jul 16, 2007, at 22:51, Giles Bowkett wrote:

> 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

--
Poor workers blame their tools. Good workers build better tools. The
best workers get their tools to do the work for them. -- Syndicate Wars

Eric Hodel

7/17/2007 8:25:00 AM

On Jul 17, 2007, at 01:18, Eric Hodel wrote:
> On Jul 16, 2007, at 22:51, Giles Bowkett wrote:
>> 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.

--
Poor workers blame their tools. Good workers build better tools. The
best workers get their tools to do the work for them. -- Syndicate Wars

Brad Phelan

7/17/2007 8:29:00 AM

Giles Bowkett wrote:
> 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...

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

--
Brad Phelan
http://xt...

James Gray

7/17/2007 1:42:00 PM

On Jul 17, 2007, at 12:51 AM, Giles Bowkett wrote:

> 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

7/17/2007 3:43:00 PM

On Jul 17, 2007, at 06:42 , James Edward Gray II wrote:

> 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 Gray

7/17/2007 3:49:00 PM

On Jul 17, 2007, at 10:42 AM, Ryan Davis wrote:

>
> On Jul 17, 2007, at 06:42 , James Edward Gray II wrote:
>
>> 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).

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

7/17/2007 5:47:00 PM

> 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

Blog: http://gilesbowkett.bl...
Portfolio: http://www.gilesg...

Giles Bowkett

7/17/2007 5:48:00 PM

> > 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.

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

Blog: http://gilesbowkett.bl...
Portfolio: http://www.gilesg...

Giles Bowkett

7/17/2007 5:49:00 PM

> 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

Blog: http://gilesbowkett.bl...
Portfolio: http://www.gilesg...

comp.lang.ruby

possibly bulletproof eval

Giles Bowkett

Robert Dober

Eric Hodel

Eric Hodel

Brad Phelan

James Gray

Ryan Davis

James Gray

Giles Bowkett

Giles Bowkett

Giles Bowkett

x Login to ForumsZone