Add count option in heap chunks command to limit the number of chunks to process / output. #1029
Conversation
|
|
||
| if not summary and chunk.base_address == arena.top: | ||
| if not ctx.summary and chunk.base_address == arena.top: |
There was a problem hiding this comment.
The default value of None would cause this to throw an Exception.
There was a problem hiding this comment.
updated the code to fix this problem.
There was a problem hiding this comment.
OK looks good, but technically you're passing a mutable object (the context) as the default value, which is bound to the function object. If it changes (and you take care not to) then subsequent calls would have this modified default. https://stackoverflow.com/questions/1132941/least-astonishment-and-the-mutable-default-argument
Since you don't modify it, maybe it's fine, but we run the risk of someone modifying it in the future. Probably OK though.
There was a problem hiding this comment.
Yea, it will never be null. The default value is added there just to follow existing code. We can also remove the default value, but I will have to move the parameter before the ones with default value. not sure if it is ok.
There was a problem hiding this comment.
i removed the default value and lifted the ctx parameter to the front.
|
|
||
| if not summary and chunk.base_address == arena.top: | ||
| if not ctx.summary and chunk.base_address == arena.top: |
There was a problem hiding this comment.
OK looks good, but technically you're passing a mutable object (the context) as the default value, which is bound to the function object. If it changes (and you take care not to) then subsequent calls would have this modified default. https://stackoverflow.com/questions/1132941/least-astonishment-and-the-mutable-default-argument
Since you don't modify it, maybe it's fine, but we run the risk of someone modifying it in the future. Probably OK though.
|
LGTM after Graz's comments |
gef.py
Outdated
| @@ -6374,18 +6374,18 @@ def do_invoke(self, _: List[str], **kwargs: Any) -> None: | |||
| ctx = GlibcHeapWalkContext(min_size=args.min_size, max_size=args.max_size, count=args.count, summary=args.summary) | |||
| if args.all or not args.arena_address: | |||
| for arena in gef.heap.arenas: | |||
| self.dump_chunks_arena(arena, print_arena=args.all, allow_unaligned=args.allow_unaligned, ctx=ctx) | |||
| self.dump_chunks_arena(arena, ctx, print_arena=args.all, allow_unaligned=args.allow_unaligned) | |||
There was a problem hiding this comment.
If we use the context idiom, I wonder whether we should put it as the first arg (which is what we do in Go). Do you know if it's also done this way in python?
There was a problem hiding this comment.
yea, i was hesitated on this one too - which name is better: context or options? eventually I chose context, because the remaining chunk count changes over time... options usually won't change.
for the order of parameters, i don't really have strong opinion on that. what i usually do is to put parameters that absolutely required but need to be part of the workflow in the front (in this case - arena), then followed by the context (which contains mostly everything).
If you like the code to be modified, i can definitely do that by moving print_arena and allow_unaligned in - so it will be dump_chunks_arena(arena, ctx), which is what i usually do.
There was a problem hiding this comment.
i have updated the code now. let's see how it looks.
|
Last merge also caused a conflict, you'll need to rebase. |
r12f
force-pushed
the
user/r12f/limit
branch
from
6825389 to
afabab9
Compare
r12f
force-pushed
the
user/r12f/limit
branch
from
afabab9 to
3be1c00
Compare
yep! it is conflicting with the symbol change. i have rebased the branch now. |
There was a problem hiding this comment.
I'm ok with the current state of the PR. @Grazfather any last comment?
|
Thanks for your PR and patience @r12f , it's all good now 🎉 |
|
Woot! thanks a lot for the review @Grazfather and @hugsy ! And happy new year! 🎉 |
|
Happy new year to you too 😁 🎉 |
Description
This change add
--countoption in heap chunks command to limit the number of chunks to process / output.Currently, since we don't have the count limit, even with the size filter, we might get too many chunks in the output, especially when debugging large dumps.
With this change, we can use
--countoption to give us only a few samples to check what might be happening, e.g. large buffers with certain special size.Here is the screenshot that demos the usage (mixed with
--min-size)L