x86-64 redzone

[davidyeager]

The version of hotpatcher that I looked at appears to overwrite the red zone on x86-64. Not sure if this was ever fixed. ie it will overwrite the 128 byte stack area beyond the stack when malloc and dlopen are called which some programs may use according to the AMD-64 abi: http://en.wikipedia.org/wiki/Red_zone_(computing) I don't believe this is an issue on 32-bit systems.

I tried bumping the stack by more that 128 bytes at the start of hotpatch_inject_library() and it seems to remove the intermittent crashes I was getting on 64-bit linux (32-bit already worked perfectly).

Thanks, David

[vikasnkumar]

Hi David

What flavor of Linux are you using ? I did not have this problem on Debian 6 or Ubuntu 12 or CentOS 6.2.

Can you send me a test case that reproduces this problem for you ?

Thanks Vikas

On 12/02/2013 03:06 PM, davidyeager wrote:

The version of hotpatcher that I looked at appears to overwrite the red zone on x86-64. Not sure if this was ever fixed. ie it will overwrite the 128 byte stack area beyond the stack when malloc and dlopen are called which some programs may use according to the AMD-64 abi: http://en.wikipedia.org/wiki/Red_zone_(computing) http://en.wikipedia.org/wiki/Red_zone_%28computing%29 I don't believe this is an issue on 32-bit systems.

I tried bumping the stack by more that 128 bytes at the start of hotpatch_inject_library() and it seems to remove the intermittent crashes I was getting on 64-bit linux (32-bit already worked perfectly).

Thanks, David

— Reply to this email directly or view it on GitHub https://github.com/vikasnkumar/hotpatch/issues/6.

[davidyeager]

The SPEC CPU 2006 version of bzip2 is the failing test case on Ubuntu 12.04 lts. I don't believe I can share the code/program because of the license. You can try out the free version of bzip2, or just write a simple assembly program that uses the red zone to store temporaries.

But I wouldn't worry too much about reproducing it before you fix it. It's clear that according to the x86-64 ABI, you must preserve the first 128 bytes beyond the stack, except for kernel code: http://www.x86-64.org/documentation/abi.pdf http://eli.thegreenplace.net/2011/09/06/stack-frame-layout-on-x86-64/

David

[vikasnkumar]

Alright, you can supply a patch to the code then if you like this resolved as early as possible and I will merge it after testing it tonight. You can use github pull requests to do this so that it shows up on your github account too.

Thanks Vikas

On 12/02/2013 03:37 PM, davidyeager wrote:

The SPEC CPU 2006 version of bzip2 is the failing test case on Ubuntu 12.04 lts. I don't believe I can share the code/program because of the license. You can try out the free version of bzip2, or just write a simple assembly program that uses the red zone to store temporaries.

But I wouldn't worry too much about reproducing it before you fix it. It's clear that according to the x86-64 ABI, you must preserve the first 128 bytes beyond the stack, except for kernel code: http://www.x86-64.org/documentation/abi.pdf http://eli.thegreenplace.net/2011/09/06/stack-frame-layout-on-x86-64/

David

— Reply to this email directly or view it on GitHub https://github.com/vikasnkumar/hotpatch/issues/6#issuecomment-29655045.

[davidyeager]

You're gonna laugh but I've never downloaded or contributed code with github before (got your this source from a different website). I'll need to take some time to get setup and figure out how to do this and unfortunately I have an urgent work deadline so not gonna be able to do this until Thursday. Feel free to do it yourself or if you prefer then I will do it on Thursday.

Thanks

[vikasnkumar]

That's ok. I hope you're then using the latest source code from github for your work.

You can just paste the changes you made here in an email and I can fix it whenever I get those changes from you.

Thanks

On 12/02/2013 03:59 PM, davidyeager wrote:

You're gonna laugh but I've never downloaded or contributed code with github before (got your this source from a different website). I'll need to take some time to get setup and figure out how to do this and unfortunately I have an urgent work deadline so not gonna be able to do this until Thursday. Feel free to do it yourself or if you prefer then I will do it on Thursday.

Thanks

— Reply to this email directly or view it on GitHub https://github.com/vikasnkumar/hotpatch/issues/6#issuecomment-29656841.

[davidyeager]

Changes to hotpatch_inject_library():

declare uintptr_t newSp;

...

memcpy(&iregs, &oregs, sizeof(oregs)); New code added--> HP_REG_SP(iregs) -= 128; newSp = HP_REG_SP(iregs); <--New code added

In this code replace HP_REG_SP(oregs) with newSp. ie

original:

for (idx = 0; idx < sizeof(stack)/sizeof(uintptr_t); ++idx) { if ((rc = hp_pokedata(hp->pid, HP_REG_SP(oregs) + idx * sizeof(size_t), stack[idx], verbose)) < 0) break; }

new:

for (idx = 0; idx < sizeof(stack)/sizeof(uintptr_t); ++idx) { if ((rc = hp_pokedata(hp->pid, newSp + idx * sizeof(size_t), stack[idx], verbose)) < 0) break; }

[vikasnkumar]

Hi David

Thanks.

I have updated the code and checked it in. You can pull the latest code and see the changes for yourself. I have credited you in the code at the point of change.

--Vikas

On 12/02/2013 04:35 PM, davidyeager wrote:

Changes to hotpatch_inject_library():

declare uintptr_t newSp;

...

memcpy(&iregs, &oregs, sizeof(oregs)); New code added--> HP_REG_SP(iregs) -= 128; newSp = HP_REG_SP(iregs); <--New code added

In this code replace HP_REG_SP(oregs) with newSp. ie

original:

for (idx = 0; idx < sizeof(stack)/sizeof(uintptr_t); ++idx) { if ((rc = hp_pokedata(hp->pid, HP_REG_SP(oregs) + idx * sizeof(size_t), stack[idx], verbose)) < 0) break; }

new:

for (idx = 0; idx < sizeof(stack)/sizeof(uintptr_t); ++idx) { if ((rc = hp_pokedata(hp->pid, newSp + idx * sizeof(size_t), stack[idx], verbose)) < 0) break; }

— Reply to this email directly or view it on GitHub https://github.com/vikasnkumar/hotpatch/issues/6#issuecomment-29660018.