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

crash in privload_call_lib_func i386 , when use client with drrun. #4200

Closed
hac425xxx opened this issue Mar 20, 2020 · 12 comments · Fixed by #4265
Closed

crash in privload_call_lib_func i386 , when use client with drrun. #4200

hac425xxx opened this issue Mar 20, 2020 · 12 comments · Fixed by #4265
Assignees

Comments

@hac425xxx
Copy link

platform

i386 ubuntu 18

version

https://github.com/DynamoRIO/dynamorio/releases/download/cronbuild-7.91.18319/DynamoRIO-Linux-7.91.18319.tar.gz

master also has this bug.

when run drrun with client , it could crash.

drrun -c samples/bin32/libinscount.so  -- ./filefuzz
<Application /xxx/filefuzz (101859).  DynamoRIO internal crash at PC 0x00000035.  Please report this at http://dynamorio.org/issues/.  Program aborted.
Received SIGSEGV at unknown pc 0x00000035 in thread 101859
Base: 0xf7d27000
Registers:eax=0x00000000 ebx=0xff8c8e20 ecx=0x00000035 edx=0xf7a82a40
	esi=0xf7f260dc edi=0xff8c8e14 esp=0xff8c8dfc ebp=0xff8c8e1a
	eflags=0x00010282
version 7.91.18319, custom build
-no_dynamic_options -client_lib '/xxxx/samples/bin32/libinscount.so;0;' -code_api -stack_size 56K -signal_stack_size 32K -max_elide_jmp 0 -max_elide_call 0 -early_inject -emulate_brk -no_inline_ignored_syscalls -native_exec_default_list '' -no_native_exec_managed_code -no_indcall2d

this is the backtrace of this bug

(gdb) bt
#0  0x00000035 in ?? ()
#1  0xf7f07e59 in privload_call_lib_func (func=0x35) at /home/travis/build/DynamoRIO/dynamorio/core/unix/loader.c:990
#2  privload_call_entry (dcontext=0xf7fb6000, privmod=0xffffcd54, reason=4159886336) at /home/travis/build/DynamoRIO/dynamorio/core/unix/loader.c:619
#3  0x00000000 in ?? ()
(gdb) quit

@hac425xxx
Copy link
Author

hac425xxx commented Mar 20, 2020

It seems, this commit import the bug

eab941b

@johnfxgalea
Copy link
Contributor

johnfxgalea commented Mar 20, 2020

I confirm this is happening on my machine too. It seems that this is not just a particular issue of libinscount.so but also other samples.

What I noted is that if I I run it under debug mode it works right?

#0  privload_call_entry (dcontext=0x461b3040, privmod=0x461b66c0, reason=1) at /home/travis/build/DynamoRIO/dynamorio/core/unix/loader.c:616
#1  0xb7e86c1e in privload_call_entry_if_not_yet (reason=1, privmod=0x461b66c0, dcontext=0x461b3040) at /home/travis/build/DynamoRIO/dynamorio/core/loader_shared.c:123
#2  privload_load_finalize (privmod=0x461b66c0, dcontext=0x461b3040) at /home/travis/build/DynamoRIO/dynamorio/core/loader_shared.c:845
#3  loader_init_epilogue (dcontext=0x461b3040) at /home/travis/build/DynamoRIO/dynamorio/core/loader_shared.c:225
#4  0xb7e1fa28 in dynamorio_app_init () at /home/travis/build/DynamoRIO/dynamorio/core/dynamo.c:638
#5  0xb7f0a51b in privload_early_inject (sp=0xbfffe3d0, old_libdr_base=0x0, old_libdr_size=0) at /home/travis/build/DynamoRIO/dynamorio/core/unix/loader.c:2034
#6  0xb7eeea91 in reloaded_xfer ()

@johnfxgalea
Copy link
Contributor

Also, what gcc version are you using please?

@hac425xxx
Copy link
Author

gcc version 7.4.0 (Ubuntu 7.4.0-1ubuntu1~18.04.1)

@hac425xxx
Copy link
Author

I comment the assembly code in privload_call_lib_func, and it works


static void
privload_call_lib_func(fp_t func)
{
    char dummy_str[] = "dummy";
    char *dummy_argv[2];
    /* FIXME: i#475
     * The regular loader always passes argc, argv and env to libaries,
     * (see libc code elf/dl-init.c), which might be ignored by those
     * routines.
     * we create dummy argc and argv, and passed with the real __environ.
     */
    dummy_argv[0] = dummy_str;
    dummy_argv[1] = NULL;
    func(1, dummy_argv, our_environ);
}

