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

Merged
merged 2 commits into from
Jan 25, 2025
+11 −2
Merged

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9088be
Prevent infinite looping and out of memory errors #1482
atcuno Dec 28, 2024
506a61d
Address feedback
atcuno Jan 24, 2025
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
Next Next commit
Loading
Loading status checks…
Prevent infinite looping and out of memory errors #1482
@atcuno
atcuno committed Jan 24, 2025
commit e9088be0d86fa2f68774d47371ebd2736287f1c5
9 changes: 9 additions & 0 deletions volatility3/framework/symbols/windows/extensions/registry.py
View file
Original file line number Diff line number Diff line change
@@ -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