Differential D9259

fix heaptrack_inject to find second kind of relocation
ClosedPublic
Actions

Authored by middleton on Dec 8 2017, 4:30 PM.

Details

Reviewers

mwolff

Group Reviewers

Heaptrack

Commits

R45:779dceb8cbe7: Override allocation functions accessible through .rel{a,}.dyn

Summary

Currently, only ordinary PLT entries for malloc, free, etc. are found by heaptrack_inject. Some libraries have a different type of entry (in the .plt.got section) which heaptrack misses. (I'm not sure why the compiler does this-- can anyone enlighten me?) This causes heaptrack to miss some allocations and/or frees. (I found this problem when heaptrack was telling me about tons of memory leaks that didn't really exist-- it was only seeing the mallocs but not the corresponding frees.)

This patch adds support for the other type of relocation table (.rela.dyn section) so that hopefully no functions are missed now.

Diff Detail

Repository

R45 Heaptrack

Lint

Lint Skipped

Unit

Unit Tests Skipped

middleton requested review of this revision.Dec 8 2017, 4:30 PM

middleton created this revision.

hey, thanks for the contribution! do you have a code snippet that showed the behavior we could use for testing purposes?

src/track/heaptrack_inject.cpp
250 ↗	(On Diff #23658)	please use a template function for this instead of a macro

This revision now requires changes to proceed.Dec 20 2017, 10:05 PM

I'm not much of a C++ guy, so take it from here, Milian, if this patch still needs work. :)

testbind.c1 KBDownload

I was curious enough to figure out what compiler behavior causes the different type of relocation entries, so here's a test program with extensive comments. I was originally seeing the behavior with libcairo, but this should make testing easy.

Thanks, I'll take over then. What email address should I use to attribute (parts of) the patch to?

I fail to reproduce the issue with the provided test case on ArchLinux. When I compile the three variants, I always properly detect the free calls.

I've now also adapted your test case a bit to run it in an automated fashion:

#include <stdlib.h>
#include <unistd.h>

void escape(void *p)
{
    asm volatile("" : : "g"(p) : "memory");
}

int main()
{
#if defined(TAKE_ADDR) || defined(USE_FREEPTR)
    void (*freePtr)(void*) = &free;
    escape(freePtr);
#endif

    sleep(1);

    for(int i = 0; i < 10; ++i) {
        void* foo = malloc(256);
        escape(foo);

        usleep(200);
#if defined(USE_FREEPTR)
        freePtr(foo);
#else
        free(foo);
#endif
        usleep(200);
    }
    return 0;
}

The above can be tested with this bash script:

#!/bin/bash

function run {
  gcc $@ -O0 -g test_linkage.c -o test_linkage
  ./test_linkage &
  p=$!
  sleep .1
  heaptrack -p $p
}

set -o xtrace

run
run -Wl,-z,now
run -DTAKE_ADDR
run -DUSE_FREEPTR

The output I get:

$ ./run_tests.sh   
+ run
+ gcc -O0 -g test_linkage.c -o test_linkage
+ p=1507
+ sleep .1
+ ./test_linkage
+ heaptrack -p 1507

heaptrack output will be written to "/home/milian/projects/src/heaptrack/tests/manual/heaptrack.test_linkage.1509.gz"
injecting heaptrack into application via GDB, this might take some time...
injection finished
heaptrack stats:
        allocations:            10
        leaked allocations:     0
        temporary allocations:  10
removing heaptrack injection via GDB, this might take some time...
ptrace: No such process.
No symbol table is loaded.  Use the "file" command.
The program is not being run.
Heaptrack finished! Now run the following to investigate the data:

  heaptrack --analyze "/home/milian/projects/src/heaptrack/tests/manual/heaptrack.test_linkage.1509.gz"
+ run -Wl,-z,now
+ gcc -Wl,-z,now -O0 -g test_linkage.c -o test_linkage
+ p=1553
+ sleep .1
+ ./test_linkage
+ heaptrack -p 1553

heaptrack output will be written to "/home/milian/projects/src/heaptrack/tests/manual/heaptrack.test_linkage.1555.gz"
injecting heaptrack into application via GDB, this might take some time...
injection finished
heaptrack stats:
        allocations:            10
        leaked allocations:     0
        temporary allocations:  10
removing heaptrack injection via GDB, this might take some time...
ptrace: No such process.
No symbol table is loaded.  Use the "file" command.
The program is not being run.
Heaptrack finished! Now run the following to investigate the data:

  heaptrack --analyze "/home/milian/projects/src/heaptrack/tests/manual/heaptrack.test_linkage.1555.gz"
+ run -DTAKE_ADDR
+ gcc -DTAKE_ADDR -O0 -g test_linkage.c -o test_linkage
+ p=1599
+ sleep .1
+ ./test_linkage
+ heaptrack -p 1599

heaptrack output will be written to "/home/milian/projects/src/heaptrack/tests/manual/heaptrack.test_linkage.1601.gz"
injecting heaptrack into application via GDB, this might take some time...
injection finished
heaptrack stats:
        allocations:            10
        leaked allocations:     0
        temporary allocations:  10
removing heaptrack injection via GDB, this might take some time...
ptrace: No such process.
No symbol table is loaded.  Use the "file" command.
The program is not being run.
Heaptrack finished! Now run the following to investigate the data:

  heaptrack --analyze "/home/milian/projects/src/heaptrack/tests/manual/heaptrack.test_linkage.1601.gz"
+ run -DUSE_FREEPTR
+ gcc -DUSE_FREEPTR -O0 -g test_linkage.c -o test_linkage
+ p=1645
+ sleep .1
+ ./test_linkage
+ heaptrack -p 1645

heaptrack output will be written to "/home/milian/projects/src/heaptrack/tests/manual/heaptrack.test_linkage.1647.gz"
injecting heaptrack into application via GDB, this might take some time...
injection finished
heaptrack stats:
        allocations:            10
        leaked allocations:     10
        temporary allocations:  0
removing heaptrack injection via GDB, this might take some time...
ptrace: No such process.
No symbol table is loaded.  Use the "file" command.
The program is not being run.
Heaptrack finished! Now run the following to investigate the data:

  heaptrack --analyze "/home/milian/projects/src/heaptrack/tests/manual/heaptrack.test_linkage.1647.gz"

So only the version where I try to free by using the function pointer is not detected. Is that what you are seeing too? Note that the behavior is unaffected for me with or without your patch...

What version of GCC are you using? Do you maybe have more compiler options enabled by default through some environment variables?

Aha, -DTAKE_ADDR fails when I use ld.bfd instead of ld.gold, I assume you do that too? I still don't see a difference with using -Wl,-z,now though...

This revision was not accepted when it landed; it landed in state Needs Review.Jan 17 2018, 3:39 PM

Closed by commit R45:779dceb8cbe7: Override allocation functions accessible through .rel{a,}.dyn (authored by mwolff). · Explain Why

This revision was automatically updated to reflect the committed changes.

I've adapted and pushed this now. Many thanks Ivan! Much appreciated.

Btw: Do you think we need to do anything about DT_REL? It seems to be unused, but I'm unsure...

I see from your recent commits that you've figured most things out here.

I am indeed using ld.bfd (I'm on Debian, where all packages seem to be built with ld.bfd as linker by default).

ld.gold seems to ignore the -z now option, even though it's documented in its man page. And in your TAKE_ADDR case, ld.gold simply creates two different relocs (unlike ld.bfd), so the issue still doesn't come up.

In the USE_FREEPTR case (as you had it originally, where the pointer is stored before heaptrack_inject is run), there's not much heaptrack_inject can do, unless maybe we want to do something like scan through the appropriate parts of the process memory?

As for DT_REL, it looks to me like 32-bit binaries always use it, whereas 64-bit binaries always use DT_RELA. From your commits, it looks like maybe you already figured this out. I have a 32-bit virtual machine that I can use to help test, if you want me to.

Revision Contents
Changeset List

			Path	Packages
M			heaptrack_inject.cpp (25 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	23658			Dec 8 2017, 4:13 PM	★	★
Diff 2	25357			Jan 15 2018, 12:46 AM	★	★
Diff 3	25542	481bb53	R45:779dceb8cbe77a2d653d5244126a80e0d5d5346f	Jan 17 2018, 3:32 PM	★	★

Diff 25357

View Options

fix heaptrack_inject to find second kind of relocationClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 25357

heaptrack_inject.cpp

fix heaptrack_inject to find second kind of relocation
ClosedPublic
Actions

Revision Contents
Changeset List