Add missing exception handling in paths related to cached files and i… #1516
Conversation
|
@gcmoreira check the spots here where I asked you a question as I wasn't 100% sure in those spots the best action/return value. |
This also directly relates to my comment here: Where |
hm it should work. Could you point me to a sample where using |
Sure, check this backtrace, which the fix is going to be another PR tomorrow (using the fixed This is running and the backtrace triggers on the first line above's access to The pattern of accessing a pointer member and then trying to check its value is the Volatility 2 way of doing things. In Volatility 3, every single access like this needs to be in a try/except. David and I have been making an effort to localize the checks (per access or similar, with debug prints where it makes sense), but there is no way to avoid the try/except (or contextlib.suppress when its better) in Volatility 3. I want to make sure you understand all this as I will need alot of help over the next week or two cleaning up all the |
|
Ok, I debugged what happened with the
In this case, the problem is that the dentry is invalid, not the inode. So, it's clear that the line pointed above won't catch this type of issues. if not (inode_ptr and inode_ptr.is_readable()):To correctly fix this, instead of making a try/except block every time a dentry member is referenced like this PR suggests, a better approach is to make sure the generated dentries are correct. In this case, the problem is not where we were about to use the try/except but in For instance, with the following fix, --- a/volatility3/framework/symbols/linux/extensions/__init__.py
+++ b/volatility3/framework/symbols/linux/extensions/__init__.py
@@ -1133,7 +1133,11 @@ class dentry(objects.StructType):
raise exceptions.VolatilityException("Unsupported dentry type")
dentry_type_name = self.get_symbol_table_name() + constants.BANG + "dentry"
- yield from list_head_member.to_list(dentry_type_name, walk_member)
+
+ layer = self._context.layers[self.vol.layer_name]
+ for dentry in list_head_member.to_list(dentry_type_name, walk_member):
+ if layer.is_valid(dentry.vol.offset):
+ yield dentry
def get_inode(self) -> interfaces.objects.ObjectInterface:
"""Returns the inode associated with this dentry"""We could also use the following line to check the whole dentry object, but it seems unnecessary and might introduce some overhead. The patch above should handle the preliminary check, and the dentry user can then validate the individual members/pointers where needed. if layer.is_valid(dentry.vol.offset, dentry.vol.size):Alternatively, we can do a similar check even deeper, in the to_list() methods. Having said that, I do understand that the try/except workarounds might be acceptable if, for some reason, we're pressed for time, but eventually, we need to revisit and address the underlying issues causing each of these problems. Also, I'm happy to help with debugging and identifying the root causes of the remaining issues if needed. Just point me to the sample, its ISF and how to reproduce the bug. |
|
It is fine to add the Adding the Adding the check in the layer though doesn't fix the overall problem though in that:
In summary, we can make the APIs return less smeared instances but it won't remove the need for plugins to check that members are in memory before using them. We also need to move frequently accessed pointer members to accessor methods like I will be out of town this week with only a few samples with me, so today I will make a bunch of tickets here and you can check the best places to do this while I am gone. I can then re-run the tests when back. |
There was a problem hiding this comment.
I'll let Gus answer the questions you've posed, but from my perspective it seems to wrap each individual line in a try/except which isn't necessary. Chunk them together by the biggest block that might fail (in the same way) and then do a single except around that. So as big as you can get, and you can do a try/except inside another try except if you need something specific). So in general, the whole body inside a loop, because there is a small execution time cost of setting up the exception handler and the end result will be identical. Exceptions aren't free, only close to free... 5:)
| reversed_path.append(current_dentry.d_name.name_as_str()) | ||
| try: | ||
| parent = current_dentry.d_parent | ||
| except exceptions.InvalidAddressException: |
There was a problem hiding this comment.
These statements end in the same result, they should probably be in a single try/except block? The traceback would show which line went on, so this just seems excessive?
| @@ -1102,10 +1116,24 @@ def d_ancestor(self, ancestor_dentry): | |||
| not current_dentry.is_root() | |||
| and current_dentry.vol.offset not in dentry_seen | |||
| ): | |||
| if current_dentry.d_parent == ancestor_dentry.vol.offset: | |||
| try: | |||
There was a problem hiding this comment.
Again, feels like a single try/except could wrap all the statements here because the middle statements don't matter if it's still going to return None on a failure?
The OS will do its best to ensure allocations remain within the same page. Otherwise, it will cause TLB or cache misses, leading to page faults and slowing down the system. While it is possible for allocations to span multiple pages, fixing this issue in the proposed way doesn't scale and IMO is not good programming practice. If we follow your logic, we would need to wrap every single member access in the framework with try/except, assuming it might be on the next page, which to me doesn't make sense. Again, everything points to the issue being deeper in the framework. It's better to resolve it at the root once and for all, rather than applying workarounds everywhere. I think we should try my suggested fix in |
|
As I said, I am fine with fixing things in the deeper layers to bubble up less junk but we really do need try/except over accesses that can span a page and/or when trying to follow pointers. You can rework this PR or just start from scratch as long as the failing cases are fixed. I am not attached to this particular PR at all, and I made the other tickets separate and assigned to you since you wrote so much of the cached file / inode code originally. |
|
Soooo... am I merging this one (in which case the exceptions need combining/reducing) or @gcmoreira are you gonna make a new PR? I'm fine either way, just don't want to commit something we're going to have to change as soon as it's gone in... |
|
@ikelos just ignore this one until Gus sorts out the issues. He is very focused on this one plus the other cached file ones this week. |
…nodes
The missing exception handling in these paths causes many backtraces across plugins and samples. Add in the missing handlers.