unicorn-engine / unicorn

Unicorn CPU emulator framework (ARM, AArch64, M68K, Mips, Sparc, PowerPC, RiscV, S390x, TriCore, X86)
http://www.unicorn-engine.org
GNU General Public License v2.0
7.47k stars 1.33k forks source link

2 bugs in memory mapping #420

Closed eqv closed 8 years ago

eqv commented 8 years ago

Testcases added in #419

aquynh commented 8 years ago

confirmed 1 segfault & 1 abortion (Resource busy).

farmdve commented 8 years ago

I wonder if with my memory leak fix commit this still happens.

aquynh commented 8 years ago

No, this is a bug in uc_mem_unmap().

farmdve commented 8 years ago

No I mean if my commit fixes this.

aquynh commented 8 years ago

No i dont think so. This is an unrelated bug.

aquynh commented 8 years ago

the latest commit https://github.com/unicorn-engine/unicorn/commit/5719481e3ffa66e7b9829f25dc693dd9fc0caf04 fixes tests/unit/test_mem_map.c, and all the tests pass now.

tests/regress/mem_fuzz.c does not crash anymore, either.

please confirm, thanks.

eqv commented 8 years ago

test_strange_map still fails for me with:

/src/github.com/unicorn-engine/unicorn/qemu/memory.c:1626: memory_region_del_subregion_x86_64: Assertion `subregion->container == mr' failed.

valgrind says its due to:

==6240== Source and destination overlap in memcpy(0x1b110be0, 0x1b110be8, 40)
==6240==    at 0x4CAF72C: memcpy@@GLIBC_2.14 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6240==    by 0x51AFFE4: memory_unmap_x86_64 (memory.c:88)
==6240==    by 0x574F1A6: uc_mem_unmap (uc.c:928)
==6240==    by 0x41026E: test_strange_map (test_mem_map.c:158)
==6240==    by 0x4E37C0F: cmocka_run_one_test_or_fixture (in /usr/local/lib/libcmocka.so.0.3.1)
==6240==    by 0x4E42EBC: cmocka_run_one_tests (in /usr/local/lib/libcmocka.so.0.3.1)
==6240==    by 0x4E4D2CE: _cmocka_run_group_tests (in /usr/local/lib/libcmocka.so.0.3.1)
==6240==    by 0x400A4D: main (test_mem_map.c:177)
==6240== 

All other tests pass.

eqv commented 8 years ago

I added another two test cases & enabled read/write in the fuzzer again in PR #431. I hope the two testcases can help to spot the remaining problem.

eqv commented 8 years ago

I just had a look into the code, it seems that this memcpy can simply be replaced by memmove, I will try this and submit a PR if it works.

eqv commented 8 years ago

bug should be fixed in PR #432

aquynh commented 8 years ago

very nice! can you confirm that Valgrind stops complaining after this fix?

eqv commented 8 years ago

Confirmed

On 11.02.2016 16:47, Nguyen Anh Quynh wrote:

very nice! can you confirm that Valgrind stops complaining after this fix?

— Reply to this email directly or view it on GitHub https://github.com/unicorn-engine/unicorn/issues/420#issuecomment-182924412.

aquynh commented 8 years ago

any ideas on why we have overlapping regions in memcpy here? i suppose these memory areas for these blocks cannot overlap, so i am confused (?)

eqv commented 8 years ago

memcpy(&uc->mapped_blocks[i], &uc->mapped_blocks[i + 1], sizeof(MemoryRegion) \ (uc->mapped_block_count - i));

because we copy all remaining lbocks one index to the left, assuming there are at least 2 blocks left, this means that we copy blocks i+1,i+2 to the offsets i,i+1. i+1 is thus overlapped by both the read and the write.

On 11.02.2016 16:55, Nguyen Anh Quynh wrote:

any ideas on why we have overlapping regions in memcpy here? i suppose these memory areas for these blocks cannot overlap, so i am confused (?)

— Reply to this email directly or view it on GitHub https://github.com/unicorn-engine/unicorn/issues/420#issuecomment-182930527.

eqv commented 8 years ago

Also, the fuzzer now fails to trigger any crashes in the first few million iterations.

On 11.02.2016 16:55, Nguyen Anh Quynh wrote:

any ideas on why we have overlapping regions in memcpy here? i suppose these memory areas for these blocks cannot overlap, so i am confused (?)

— Reply to this email directly or view it on GitHub https://github.com/unicorn-engine/unicorn/issues/420#issuecomment-182930527.

aquynh commented 8 years ago

oh right, i did not look close enough on the arguments of memcpy()

aquynh commented 8 years ago

i merged the PR https://github.com/unicorn-engine/unicorn/pull/432. please report if you still have issues.

thanks.

eqv commented 8 years ago

fixed, thanks