Bug 15592 - mtrace.c tr_break() is not called from malloc hooks
Summary: mtrace.c tr_break() is not called from malloc hooks
Status: RESOLVED WONTFIX
Alias: None
Product: glibc
Classification: Unclassified
Component: malloc (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-06 16:52 UTC by Felipe Collyer
Modified: 2021-07-14 02:50 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments
patch (255 bytes, patch)
2013-10-14 16:08 UTC, Ondrej Bilka
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Felipe Collyer 2013-06-06 16:52:55 UTC
Looking at some dumps generated on a CentOS 5.6, it seems as if gcc is optimizing away some calls to tr_break().

Everything is in place (according to the file instructions), but tr_break() is not firing after setting its breakpoint.

Small gdb disasm from glibc-2.5x:

Dump of assembler code for function tr_freehook:
0x0066ffc0 <tr_freehook+0>:     push   %ebp
0x0066ffc1 <tr_freehook+1>:     mov    %esp,%ebp
0x0066ffc3 <tr_freehook+3>:     sub    $0x18,%esp
0x0066ffc6 <tr_freehook+6>:     mov    0x8(%ebp),%eax
0x0066ffc9 <tr_freehook+9>:     mov    %ebx,-0xc(%ebp)
0x0066ffcc <tr_freehook+12>:    call   0x616ce0 <__i686.get_pc_thunk.bx>
0x0066ffd1 <tr_freehook+17>:    add    $0xd3023,%ebx
0x0066ffd7 <tr_freehook+23>:    mov    %esi,-0x8(%ebp)
0x0066ffda <tr_freehook+26>:    test   %eax,%eax
0x0066ffdc <tr_freehook+28>:    mov    %edi,-0x4(%ebp)
0x0066ffdf <tr_freehook+31>:    je     0x6700a4 <tr_freehook+228>
0x0066ffe5 <tr_freehook+37>:    xor    %edi,%edi
0x0066ffe7 <tr_freehook+39>:    mov    $0x1,%esi
0x0066ffec <tr_freehook+44>:    mov    %edi,%eax
0x0066ffee <tr_freehook+46>:    mov    %esi,%ecx
0x0066fff0 <tr_freehook+48>:    cmpl   $0x0,%gs:0xc
0x0066fff8 <tr_freehook+56>:    je     0x66fffb <tr_freehook+59>
0x0066fffa <tr_freehook+58>:    lock cmpxchg %ecx,0x1648(%ebx)
0x00670002 <tr_freehook+66>:    jne    0x67040c <_L_lock_464>
0x00670008 <tr_freehook+72>:    mov    0xc(%ebp),%eax
0x0067000b <tr_freehook+75>:    call   0x66fe70 <tr_where>
0x00670010 <tr_freehook+80>:    mov    0x8(%ebp),%eax
0x00670013 <tr_freehook+83>:    mov    %eax,0x8(%esp)
0x00670017 <tr_freehook+87>:    lea    -0x1b2a9(%ebx),%eax
0x0067001d <tr_freehook+93>:    mov    %eax,0x4(%esp)
0x00670021 <tr_freehook+97>:    mov    0x1640(%ebx),%eax
0x00670027 <tr_freehook+103>:   mov    %eax,(%esp)
0x0067002a <tr_freehook+106>:   call   0x646e20 <fprintf>
0x0067002f <tr_freehook+111>:   cmpl   $0x0,%gs:0xc
0x00670037 <tr_freehook+119>:   je     0x67003a <tr_freehook+122>
0x00670039 <tr_freehook+121>:   lock subl $0x1,0x1648(%ebx)
0x00670041 <tr_freehook+129>:   jne    0x67041c <_L_unlock_481>
---Type <return> to continue, or q <return> to quit---
0x00670047 <tr_freehook+135>:   mov    %edi,%eax
0x00670049 <tr_freehook+137>:   mov    %esi,%ecx
0x0067004b <tr_freehook+139>:   cmpl   $0x0,%gs:0xc
0x00670053 <tr_freehook+147>:   je     0x670056 <tr_freehook+150>
0x00670055 <tr_freehook+149>:   lock cmpxchg %ecx,0x1648(%ebx)
0x0067005d <tr_freehook+157>:   jne    0x67042c <_L_lock_490>
0x00670063 <tr_freehook+163>:   mov    0x164c(%ebx),%eax
0x00670069 <tr_freehook+169>:   mov    -0x38(%ebx),%esi
0x0067006f <tr_freehook+175>:   test   %eax,%eax
0x00670071 <tr_freehook+177>:   mov    %eax,(%esi)
0x00670073 <tr_freehook+179>:   je     0x6700b1 <tr_freehook+241>
0x00670075 <tr_freehook+181>:   mov    0xc(%ebp),%edx
0x00670078 <tr_freehook+184>:   mov    %edx,0x4(%esp)
0x0067007c <tr_freehook+188>:   mov    0x8(%ebp),%edx
0x0067007f <tr_freehook+191>:   mov    %edx,(%esp)
0x00670082 <tr_freehook+194>:   call   *%eax
0x00670084 <tr_freehook+196>:   lea    -0xd3034(%ebx),%eax
0x0067008a <tr_freehook+202>:   mov    %eax,(%esi)
0x0067008c <tr_freehook+204>:   cmpl   $0x0,%gs:0xc
0x00670094 <tr_freehook+212>:   je     0x670097 <tr_freehook+215>
0x00670096 <tr_freehook+214>:   lock subl $0x1,0x1648(%ebx)
0x0067009e <tr_freehook+222>:   jne    0x67043c <_L_unlock_517>
0x006700a4 <tr_freehook+228>:   mov    -0xc(%ebp),%ebx
0x006700a7 <tr_freehook+231>:   mov    -0x8(%ebp),%esi
0x006700aa <tr_freehook+234>:   mov    -0x4(%ebp),%edi
0x006700ad <tr_freehook+237>:   mov    %ebp,%esp
0x006700af <tr_freehook+239>:   pop    %ebp
0x006700b0 <tr_freehook+240>:   ret
0x006700b1 <tr_freehook+241>:   mov    0x8(%ebp),%eax
0x006700b4 <tr_freehook+244>:   mov    %eax,(%esp)
0x006700b7 <tr_freehook+247>:   call   0x66a990 <free>
0x006700bc <tr_freehook+252>:   jmp    0x670084 <tr_freehook+196>
End of assembler dump.

This can be mapped to the corresponding source-code:

http://sourceware.org/git/?p=glibc.git;a=blob;f=malloc/mtrace.c;h=1a9522b09de37f96fb9e4ed807f3cc1dedaca3fb;hb=88cc61e84e8e75e6e91b1a2e51147aeb63712ff8

146   tr_where (caller);
147   /* Be sure to print it first.  */
148   fprintf (mallstream, "- %p\n", ptr);
149   __libc_lock_unlock (lock);
150   if (ptr == mallwatch)
151     tr_break ();
152   __libc_lock_lock (lock);

Inlined gcc tls lock code around tr_break() seems to be ok.
tr_break() related code seems to be missing.
Comment 1 Ondrej Bilka 2013-10-14 16:08:45 UTC
Created attachment 7233 [details]
patch

Does marking tr_break noinline help? 
What about second trick of adding side effect as a __asm__ in attached patch does?
Comment 2 Felipe Collyer 2014-01-04 23:20:46 UTC
(In reply to Ondrej Bilka from comment #1)
> Created attachment 7233 [details]
> patch
> 
> Does marking tr_break noinline help? 
> What about second trick of adding side effect as a __asm__ in attached patch
> does?

The 2 techniques employed together may work. I don't have the resources at hand to check (let me know if any final dso analysis helps).

As further notice, _dl_debug_state() follows similar tr_break() usage pattern. But its address is taken (r_debug struct), defeating optimizations (some, I believe).
Comment 3 Siddhesh Poyarekar 2021-07-14 02:50:42 UTC
tr_break has been removed in 2.34.

commit 00d28960c5388a582a0485e07629b553c32dde49
Author: Siddhesh Poyarekar <siddhesh@sourceware.org>
Date:   Sat Jul 3 00:47:34 2021 +0530

    mtrace: Deprecate mallwatch and tr_break
    
    The variable and function pair appear to provide a way for users to
    set conditional breakpoints in mtrace when a specific address is
    returned by the allocator.  This can be achieved by using conditional
    breakpoints in gdb so it is redundant.  There is no documentation of
    this interface in the manual either, so it appears to have been a hack
    that got added to debug malloc.  Deprecate these symbols and do not
    call tr_break anymore.
    
    Reviewed-by: DJ Delorie <dj@redhat.com>
    Reviewed-by: Carlos O'Donell <carlos@redhat.com>