Add new safe eval for trytond

[bch, 2009-09-21 13:54:48]

I'm a bit worried because safe_eval is quite slow. Since most of the "eval-ed"
strings comes from the db, I though about checking them before writing it in the
db (and use the normal eval after), but for some of the evaluations a
environment is needed.

http://codereview.appspot.com/121052/diff/1/9
File trytond/ir/module/module.py (right):

http://codereview.appspot.com/121052/diff/1/9#newcode396
Line 396: or mod.website != tryton.get('website', ''):
Not related to the eval modification. (idem for next chunk).

http://codereview.appspot.com/121052/diff/1/11
File trytond/model/browse.py (right):

http://codereview.appspot.com/121052/diff/1/11#newcode280
Line 280: return super(EvalEnvironment, self).get(item, default=default)
This chunk is not related to the eval modification.

[ced1, 2009-09-21 14:18:18]

On 2009/09/21 13:54:48, bch wrote:
> I'm a bit worried because safe_eval is quite slow.

It is the instantiation of Expression that is slow because it is parsed by
compiler.parse. The evaluation is as fast as standard eval.

> Since most of the "eval-ed"
> strings comes from the db, I though about checking them before writing it in
the
> db (and use the normal eval after), but for some of the evaluations a
> environment is needed.

This is a bad idea because if the simplest way to attack the server will be to
be specific string in the database because it is the place where data can be
written.

We can add a Cache on safe_eval to keep latest Expression and reuse them instead
of instantiate new one.

> 
> http://codereview.appspot.com/121052/diff/1/9
> File trytond/ir/module/module.py (right):
> 
> http://codereview.appspot.com/121052/diff/1/9#newcode396
> Line 396: or mod.website != tryton.get('website', ''):
> Not related to the eval modification. (idem for next chunk).

It is because safe eval return unicode string.

> 
> http://codereview.appspot.com/121052/diff/1/11
> File trytond/model/browse.py (right):
> 
> http://codereview.appspot.com/121052/diff/1/11#newcode280
> Line 280: return super(EvalEnvironment, self).get(item, default=default)
> This chunk is not related to the eval modification.

It is because safe eval use a default value when calling get on data.

[bch, 2009-09-21 14:45:32]

On 2009/09/21 14:18:18, ced wrote:
>> I'm a bit worried because safe_eval is quite slow.
> 
> It is the instantiation of Expression that is slow because it is parsed
> by compiler.parse. The evaluation is as fast as standard eval.
> 

This is the test I used to benchmark both eval:
http://hpaste.org/fastcgi/hpaste.fcgi/view?id=9657#a9657

>> Since most of the "eval-ed"
>> strings comes from the db, I though about checking them before writing
> it in the
>> db (and use the normal eval after), but for some of the evaluations a
>> environment is needed.
> 
> This is a bad idea because if the simplest way to attack the server will
> be to be specific string in the database because it is the place where
> data can be written.

I don't understand this point.

> We can add a Cache on safe_eval to keep latest Expression and reuse them
> instead of instantiate new one.

Yes, I thought of it too, but there is a risk of creating a big cache 
that is rarely read (for example if there is a lot of users or if a user 
always access different records).

[ced1, 2009-09-21 14:51:53]

On 2009/09/21 14:45:32, bch wrote:
> On 2009/09/21 14:18:18, ced wrote:
> >> Since most of the "eval-ed"
> >> strings comes from the db, I though about checking them before writing
> > it in the
> >> db (and use the normal eval after), but for some of the evaluations a
> >> environment is needed.
> > 
> > This is a bad idea because if the simplest way to attack the server will
> > be to be specific string in the database because it is the place where
> > data can be written.
> 
> I don't understand this point.

Look at my security hole in OpenERP. The first thing I do is to modify the data
in the database. This is the simplest way because the server can write in the
database.

> 
> > We can add a Cache on safe_eval to keep latest Expression and reuse them
> > instead of instantiate new one.
> 
> Yes, I thought of it too, but there is a risk of creating a big cache 
> that is rarely read (for example if there is a lot of users or if a user 
> always access different records).

I put a limit on the size.

[bch, 2009-09-22 11:01:54]

On 2009/09/22 10:19:58, ced wrote:
> 

OK for me

[bch, 2009-09-22 11:49:12]

http://codereview.appspot.com/121052/diff/1113/107
File trytond/tools/misc.py (right):

http://codereview.appspot.com/121052/diff/1113/107#newcode569
Line 569: _ALLOWED_CODES = [dis.opmap[x] for x in [
"_ALLOWED_CODES = set(dis.opmap[x] for x in [ ..."

will be a bit quicker

[bch, 2009-09-23 10:27:42]

On 2009/09/23 09:15:33, ced wrote:
> 

OK

[yangoon1, 2009-09-24 09:51:07]

Testing.

[yangoon1, 2009-09-24 12:47:23]

I don't know for sure, if it is related to this patch, but printing of general
ledger is really slooow. It took more than a minute to print a report of 33
pages and it didn't seem, that OOo was the bottleneck.

[ced1, 2009-09-24 16:13:19]

On 2009/09/24 12:47:23, yangoon wrote:
> I don't know for sure, if it is related to this patch, but printing of general
> ledger is really slooow. It took more than a minute to print a report of 33
> pages and it didn't seem, that OOo was the bottleneck.

It is not

[yangoon1, 2009-09-25 21:31:04]

just to give a liitle feedback: running with this patch discovered a wrong
import in modules, that was not detected before.

[yangoon1, 2009-09-27 13:31:19]

On 2009/09/25 21:31:04, yangoon wrote:
> just to give a liitle feedback: running with this patch discovered a wrong
> import in modules, that was not detected before.

forget it, it was removed function send_email, that caused wrong import...