CType object attributes conflict with member names

[GoogleCodeExporter]

Hiya guys,

Just a quick note that since CTypes have a self.name, self.offset, self.theType 
and so on, those values can't then be used for attributes.  This probably most 
affects automatic generation of vtypes from a pdb, although, in general, 
generated members all start with a capital letter, and all the object 
attributes start with a lower case letter.  Unfortunately we already have 
_CLS_LSN in the vista and 7 vtypes which has an "offset" member, which will be 
in accessible to any plugin that might want it.

We've got a few options, and I don't think this'll get solved quickly, since 
they're all relatively invasive, and it's not clear how big a problem this will 
be.  However, best to have a place holder for discussions and so on.

So, as I see it options are:

a) Ensure the pdb generator mangles any member names so that they're not the 
same as BaseObject/CType attributes (mostly by case mangled).  There *may* be 
situations where a vtype has two members differentiated only by case, but I 
doubt it.  This is by far the easiest option.

b) Change all the attributes, trying very hard to avoid introducing bugs 
because of it, and probably failing because things ask for .offset and .vm all 
over the place.  Changing them to _blah or __blah won't work because there are 
members generated with that start with those too, so probably best to go with 
vol_blah or even _volatility_blah for total certainty.

c) Some other ingenious option that I've completely neglected and would never 
have thought of in a million years that simultaneously solves all the problems 
without any drawbacks...  5:)

Original issue reported on code.google.com by mike.auty@gmail.com on 27 Aug 2010 at 7:58

[GoogleCodeExporter]

Also, we should probably document somewhere that if people are going to write 
their own vtypes files, they should stick to the convention of having members 
start with uppercase letters (there really is a reason to the convention!).  5:)

Original comment by mike.auty@gmail.com on 27 Aug 2010 at 8:08

[GoogleCodeExporter]

Although name clashes can happen they are a big deal. You said that if eg. a 
struct has "offset" member then it will be inaccessible, but this is not true - 
it can still be accessible by obj.m("offset"). Its just a little less intuitive 
for these special fields.

We could prefix all internal fields by a special prefix but that might change a 
whole lot of code.

Another solution is to alias the member name using overlays - for example for 
offset change the name to Offset = lambda x: x.m("offset")  According to the 
standard aliasing method.

I personally do not think this is a big issue because those ambiguous members 
can actually be reachable - its just a little less convenient (it might be 
confusing though for new developers).

Original comment by scude...@gmail.com on 6 Sep 2010 at 8:53

[GoogleCodeExporter]

I can see some bugs coming about because people blindly asked for ctype.offset 
without realizing.  If we're thinking of aliasing the member name, why not just 
alter the vtype generation tool to only ever produce members that start with 
capital letters?  There shouldn't be a name clash between upper- and lower-case 
members, since I doubt even Microsoft would do that.  That should solve all the 
problems, but it still needs doing.  Perhaps we should move the vtype 
generation code into the volatility repository (or even make it a plugin)?

Original comment by mike.auty@gmail.com on 6 Sep 2010 at 10:16

[GoogleCodeExporter]

I think this could be even more confusing for people who are working with say 
Linux kernel code where the style convention is very different - and never 
starts with a capital. I think we should not fix this.

On a sidenote I think we should change the offset member to have a getter (e.g. 
get_offset()) and make it private.

I am all for moving the vtype generation code into the repository. This way we 
can generate both from header files and pdbs.

Original comment by scude...@google.com on 23 Sep 2010 at 6:49

[GoogleCodeExporter]

Right, some progress.  So thinking about it again, I spotted that the object 
defined value will always win over a member, which is a safe course of action.  
This also means that the user can always request m("offset") as scudette 
mentioned (and I seemingly didn't get my head around), so that's all good, and 
in fact, I'm happy to mark this as won't fix once we've dealt with the 
remaining discussion...

So I've mocked up a patch for the framework, and a few of the plugins 
(basically pslist), to covert them to get_offset() with a private _vol_offset 
member (which hopefully no debug symbols will conflict with).

However, I'm not sure why we'd be doing this just for offset, rather than also 
for things like parent and/or vm, all of which are pretty vital for the object 
model?  Also, I'm not yet convinced it a good idea (hence why I only mocked it 
up), because it makes any plugins that use .offset (which seemingly many of 
them do), a big uglier/more cumbersome to code.  As I say, the patch is for 
people to take a look and see what they think...

