Fixed some fragility in the search plugin.

Fixed some fragility in the search plugin.

[scudette, 2016-04-14 18:10:17]

While trying out the search plugin I found lots of breakages.

Specifically I tried to do:

search "procdump(eprocess: (select _EPROCESS from pslist where
_EPROCESS.FullPath =~ '.+Users')._EPROCESS)"

[scudette, 2016-04-14 20:04:31]

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/plugins/co...
File rekall-core/rekall/plugins/common/search.py (right):

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/plugins/co...
rekall-core/rekall/plugins/common/search.py:266: with renderer.start():
This usage is problematic - we really need to have a way to pass the original
renderer down into the search plugin's sub plugin.

[the80srobot, 2016-04-15 02:00:57]

Thanks for fixes! I had some questions inline.

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/plugins/co...
File rekall-core/rekall/plugins/common/search.py (right):

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/plugins/co...
rekall-core/rekall/plugins/common/search.py:265: renderer =
identity_renderer.IdentityRenderer(session=self.session)
Was something broken by the identity renderer not having a session? Just
wondering what this is fixing.

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/plugins/co...
rekall-core/rekall/plugins/common/search.py:266: with renderer.start():
On 2016/04/14 20:04:31, scudette wrote:
> This usage is problematic - we really need to have a way to pass the original
> renderer down into the search plugin's sub plugin.
> 

Why is it problematic? The IdentityRenderer captures a bunch of information
about the output, it's really handy for plugins that don't forward declare
headers.

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/ui/identit...
File rekall-core/rekall/ui/identity.py (right):

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/ui/identit...
rekall-core/rekall/ui/identity.py:78: # to the session renderer.
So I have no idea how this would behave if you actually tried to write a file
but my feeling is it would be bad.

The search plugin isn't really designed to deal with file output - anything that
has side effects, really, is probably not a great candidate for search.

[scudette, 2016-04-15 02:28:00]

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/plugins/co...
File rekall-core/rekall/plugins/common/search.py (right):

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/plugins/co...
rekall-core/rekall/plugins/common/search.py:265: renderer =
identity_renderer.IdentityRenderer(session=self.session)
On 2016/04/15 02:00:57, the80srobot wrote:
> Was something broken by the identity renderer not having a session? Just
> wondering what this is fixing.

Yes. The Identity renderer does not support writing files and so it explodes for
every plugin which tries to write files.

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/plugins/co...
rekall-core/rekall/plugins/common/search.py:266: with renderer.start():
On 2016/04/15 02:00:57, the80srobot wrote:
> On 2016/04/14 20:04:31, scudette wrote:
> > This usage is problematic - we really need to have a way to pass the
original
> > renderer down into the search plugin's sub plugin.
> > 
> 
> Why is it problematic? The IdentityRenderer captures a bunch of information
> about the output, it's really handy for plugins that don't forward declare
> headers.

It is quite problematic because it interfers with the operation of the original
renderer. Say we use this in GRR for example, we create a main renderer then
call the search plugin - but at some point it makes a completely new Identity
renderer (The bug here BTW is that it doesnt even start it so the existing
Identity renderer explodes - was this ever working?). 

The new plugin has no way to interact with the old renderer.

I understand that the idea is to buffer the rows and replay them into the old
renderer via the search plugin but this is only some of what the renderer
offers. In particular it needs to support proper heart beating (via progress
reporting), file writing etc. Since in general there is no way to know what the
original renderer is supposed to be doing, you cant solve this in all cases.

I think what needs to happen here is that maybe some of the calls be intercepted
and others be relayed to the parent renderer. In order to do this there has to
be a way to pass the actual original renderer down through the efilter stack so
the Identity renderer can delegate some methods to it.

This CL is not a good solution either because we just call session.GetRenderer()
which makes a new renderer, but this will not work in cases where the original
renderer has some state (e.g. GRR will fail here).

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/ui/identit...
File rekall-core/rekall/ui/identity.py (right):

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/ui/identit...
rekall-core/rekall/ui/identity.py:78: # to the session renderer.
On 2016/04/15 02:00:57, the80srobot wrote:
> So I have no idea how this would behave if you actually tried to write a file
> but my feeling is it would be bad.
> 
> The search plugin isn't really designed to deal with file output - anything
that
> has side effects, really, is probably not a great candidate for search.

