More refactoring of address space code - now supports paged out PDEs.

More refactoring of address space code - now supports paged out PDEs.

R=adam.sindelar@gmail.com

Committed: 0070307

[the80srobot, 2015-07-31 15:29:32]

Sorry for taking an extra day with nitpicks :)

https://codereview.appspot.com/255440043/diff/20001/rekall/plugins/windows/pa...
File rekall/plugins/windows/pagefile.py (right):

https://codereview.appspot.com/255440043/diff/20001/rekall/plugins/windows/pa...
rekall/plugins/windows/pagefile.py:237: if pde_value is None or pde_value == 0:
This looks like it might cause subtle bugs later on - why not 'not pde_value' ?

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
File rekall/plugins/addrspaces/amd64.py (right):

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/amd64.py:62: def describe_vtop(self, vaddr,
collection=None):
Can we have a docstring saying this is overriden here? The arguments aren't
obvious IMO, and the base method is documented.

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/amd64.py:135: ((vaddr & 0xff8000000000) >> 36))
So we repeat these masks all over the place - maybe this would work well as a
function?

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
File rekall/plugins/addrspaces/intel.py (right):

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/intel.py:89: class
AddressTranslationDescriptor(object):
This looks like it's an abstract base class - is that correct? Would you ever
instantiate this directly?

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/intel.py:105: def Render(self, renderer):
Why not have an object renderer?

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/intel.py:130: def add(self, descriptor_cls, **kwargs):
We're mixing CamelCase and lower_case method names in these new classes. Can we
pick a convention and use it throughout? :)

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/intel.py:153: 
Extra line here.

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/intel.py:235: '''Translates virtual addresses into
physical offsets.
We're using two different docstring formats in this file.

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/intel.py:262: 
Could we also document arguments here?

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
File rekall/plugins/addrspaces/mips.py (right):

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/mips.py:99: Return the offset in a 4KB memory page
from the given virtual
We're breaking with the docstring style in some of these files. Maybe we can
just do a cleanup CL later.

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/windows/p...
File rekall/plugins/windows/pagefile.py (right):

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/windows/p...
rekall/plugins/windows/pagefile.py:85: class
DemandZeroDescriptor(intel.AddressTranslationDescriptor):
Maybe a docstring here to say what a demand zero is?

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/windows/p...
rekall/plugins/windows/pagefile.py:150: class
VadPteDescriptor(WindowsPTEDescriptor):
Total nitpick: Parent class capitalizes all letters in PTE. This one should
probably be called VADPTEDescriptor to keep the style consistent.

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/windows/p...
rekall/plugins/windows/pagefile.py:190: (self.virtual_address - start) >> 12]
If you try to run this is a virtual_address < start it won't behave correctly. I
don't know if that's realistic, but we never check for it.

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/windows/v...
File rekall/plugins/windows/vadinfo.py (right):

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/windows/v...
rekall/plugins/windows/vadinfo.py:344: class
WindowsCollector(pfn.SummarizingAddressTranslationDescriptorCollector):
The codebase now has two classes named WindowsCollector, albeit in different
namespaces. Probably fine :) Might be confusing for new readers?

[scudette, 2015-07-31 17:20:17]

https://codereview.appspot.com/255440043/diff/20001/rekall/plugins/windows/pa...
File rekall/plugins/windows/pagefile.py (right):

https://codereview.appspot.com/255440043/diff/20001/rekall/plugins/windows/pa...
rekall/plugins/windows/pagefile.py:237: if pde_value is None or pde_value == 0:
On 2015/07/31 15:29:31, the80srobot wrote:
> This looks like it might cause subtle bugs later on - why not 'not pde_value'
?

Done.

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
File rekall/plugins/addrspaces/amd64.py (right):

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/amd64.py:62: def describe_vtop(self, vaddr,
collection=None):
On 2015/07/31 15:29:31, the80srobot wrote:
> Can we have a docstring saying this is overriden here? The arguments aren't
> obvious IMO, and the base method is documented.

Done.

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/amd64.py:135: ((vaddr & 0xff8000000000) >> 36))
On 2015/07/31 15:29:31, the80srobot wrote:
> So we repeat these masks all over the place - maybe this would work well as a
> function?

So it was a function but:
 - They are not used that often 
 - It makes reading harder because you have to look for the right version of the
function. Previous code uses inheritance heavily so the function is often
defined in the base class (and overridden possibly).

I think it is more obvious when you see the masks inline - especially when you
compare it with the manual - then reading the function is more linear.

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
File rekall/plugins/addrspaces/intel.py (right):

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/intel.py:89: class
AddressTranslationDescriptor(object):
On 2015/07/31 15:29:31, the80srobot wrote:
> This looks like it's an abstract base class - is that correct? Would you ever
> instantiate this directly?

Yes it is instantiate directly. Its used to describe anything that has a value
and an associated address (e.g. pte, pde, pdpte etc).

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/intel.py:105: def Render(self, renderer):
On 2015/07/31 15:29:32, the80srobot wrote:
> Why not have an object renderer?

Im not sure what an object renderer has to do with this?

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/intel.py:130: def add(self, descriptor_cls, **kwargs):
On 2015/07/31 15:29:31, the80srobot wrote:
> We're mixing CamelCase and lower_case method names in these new classes. Can
we
> pick a convention and use it throughout? :)

Done.

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/intel.py:153: 
On 2015/07/31 15:29:32, the80srobot wrote:
> Extra line here.

Done.

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/intel.py:235: '''Translates virtual addresses into
physical offsets.
On 2015/07/31 15:29:32, the80srobot wrote:
> We're using two different docstring formats in this file.

