-
-
Notifications
You must be signed in to change notification settings - Fork 725
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.
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
[memory] Fix read_cstring trying to read too far #1112
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach is not very optimal. I suggested several options but feel free to bring others as long as they don't impact the perf as much.
Also this needs to be tested.
gef.py
Outdated
@@ -10597,10 +10597,34 @@ def read_cstring(self, | |||
encoding = encoding or "unicode-escape" | |||
length = min(address | (DEFAULT_PAGE_SIZE-1), max_length+1) | |||
|
|||
while not self.get_section(address + length - 1) and length > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't measure it but I think this approach may cause a slow down in read_cstring
for long printable strings. I also find quite complex for what is actually intended to do because you'll be impacting all cases of use of this function for only a minority of cases that violate page boundary.
A simpler approach would be to remove all this, simply let self.read
execute: if a gdb.MemoryError, use the exception information calculate the maximum valid length and re-read.
Another approach would be to check early on when setting length
that address + length is a readable address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be tested to ensure no regression later on.
Also still curious about the perf impact, did you run any perf test?
I didn't tested yet, and I'm thinking of cases where this fix doesn't work so I'll probably rework this PR soon |
Awesome! If this can wait, we can collab on this more actively by end of July/August when things get quieter on my end. Otherwise I'll try to find time here and there to review what you've done. |
e057b6d
to
effaa16
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, comments might be nice to explain what you're doing with the masking
res_bytes = b"" | ||
while len(res_bytes) < length: | ||
try: | ||
new_end_addr = current_address + DEFAULT_PAGE_SIZE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that this variable name is very good, since it's not where we end at all, it's just a page away, and we are about to chop it down with the mask
try: | ||
new_end_addr = current_address + DEFAULT_PAGE_SIZE | ||
page_mask = ~(DEFAULT_PAGE_SIZE - 1) | ||
size = (new_end_addr & page_mask) - current_address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment something like "calculate size by going to the next page-aligned address after current address"
fix for #1055