Off by 1 from using the get_end and get_start functions

[paulkermann]

trafficstars

https://github.com/volatilityfoundation/volatility3/blob/7d17c191e5cfacc84e0d355b18dcf6c7ce9f68d0/volatility3/framework/plugins/windows/malfind.py#L59

This should actually be vad_length = vad.get_end() - vad.get_start() + 1

get_end is the last valid address including so there is an off by 1. Reading vad_length would yield 1 less byte. I would add a get_size function that would return vad.get_end() - vad.get_start() + 1 to avoid problems like this in the future. This problem is also present here https://github.com/volatilityfoundation/volatility3/blob/0c603a20ba877795b3da49f6a7ae0f5721d23e7d/volatility3/framework/plugins/windows/vadinfo.py#L154 as another off by 1 as vad_end is readable.

It also looks like this wrong logic happens in other places. One could also maybe just not subtract 1 from get_end but that could probably cause compatibility issues so the first solution is probably better. However it looks like everybody is using get_end as it is not subtracted by 1 so I don't really know.

[iMHLv2]

@paulkermann Thanks for the bug report. I pushed a branch with a proposed fix. Historically, even back to vol2, the get_end() function has returned the last accessible byte in the range, so I'm keeping that unchanged. I added a get_size() method that calculates the total size in bytes and fixed a number of plugins that had off-by-one issues. Anyone relying on get_end() to calculate size in custom plugins (outside of the core code base) will still have one-by-one issues, but they can fix it by using the new get_size() - IMO that is better than changing the behavior of get_end() and potentially breaking things "silently."

[digitalisx]

Hello @iMHLv2, Thank you for your good work! It seems that it is not pushed to the development branch and exists only in issue branch. Could you tell me the intend or reason why not convert to PR? 🙂 (Additional this changes may require a bump version.)

[iMHLv2]

Hello @iMHLv2, Thank you for your good work! It seems that it is not pushed to the development branch and exists only in issue branch. Could you tell me the intend or reason why not convert to PR? 🙂 (Additional this changes may require a bump version.)

PR is in. I just created a branch initially so people could see the changes and experiment with it.

[ikelos]

Looks like this should have been closed when #818 landed. Feel free to reopen if the issue hasn't been resolved.