Thats not the point. It does actually work and is a useful use case. Either
search supports the full API or not at all - you cant say - oh we only support
half the plugins which are good candidate, so expect explosions for some plugins
- WAI.

Renderers must be supported to their full API - not just some of it.

[the80srobot, 2016-04-15 02:34:15]

lgtm with comments

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/plugins/co...
File rekall-core/rekall/plugins/common/search.py (right):

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/plugins/co...
rekall-core/rekall/plugins/common/search.py:265: renderer =
identity_renderer.IdentityRenderer(session=self.session)
On 2016/04/15 02:28:00, scudette wrote:
> On 2016/04/15 02:00:57, the80srobot wrote:
> > Was something broken by the identity renderer not having a session? Just
> > wondering what this is fixing.
> 
> Yes. The Identity renderer does not support writing files and so it explodes
for
> every plugin which tries to write files.

Yeah, that was the design decision at the time - it was supposed to only work
for table output. (See my other comment - I'm not married to that idea, but
don't know what the expectation is).

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/plugins/co...
rekall-core/rekall/plugins/common/search.py:266: with renderer.start():
On 2016/04/15 02:28:00, scudette wrote:
> On 2016/04/15 02:00:57, the80srobot wrote:
> > On 2016/04/14 20:04:31, scudette wrote:
> > > This usage is problematic - we really need to have a way to pass the
> original
> > > renderer down into the search plugin's sub plugin.
> > > 
> > 
> > Why is it problematic? The IdentityRenderer captures a bunch of information
> > about the output, it's really handy for plugins that don't forward declare
> > headers.
> 
> It is quite problematic because it interfers with the operation of the
original
> renderer. Say we use this in GRR for example, we create a main renderer then
> call the search plugin - but at some point it makes a completely new Identity
> renderer (The bug here BTW is that it doesnt even start it so the existing
> Identity renderer explodes - was this ever working?). 
> 
> The new plugin has no way to interact with the old renderer.
> 
> I understand that the idea is to buffer the rows and replay them into the old
> renderer via the search plugin but this is only some of what the renderer
> offers. In particular it needs to support proper heart beating (via progress
> reporting), file writing etc. Since in general there is no way to know what
the
> original renderer is supposed to be doing, you cant solve this in all cases.
> 
> I think what needs to happen here is that maybe some of the calls be
intercepted
> and others be relayed to the parent renderer. In order to do this there has to
> be a way to pass the actual original renderer down through the efilter stack
so
> the Identity renderer can delegate some methods to it.
> 
> This CL is not a good solution either because we just call
session.GetRenderer()
> which makes a new renderer, but this will not work in cases where the original
> renderer has some state (e.g. GRR will fail here).

Got it - if this fixes something without breaking other tests then lets do it
like this until we've had a chance to decide what the proper behavior is.

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/ui/identit...
File rekall-core/rekall/ui/identity.py (right):

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/ui/identit...
rekall-core/rekall/ui/identity.py:78: # to the session renderer.
On 2016/04/15 02:28:00, scudette wrote:
> On 2016/04/15 02:00:57, the80srobot wrote:
> > So I have no idea how this would behave if you actually tried to write a
file
> > but my feeling is it would be bad.
> > 
> > The search plugin isn't really designed to deal with file output - anything
> that
> > has side effects, really, is probably not a great candidate for search.
> 
> Thats not the point. It does actually work and is a useful use case. Either
> search supports the full API or not at all - you cant say - oh we only support
> half the plugins which are good candidate, so expect explosions for some
plugins
> - WAI.
> 
> Renderers must be supported to their full API - not just some of it.

Well, when I wrote this the idea was to get something that covers 80-90% of
plugins if I recall the discussion. If we're going to use this as the interface
to any plugin then yes, I agree we need to figure out side effect management.

What do you think SHOULD happen if a plugin writes a file to output? The search
plugin currently operates under the assumption that everything is a table - I'm
sure we can write code to support non-table output, but I don't know what the
right/expected behavior is?

[scudette, 2016-04-24 19:22:17]

GetRenderer() now manages a stack of renderers so the Identity renderer can just
delegate to the last one in the stack those functions it does not implement.

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/plugins/co...
File rekall-core/rekall/plugins/common/search.py (right):

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/plugins/co...
rekall-core/rekall/plugins/common/search.py:265: renderer =
identity_renderer.IdentityRenderer(session=self.session)
On 2016/04/15 02:34:15, the80srobot wrote:
> On 2016/04/15 02:28:00, scudette wrote:
> > On 2016/04/15 02:00:57, the80srobot wrote:
> > > Was something broken by the identity renderer not having a session? Just
> > > wondering what this is fixing.
> > 
> > Yes. The Identity renderer does not support writing files and so it explodes
> for
> > every plugin which tries to write files.
> 
> Yeah, that was the design decision at the time - it was supposed to only work
> for table output. (See my other comment - I'm not married to that idea, but
> don't know what the expectation is).

Probably first expectation is to not explode :-).

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/plugins/co...
rekall-core/rekall/plugins/common/search.py:266: with renderer.start():
On 2016/04/15 02:34:15, the80srobot wrote:
> On 2016/04/15 02:28:00, scudette wrote:
> > On 2016/04/15 02:00:57, the80srobot wrote:
> > > On 2016/04/14 20:04:31, scudette wrote:
> > > > This usage is problematic - we really need to have a way to pass the
> > original
> > > > renderer down into the search plugin's sub plugin.
> > > > 
> > > 
> > > Why is it problematic? The IdentityRenderer captures a bunch of
information
> > > about the output, it's really handy for plugins that don't forward declare
> > > headers.
> > 
> > It is quite problematic because it interfers with the operation of the
> original
> > renderer. Say we use this in GRR for example, we create a main renderer then
> > call the search plugin - but at some point it makes a completely new
Identity
> > renderer (The bug here BTW is that it doesnt even start it so the existing
> > Identity renderer explodes - was this ever working?). 
> > 
> > The new plugin has no way to interact with the old renderer.
> > 
> > I understand that the idea is to buffer the rows and replay them into the
old
> > renderer via the search plugin but this is only some of what the renderer
> > offers. In particular it needs to support proper heart beating (via progress
> > reporting), file writing etc. Since in general there is no way to know what
> the
> > original renderer is supposed to be doing, you cant solve this in all cases.
> > 
> > I think what needs to happen here is that maybe some of the calls be
> intercepted
> > and others be relayed to the parent renderer. In order to do this there has
to
> > be a way to pass the actual original renderer down through the efilter stack
> so
> > the Identity renderer can delegate some methods to it.
> > 
> > This CL is not a good solution either because we just call
> session.GetRenderer()
> > which makes a new renderer, but this will not work in cases where the
original
> > renderer has some state (e.g. GRR will fail here).
> 
> Got it - if this fixes something without breaking other tests then lets do it
> like this until we've had a chance to decide what the proper behavior is.

I might rework it somewhat to try to pass the original renderer through because
i think that is the correct approach.

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/ui/identit...
File rekall-core/rekall/ui/identity.py (right):

https://codereview.appspot.com/297830043/diff/1/rekall-core/rekall/ui/identit...
rekall-core/rekall/ui/identity.py:78: # to the session renderer.
> What do you think SHOULD happen if a plugin writes a file to output? 

The file should be written :-).

> The search plugin currently operates under the assumption that everything is a
table - I'm
> sure we can write code to support non-table output, but I don't know what the
> right/expected behavior is?

I think the expectation is to run new plugins under the existing renderer - this
should provide all the right things to make things work.

[the80srobot, 2016-04-28 12:05:14]

Did you forget to submit this? :)

[scudette, 2016-05-04 18:31:23]

This was submitted as part of another cl (Refactored scanners to use a common
interface for selection of scanning regions.)

I will drop this cl now.