Closed GoogleCodeExporter closed 8 years ago
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
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
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
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
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:
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
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
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
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
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
Original issue reported on code.google.com by
mike.auty@gmail.com
on 27 Aug 2010 at 7:58