Fix on 'end_bit' computation #133
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ikelos
approved these changes
Nov 11, 2019
| @@ -369,7 +369,7 @@ def _interdict_to_template(self, dictionary: Dict[str, Any]) -> interfaces.objec | |||
| elif type_name == 'enum': | |||
| update = self._lookup_enum(dictionary['name']) | |||
| elif type_name == 'bitfield': | |||
| update = {'start_bit': dictionary['bit_position'], 'end_bit': dictionary['bit_length']} | |||
| update = {'start_bit': dictionary['bit_position'], 'end_bit': dictionary['bit_position'] + dictionary['bit_length']} | |||
There was a problem hiding this comment.
Great spot! 5:)
It looks like the only place we use end_bit, we use it as the actual end rather than the length, so this makes sense.
ikelos
added a commit
that referenced
this pull request
Nov 13, 2019
Turns out that #133 exposed a mistake in how we were using the end_bit field (and it should have been picked up in review, my bad). Essentially we were masking based on the length of the end_bit after *already* shifting by the start_bit. We should mask then shift, not the other way around. May well fix issue #135 (pdb generation will have been affected by this).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi, I was looking at the code for managing symbols and found this line:
https://github.com/volatilityfoundation/volatility3/blob/master/volatility/framework/symbols/intermed.py#L372
I think there might be a semantic inconsistency there: the ‘end_bit’ keyword is populated with the bit_length of the field, instead of the end_bit.
I have looked at the symbol json file, and it seems that bitfields contain ‘start_bit’ and ‘bit_length’. On the contrary, volatility2 vtypes seem to represent bitfields with their ‘start_bit’ and ‘end_bit’. (E.g.: https://github.com/volatilityfoundation/volatility/blob/aa6b960c1077e447bda9d64df507ec02f8fcc958/volatility/plugins/overlays/windows/win2003_sp0_x86_vtypes.py#L1814)
Given that the BitField class on framework/objects/_ _ init _ .py stores start_bit and end_bit, I’d lean towards updating https://github.com/volatilityfoundation/volatility3/blob/master/volatility/framework/symbols/intermed.py#L372
That would be consistent with volatility2 and how the BitField class is defined in volatility3.
Thanks,