As for moving the vtype generation code into the repository, I hadn't realized 
it's already in a googlecode project run by moyix (now CCed in).  If we need to 
make any changes, perhaps it would just be best to ask him for commit access to 
pdbparse, rather than copy/fork it into volatility?

Original comment by mike.auty@gmail.com on 9 Nov 2010 at 2:25

Attachments:

• volatility-private-offset.patch

[GoogleCodeExporter]

For reasons of clean style it might be best to use a private _offset member for 
the VType so I like the patch - we make users use an accessor to get at the 
offset. This seems like a good approach to me.

Original comment by scude...@gmail.com on 9 Nov 2010 at 7:40

[GoogleCodeExporter]

Ok, I guess my question is more, why aren't we doing that for other important 
members of the object classes?  It seems a bit of an odd interface to allow 
people:

thing.name
thing.parent
thing.vm
thing.profile
thing.get_offset()

I'd suggest we either do it for all/most or for none at all.  Now the question 
is which?

Original comment by mike.auty@gmail.com on 9 Nov 2010 at 12:41

[GoogleCodeExporter]

Right, it was decided in the last volatility dev meeting that we should be 
making as many of these private as we can and introducing getters for them.

However, get_offset() and get_parent() feel like they'd be a little cumbersome, 
particularly for such vital members so I'm intending to use @property to turn 
these into read-only attributes (which can later be extended with setters if 
necessary).

The question is the name, since we're trying to avoid name clashes.  Even if we 
used get_offset and so on, there'd still be the risk of a clash, the question 
is just how unlikely it is, so the question becomes given these are members 
we'll most likely want to call most often, what should we name them so that 
they're easy to read, easy to code and easy to remember?

I'm currently thinking of going with v_offset, and v_parent and v_vm, because 
that will keep the code clean and relatively readable, and hopefully the 
underscore will differentiate it from other structures (most of the windows 
structures currently in our vtypes library seem to either start or not contain 
underscores).  One remaining option is to use vol_offset, however this change 
will be easy to make once the code's all in place because both v_ and vol_ are 
relatively unique within the codebase, so a sweeping change should be easy to 
implement.

Original comment by mike.auty@gmail.com on 24 Nov 2010 at 1:50

[GoogleCodeExporter]

Ok, I just committed a massive set of changes to the tree (r546-rt550), which 
cleans up the object interface to ensure that all import volatility members are 
accessible using v_<member>, so v_name, v_offset, v_parent and v_vm.  There's 
no longer a profile member, since this should be accessed by the vm (although 
we can create a convience property that returns it from the underlying vm).  
Also, the theType variable no longer has a public interface, since it was never 
used externally in the tree.

Technically there are still several remaining members: rebase, proxied, 
newattr, write, m, is_valid, cast, v, d, etc.  The only one that really 
concerns me is size, although that doesn't seem to get called too often, so it 
should be possibly to swap out for a different method or a property.  Either 
way, since this is primarily fixed, I'm going to mark it as such.   I will 
still be watching it in case anyone wants to add anything to it... 

Original comment by mike.auty@gmail.com on 24 Nov 2010 at 7:08

• Changed state: Fixed

[GoogleCodeExporter]

Right, after a few comments on channel, I've since changed the names from 
v_{name,offset,parent,vm} to obj_{name,offset,parent,vm}, since that's a little 
more descriptive and makes sense.

It's not too late to change it again if there's more complaints, but otherwise 
I think I've tinkered with the tree enough for today...  5:)

Original comment by mike.auty@gmail.com on 24 Nov 2010 at 7:28