-
Notifications
You must be signed in to change notification settings - Fork 480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Linux: update maple tree extension to fix issue #1032 #1034
Linux: update maple tree extension to fix issue #1032 #1034
Conversation
Hello, I've been digging in to work out what is going on here that caused the issue, and it is very much a python thing rather than anything at all to do with vol that I didn't understand at the time. It's explained here - and just something I didn't understand - https://rules.sonarsource.com/python/RSPEC-5717/ It's very obvious with this example.
Each call to
Take this further snippet, which doesn't use any parts of vol, but I try to mimic the maple tree parsing. To show why this affects the parsing.
If you call parent it will yield from child a few random numbers, but never the same number twice as this The default value for the seen set in parent is an empty set, so I had falsely assumed that each time you call parent without specifying seen that the default would be used - e.g. an empty set. However that is provably not true, notice how each time parent is called here
The starting_with_seen prints show that this set is growing each time parent is called without providing a new value for the the seen variable. If you call this but specify that an empty set should be used it works as expected.
That is why the maple tree parsing wouldn't work previously when called twice - the seen set was just growing. I thought it would start as an empty set, but that is not true. For the maple tree parsing to work that set must start as an empty set - else it will think it's already seen the root node and stop. |
Thanks ! |
Ah good spot! I'm usually pretty good about spotting so-called mutable types in default parameters, but that one snuck by. The solution is to default to None and then at the very start, detect if it's None and make it an empty set if so. None isn't mutable and will barf if it isn't changed when someone tries to use it as a set, so it acts as a good way to signify the need for initialization. I think that'd be a better long term fix, rather than just making sure that this one input is correct but leaving the problem open for others in the future. |
I believe the cleanest way to do this is make the current |
But if it's recursive, surely the call inside the function will populate that parameter, and it won't be an issue anyway? There shouldn't be a call where a value is implicitly expected rather than explicitly being passed in? |
…1032 correcting the mutable type used as a default parameter.
Hello @ikelos and @atcuno - thank you for taking a look at this. I've updated it now to set the default of 'seen' to be None. Then when it's None (e.g. the first call into it) a new empty set is created. I've added probably a very long and unneeded comment to explain why you might want to call the function but actually provide a set rather than allowing it to be none. I'd imagine most use would actually come via the I would like to add more to the extension in the future so that the maple tree can be queried as it can in the linux kernel. e.g. given an address find the matching slot by walking the tree and comparing the pivots. I think that would be useful functionality for none mm struct uses of maple trees (not that I've come across that yet myself...). Let me know what you think about the changes. Thanks again for all your help! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, and I'm happy with long comments that clarify issues that took us time to figure out in the past. 5;)
Hello,
While debugging the maple tree parsing to see what has caused this issue #1032 for @4n6-fl I realized how I could recreate the issue and fix it.
For some reason that I don't understand (and I would like to understand so, if this makes sense to you please would you team me) if the
get_slot_iter
function is called twice it will fail, theseen
set still contains values when the_parse_maple_tree_node
function starts. It's supposed to be an optional parameter with the default set to an empty set - so I'm not really sure what is going on. It's likely some python I don't understand.To recreate the issue it is as simple as opening volshell and trying to to parse the maple tree twice.
It is the double parsing that causes the bash plugin to have this issue. It does so here https://github.com/volatilityfoundation/volatility3/blob/develop/volatility3/framework/plugins/linux/bash.py#L82 and https://github.com/volatilityfoundation/volatility3/blob/develop/volatility3/framework/plugins/linux/bash.py#L92.
This PR changes the call to
_parse_maple_tree_node
to actually specify the empty set - which seems to do the trick as seen here:Now I rolled back to 804d68d from #996 to see if this has always been like that, and yes. Calling it twice on that commit still caused it to error. I would have tested it at the time, and alishir on slack also said this patch fixed there issue so I'm not exactly sure what's happened. I can only imagine I made some mistake with my testing, or updating python has changed something?
Here is the output showing this fixes the issue with the bash plugin: