Skip to content
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.

Sign up for GitHub

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

Prevent infinite looping and out of memory errors #1482 #1490

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Prevent infinite looping and out of memory errors #1482 #1490

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions volatility3/framework/symbols/windows/extensions/registry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,17 @@ def _skip_key_hive_entry_path(self, kcb_flags):

def get_full_key_name(self) -> str:
output = []
seen = set()

kcb = self.KeyControlBlock
while kcb.ParentKcb:
if kcb.ParentKcb.vol.offset in seen:
return ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these be None or something else? I don't want the BaseAbsentValues sneaking in here, but it feels like we should be alerting people that things didn't work? The downside with that is then you have to do error checking whenever you try to pull the full key name? Just wondering of the consequences of it returning a value but blank string in case of an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That function is called from handles on this line:

https://github.com/volatilityfoundation/volatility3/blob/develop/volatility3/framework/plugins/windows/handles.py#L313

Which, in the else case of that same block, the empty string is used to indicate an error:

https://github.com/volatilityfoundation/volatility3/blob/develop/volatility3/framework/plugins/windows/handles.py#L318

If it makes more sense, I can change both places (the registry extension + line 318 in handles) to send back None instead of "", then have the yield() handler do the "variable or renderers....." setup. Is that preferred? That avoids the extension API having to send back a renderer instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, thanks for this:

"Also, if you want github to auto tag the issues these are supposed to fix, you can't just mention it in the title, you need to put "Fixes #blah" or "Closes #blah" in the bug body, please..."

I am used to GitLab where tagging the number accomplishes it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it would be better to return None as a way of indicating an error, and then let the caller figure out what to do with it. Also, no problem. 5;)

seen.add(kcb.ParentKcb.vol.offset)

if len(output) > 128:
return ""

if kcb.NameBlock.Name is None:
break

Expand Down
Loading