Done.

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/intel.py:262: 
On 2015/07/31 15:29:31, the80srobot wrote:
> Could we also document arguments here?

Done.

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
File rekall/plugins/addrspaces/mips.py (right):

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/mips.py:99: Return the offset in a 4KB memory page
from the given virtual
On 2015/07/31 15:29:32, the80srobot wrote:
> We're breaking with the docstring style in some of these files. Maybe we can
> just do a cleanup CL later.

Yeah these are hold overs from the old Volatility codebase.

The file (mips) needs a lot of maintenance but we dont use it right now for
anything.

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/windows/p...
File rekall/plugins/windows/pagefile.py (right):

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/windows/p...
rekall/plugins/windows/pagefile.py:85: class
DemandZeroDescriptor(intel.AddressTranslationDescriptor):
On 2015/07/31 15:29:32, the80srobot wrote:
> Maybe a docstring here to say what a demand zero is?

Done.

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/windows/p...
rekall/plugins/windows/pagefile.py:150: class
VadPteDescriptor(WindowsPTEDescriptor):
On 2015/07/31 15:29:32, the80srobot wrote:
> Total nitpick: Parent class capitalizes all letters in PTE. This one should
> probably be called VADPTEDescriptor to keep the style consistent.

Maybe but then would we not merge two acronyms to make a third acronym: VADPTE
-> VAD PTE ... too many three letter words.

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/windows/p...
rekall/plugins/windows/pagefile.py:190: (self.virtual_address - start) >> 12]
On 2015/07/31 15:29:32, the80srobot wrote:
> If you try to run this is a virtual_address < start it won't behave correctly.
I
> don't know if that's realistic, but we never check for it.

Since the virtual address of the PTE must lie inside the PrototypePTE array this
can not happen - unless there is a serious bug in the calling code. 

From the point of view of the code though an array with a negative reference
just means to go back (like in C) so it does behave as it should.

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/windows/v...
File rekall/plugins/windows/vadinfo.py (right):

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/windows/v...
rekall/plugins/windows/vadinfo.py:344: class
WindowsCollector(pfn.SummarizingAddressTranslationDescriptorCollector):
On 2015/07/31 15:29:32, the80srobot wrote:
> The codebase now has two classes named WindowsCollector, albeit in different
> namespaces. Probably fine :) Might be confusing for new readers?

Done.

[the80srobot, 2015-07-31 17:49:15]

LGTM thanks!

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
File rekall/plugins/addrspaces/intel.py (right):

https://codereview.appspot.com/255440043/diff/110001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/intel.py:105: def Render(self, renderer):
On 2015/07/31 17:20:17, scudette wrote:
> On 2015/07/31 15:29:32, the80srobot wrote:
> > Why not have an object renderer?
> 
> Im not sure what an object renderer has to do with this?

I guess these are never rendered directly, nevermind.

https://codereview.appspot.com/255440043/diff/170001/rekall/plugins/addrspace...
File rekall/plugins/addrspaces/arm.py (right):

https://codereview.appspot.com/255440043/diff/170001/rekall/plugins/addrspace...
rekall/plugins/addrspaces/arm.py:130: '''Translates virtual addresses into
physical offsets.
Single-quoted docstring

[scudette, 2015-08-01 00:21:20]

Committed patchset #10 manually as r0070307 (presubmit successful).