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.

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

bug in heap command #568 (fix) #571

Merged
merged 5 commits into from Feb 27, 2019

Conversation

Projects
None yet
3 participants
@matrix1001
Copy link
Contributor

matrix1001 commented Nov 12, 2018

Bug fix for heap command.

Note that this is only for ptmalloc. Not tested on dlmalloc.

By the way, why there is still a diff for radare2.py ? ( I'm not a experienced user of git :P )

matrix1001 added some commits Nov 7, 2018

Update radare2.py
API update
@disconnect3d

This comment has been minimized.

Copy link
Member

disconnect3d commented Nov 12, 2018

By the way, why there is still a diff for radare2.py ? ( I'm not a experienced user of git :P )

It seems those changes are there on your branch. It's mainly because you used your own dev branch and didn't update it after the last PR was merged.

There are few ways to fix this... the easiest may be to do git fetch origin (it shall download changes from origin so that origin/dev will correspond to upstream - pwndbg's - dev branch) and then git merge origin/dev shall merge the changes from origin to your own branch.

You might need to solve conflicts but I am not sure about that.

@matrix1001

This comment has been minimized.

Copy link
Contributor Author

matrix1001 commented Nov 12, 2018

Thanks. Now I can see only one file changed in the PR.

@disconnect3d

This comment has been minimized.

Copy link
Member

disconnect3d commented Nov 13, 2018

@GrosQuildu @andigena @gsingh93 any comments on this?

Can we also confirm this behavior from happening in the glibc code? :/

I mean, we must be sure it is valid and understand why is that.. :F

@matrix1001

This comment has been minimized.

Copy link
Contributor Author

matrix1001 commented Nov 14, 2018

Well, I checked/debuged the source code of sysmalloc and verified this.

image

debug:

pwndbg> p aligned_brk
$3 = 0x5655a008
pwndbg> p front_misalign
$4 = 8
pwndbg>
@disconnect3d

This comment has been minimized.

Copy link
Member

disconnect3d commented Nov 21, 2018

Hey, I haven't had time to look through this yet. Please ping me if I won't make a response here till the end of this week.

@GrosQuildu

This comment has been minimized.

Copy link
Contributor

GrosQuildu commented Feb 21, 2019

How sysmalloc seems to work:

  • sysmalloc try mmap
    • if MALLOC_ALIGNMENT == 2 * SIZE_SZ (normal case), then no alignment
    • otherwise there is some alignment, idk how to detect that (we can't check MALLOC_ALIGNMENT)
  • if not mmaped
    • if if (av != &main_arena)
      • expand or create new heap, probably same case as above
    • otherwise try MORECORE (sbrk) (on failure mmap again)
      • if it expanded previous mem, then no alignment
      • otherwise memory may be aligned (in both cases: when arena is and is not contiguous), the code is as @matrix1001 copy-pasted.

As for the code:

#define chunk2mem(p)   ((void*)((char*)(p) + 2*SIZE_SZ))
#define MALLOC_ALIGNMENT 2 * SIZE_SZ
#define MALLOC_ALIGN_MASK (MALLOC_ALIGNMENT-1)

front_misalign = (INTERNAL_SIZE_T) chunk2mem (brk) & MALLOC_ALIGN_MASK;
if (front_misalign > 0) { 
    correction = MALLOC_ALIGNMENT - front_misalign; 
    aligned_brk += correction;
}

So for example on 64bit systems with 32bit programs we have compile/runtime discrepancy and chunk2mem adds 2*4 but MALLOC_ALIGNMENT is 16 and we got 8 byte alignment.

On x64 with 64b programs chunk2mem adds 2*8 and front_misalign is 0.

Same for a few libc versions I checked.

As for the PR: condition for the first chunk to have size != 0 is sane enough to merge for me ;) But may be better to change it to:

if addr is None:
        addr = page.vaddr

        size_t = pwndbg.arch.ptrsize
        first_chunk_size = pwndbg.arch.unpack(pwndbg.memory.read(addr + size_t, size_t))
        if first_chunk_size == 0:
            addr += size_t * 2  # Skip the alignment

If somebody specify addr he probably know what he's doing and changing the addr without info may be misleading.

Another thing: in ptmalloc.py there is HeapInfo.first_chunk field which takes into account that first heap has a heap_info and a malloc_state before the actual chunks. Idk when this happens, but as heap command uses get_heap_boundaries and not first_chunk that case may be broken as well. So maybe it would be better to use something like main_heap.arenas[0].heaps[0].first_chunk in heap cmd?

Nice feature would be to extend heap cmd such that it also finds top chunk and work backwards to the first one (as we should handle corrupted heaps as well).

@disconnect3d disconnect3d merged commit 0f4e31e into pwndbg:dev Feb 27, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@disconnect3d

This comment has been minimized.

Copy link
Member

disconnect3d commented Feb 27, 2019

@GrosQuildu Thx for looking into it.

Re:

But may be better to change it to (...)

And:

Another thing: in ptmalloc.py there is HeapInfo.first_chunk (...)

Can you send a PR with those two things? (And optionally with the heap command extend ;>)

@disconnect3d disconnect3d referenced this pull request Mar 5, 2019

Closed

bug in heap command #568

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.