@johnfxgalea
Copy link
Contributor

@derekbruening any idea what is wrong since it seems that it concerns your commit? If you do not got time, I'll try and have a look at a fix.

@derekbruening
Copy link
Contributor

derekbruening commented Mar 21, 2020

No, the asm looks fine. Maybe we should go ahead and abandon the 4-byte stack alignment. Maybe a good time for it if we put out a new DR release afterward (better now than after).

@derekbruening
Copy link
Contributor

@johnfxgalea if you have time to switch to 16-byte stack alignment everywhere, and I get #3995 finished, we may be all set then for the 8.0 release (except we have to figure out #4126 which is blocking all package builds including an 8.0 one...)

@johnfxgalea
Copy link
Contributor

everywhere

Sure. What do you mean everywhere? Are there more cases than just the one mentioned by OP?

@derekbruening
Copy link
Contributor

See #3966 (comment)

@derekbruening
Copy link
Contributor

Interestingly, on my machine I do not see any crashes with 32-bit libinscount.so. If I remove eab941b then I see the crash at exit #3966 that it fixes.

@derekbruening
Copy link
Contributor

I take it back: as @johnfxgalea said, there is no crash in debug build. In release I can reproduce the up-front crash. But without eab941b it still crashes, at exit.

@johnfxgalea unless you already have this in a branch I'll take it. I just did an experiment and doing the proposed switch to 16-byte stack alignment does fix the release build crash on my machine.

@derekbruening derekbruening self-assigned this Apr 6, 2020
derekbruening added a commit that referenced this issue Apr 20, 2020
Abandons the 4-byte stack alignment DR was using for 32-bit x86 Linux
(for compatibility with legacy code).  The SysV ABI breakage by gcc
4.5 was 10 years ago and in that time other compilers have followed
suit, making 16-byte the de facto standard.  Clang doesn't even
support requesting 4-byte stack alignment.  DR is now switching to the
modern ABI of 16-byte stack alignment.

Bumps the version to 7.92 and marks OLDEST_COMPATIBLE_VERSION as 791
to indicate the binary compatbility break for 32-bit x86 DR.

Leaves Windows as 4-byte-aligned, since changing the injection
assembly code and other pieces would be a bunch of extra work for
little benefit: Windows 32-bit only requires 4-byte alignment, and the
code is not that much more complex with a split alignment.

Aligns the stack after clean call preparation (state saving) and again
after clean call argument setup.  Currently the stack restores can end
up with two LEA's in a row; we leave removing that for future work.

Adds stack alignment checks to some of the clean call tests.

Updates the manual stack alignment in DR assembly code.  I only found
a few places to update and I expected to find more.  We should monitor
usage outside the test suite and look for problems.

Issue: #847, #3966, #4200
Fixes: #3966
Fixes: #4200
derekbruening added a commit that referenced this issue Apr 21, 2020
Abandons the 4-byte stack alignment DR was using for 32-bit x86 Linux
(for compatibility with legacy code).  The SysV ABI breakage by gcc
4.5 was 10 years ago and in that time other compilers have followed
suit, making 16-byte the de facto standard.  Clang doesn't even
support requesting 4-byte stack alignment.  DR is now switching to the
modern ABI of 16-byte stack alignment.

Bumps the version to 7.92 and marks OLDEST_COMPATIBLE_VERSION as 791
to indicate the binary compatbility break for 32-bit x86 DR.

The 32-bit Windows ABI only requires 4-byte alignment, so we leave
DR as only ensuring 4-byte for Windows.  There are potential issues
with gcc or clang-compiled binaries on Windows but such binaries must
already handle Windows system libraries.  #4267 covers changing DR's
alignment for such binaries.  Updating the Windows code would require
work in the various injection generated code sequences and other places.

Aligns the stack after clean call preparation (state saving) and again
after clean call argument setup.  Currently the stack restores can end
up with two LEA's in a row; we leave removing that for future work.

Adds stack alignment checks to some of the clean call tests.

Updates the manual stack alignment done in DR assembly code.  I only found
a few places to update and I expected to find more.  We should monitor
usage outside the test suite and look for problems.

Issue: #847, #3966, #4200, #4267
Fixes: #3966
Fixes: #4200
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants