libbpf-tools: Add new feature doublefree #4286

Bojun-Seo · 2022-10-19T06:00:27Z

Add doublefree tool to detect double free. It could detect user level double
free error currently and can be expanded to detect kernel level double free
error. Followings are the usage and example.

Usage:

Usage: doublefree [OPTION...]
Detect and report doublefree error.

-c or -p is a mandatory option
EXAMPLES:
    doublefree -p 1234             # Detect doublefree on process id 1234
    doublefree -c a.out            # Detect doublefree on a.out
    doublefree -c 'a.out arg'      # Detect doublefree on a.out with argument

  -c, --command=COMMAND      Execute the command and detect doublefree
  -p, --pid=PID              Detect doublefree on the specified process
  -v, --verbose              Verbose debug output
  -?, --help                 Give this help list
      --usage                Give a short usage message
  -V, --version              Print program version

Mandatory or optional arguments to long options are also mandatory or optional
for any corresponding short options.

Report bugs to https://github.com/iovisor/bcc/tree/master/libbpf-tools.

Example:

$ cat doublefree_generator.c
#include <unistd.h>
#include <stdlib.h>

int* foo() {
  return (int*)malloc(sizeof(int));
}

void bar(int* p) {
  free(p);
}

int main(int argc, char* argv[]) {
  sleep(10);
  int *val = foo();
  *val = 33;
  bar(val);
  *val = 84;
  bar(val);
  return 0;
}
$ gcc doublefree_generator.c
$ sudo ./doublefree -c a.out
2024-May-15 22:30:24 INFO Execute command: a.out(pid 99584)
Tracing doublefree... Hit Ctrl-C to stop
free(): double free detected in tcache 2

Allocation:
        #1 0x00564d1455819b foo+0x12 (/home/bojun/doublefree/libbpf-tools/a.out+0x119b)
        #2 0x00564d145581e3 main+0x27 (/home/bojun/doublefree/libbpf-tools/a.out+0x11e3)
        #3 0x0070c4e9429d90 [unknown] (/usr/lib/x86_64-linux-gnu/libc.so.6+0x29d90)

First deallocation:
        #1 0x0070c4e94a53e0 free+0x0 (/usr/lib/x86_64-linux-gnu/libc.so.6+0xa53e0)
        #2 0x00564d145581fd main+0x41 (/home/bojun/doublefree/libbpf-tools/a.out+0x11fd)
        #3 0x0070c4e9429d90 [unknown] (/usr/lib/x86_64-linux-gnu/libc.so.6+0x29d90)

Second deallocation:
        #1 0x0070c4e94a53e0 free+0x0 (/usr/lib/x86_64-linux-gnu/libc.so.6+0xa53e0)
        #2 0x00564d14558213 main+0x57 (/home/bojun/doublefree/libbpf-tools/a.out+0x1213)
        #3 0x0070c4e9429d90 [unknown] (/usr/lib/x86_64-linux-gnu/libc.so.6+0x29d90)

libbpf-tools/doublefree.bpf.c

libbpf-tools/doublefree.c

eiffel-fl

Hi!

Thank you for it, this tool is cool!
I tested it and it worked fine:

$ ./a.out &
[2] 99365
$ sudo ./doublefree -p 99365
bar: 0x55c9bd2912a0
baz: 0x55c9bd2912a0
bazz: 84
free 84
free(): double free detected in tcache 2
[2]  + abort (core dumped)  ./a.out
Warn: Is this process alive? pid: 99365
Found double free...
Allocation happended on:
stack_id: 30033
        #1 0x0055c9bc24724b foo
        #2 0x0055c9bc2472a4 main
        #3 0x007fabf0d8c083 __libc_start_main


First deallocation happended on:
stack_id: 9246
        #1 0x007fabf0e026d0 free
        #2 0x0055c9bc2472be main
        #3 0x007fabf0d8c083 __libc_start_main


Second deallocation happended on:
stack_id: 3231
        #1 0x007fabf0e026d0 free
        #2 0x0055c9bc247236 baz
        #3 0x0055c9bc2472d4 main
        #4 0x007fabf0d8c083 __libc_start_main

I nonetheless have a question regarding the memptrs map.

Best regards.

libbpf-tools/doublefree.c

libbpf-tools/doublefree.bpf.c

libbpf-tools/doublefree.c

Bojun-Seo · 2022-11-07T05:17:04Z

Add feature to detect doublefree on kernel(currently support arm and arm64)
Print report with symbol offset and module offset.

Bojun-Seo · 2022-11-21T05:13:45Z

I found some issues on detecting kernel doublefree. So I remove it.
I'll implement kernel doublefree detection later.

eiffel-fl · 2023-09-01T14:20:17Z

I took a slightly new look at this tool and I am wondering if this can be merged with lsan to create an eBPF swiss army knife to detect problem with memory.
What do you think?

Bojun-Seo · 2023-09-07T01:25:58Z

Sounds great to merging tools. In fact, it could be possible to merge memleak, lsan and doublefree, since they hook(attach uprobes) the same points.
But my(and my company's) current interest is on embedded devices, and embedded devices are totally sensitive on memory usage.
So my team decided to split tools to use minimal memory.
But still, I think merging them is cool. So maybe I could merge them later.

eiffel-fl · 2023-09-07T07:33:48Z

But still, I think merging them is cool. So maybe I could merge them later.

Sure, this can definitely be done in a future PR!
The tools themselves are really cool as they are right now!

Bojun-Seo · 2023-12-21T02:19:35Z

I simplify the usage and the example, and I updated it on commit message as well as description.

eiffel-fl

Hi!

I have some comments but they are mainly nits.
I will test it again later.

Best regards.

libbpf-tools/doublefree.bpf.c

libbpf-tools/doublefree.c

eiffel-fl

Hi!

I have one comment regarding the macros which I think can be replaced by a function.
Otherwise, it looks good to me.
Let me know about my comment and I will test it later.

Best regards.

eiffel-fl · 2024-01-26T07:52:53Z

libbpf-tools/doublefree.c

+#define ATTACH_URETPROBE_CHECKED(obj, lib_path, func_name, pid) \
+	do { \
+		off_t func_off = get_elf_func_offset(lib_path, #func_name); \
+		_CHECK_OFFSET(func_off); \
+		_ATTACH_UPROBE(obj, lib_path, func_name##_return, true, func_off, pid); \
+		_CHECK_PROGRAM(obj, func_name##_return); \
+	} while (false)


Cannot you use a function for this? Something like this:

int attach_uprobe(struct bpf_link **func_link, const struct bpf_program *func_prog, char *lib_path, char * func_name, int pid, bool is_ret, bool check) { off_t func_off = get_elf_func_offset(lib_path, func_name); if (func_off < 0) { p_warn("Failed to get func_offset"); return func_off; } *func_link = bpf_program__attach_uprobe(func_prog, is_ret, pid, lib_path, func_off); if (check) { int err = libbpf_get_error(func_link); if (err) { p_warn("Failed to attach %s: %d", #func_name, err); return err; } } return 0; }

You would call it like the following:

/* Think to check the return code. */ attach_uprobe(&obj->link.malloc_return, obj->progs.malloc_return, libc_path, "malloc" env.pid, true, true); attach_uprobe(&obj->link.free_entry, obj->progs.free_entry, libc_path, "free", env.pid, false, true); /* ... */ attach_uprobe(&obj->link.aligned_alloc_return, obj->progs.aligned_alloc_return, libc_path, "alligned_alloc", env.pid, true, false); attach_uprobe(&obj->link.valloc_return, obj->progs.valloc_return, libc_path, "valloc", env.pid, true, false);

You could even declare a struct to hold everything:

struct probe { struct bpf_link **link; struct bpf_program *prog; const char *name; bool is_ret; bool check; }; /* ... */ struct probe probes[] = { { .link = &obj->link.malloc_return, .prog = obj->progs.malloc_return, .name = "malloc", .is_ret = true, .check = true, }, /* ... */ }; for (int i = 0; i < sizeof(probes) / sizeof(probes[0]); i++) { attach_uprobe(probes[i].link, probes[i].prog, libc_path, probes[i].name, env.pid, probes[i].is_ret, probes[i].check); }

What do you think?

Sounds great! I always appreciate your valuable opinion.
I prefer the second code you suggested(using struct).
And I think it would be better to pass struct probe instance(using pointer) instead of each members to attach_uprobe function, like followings.

for (int i = 0; i < sizeof(probes) / sizeof(probes[0]); i++) { attach_uprobe(&probes[i], libc_path); }

What do you think?

And I think it would be better to pass struct probe instance(using pointer) instead of each members to attach_uprobe function, like followings.

Far better approach, it is easier to read as we have less arguments and also easier to maintain!

I changed uprobe attaching codes. I tried implementing it while incorporating your opinion, but in a slightly different way.

ekyooo

"ERROR Failed to poll perf_buffer" in usage example always displayed?

libbpf-tools/doublefree.c

ekyooo · 2024-02-01T05:36:07Z

libbpf-tools/doublefree.c

+			p_err("Failed to poll perf_buffer");
+			break;
+		}
+		if (getpgid(env.pid) < 0) {


I have a question. Is there no need to limit it to the ESRCH errno case?

There is no need to limit it to the ESRCH errno case according to the following manual page.
https://linux.die.net/man/2/getpgid
getpgid returns -1 on error and errno is set appropriately.
And there is only one errno for getpgid, which is ESRCH.
Which means, if getpgid fails, the errno should be ESRCH.

Thanks for your question.

And I found that calling getpgid is not necessary. So I remove that code.

ekyooo · 2024-02-01T05:41:47Z

libbpf-tools/doublefree.c

+
+	err = bpf_map_lookup_elem(fd, &key, &val);
+	if (err < 0) {
+		p_err("Failed to lookup elem on fd");


Did you intend the following code?

p_err("Failed` to lookup elem on fd: %d", fd);

You are right. I'll change the code. Thanks!

I'd rather change the code to print strerror(errno) instead of fd

libbpf-tools/doublefree.c

ekyooo · 2024-02-01T05:53:19Z

libbpf-tools/doublefree.c

+		ptr = strtok(NULL, delim);
+	} else {
+		p_err("failed to exec %s", cmd);
+		goto cleanup;


How about handling the error cases first?
This sentence duplicates line 196.

I can and will remove duplicates but cannot handle the error cases first.
Because error case can be checked after tokenizing or calling fork.

libbpf-tools/doublefree.c

ekyooo · 2024-02-01T05:59:18Z

libbpf-tools/doublefree.c

+		return;
+	}
+	syms = syms_cache__get_syms(syms_cache, env.pid);
+	if (syms != NULL) {


how about handling err case first?

Sounds nice, that change could reduce one indent as well.

libbpf-tools/doublefree.c

ekyooo · 2024-02-02T02:27:33Z

libbpf-tools/doublefree.c

+	int deallocs_fd = bpf_map__fd(obj->maps.deallocs);
+
+	if (e->err == -1) {
+		p_err("This message should not be printed..");


How about removing code that shouldn't happen?

It's a good point. However, thinking that something will never be executed is just the developer's assumption. In software development, it is not uncommon for code that should not be executed to actually be executed. Especially when contributing code to open source projects, where multiple developers are involved, there is a higher possibility of unintentionally executing code that should not be executed. Therefore, I believe that this code should be kept as a precaution for future mistakes by someone else. However, the message itself seems lengthy and lacks specific details. I will consider possible improvements.

I didn't remove the message but changed it.

Bojun-Seo

"ERROR Failed to poll perf_buffer" in usage example always displayed?

Yes, but it would be better not to be printed in normal case.

Bojun-Seo · 2024-02-06T01:23:17Z

libbpf-tools/doublefree.c

+			p_err("Failed to poll perf_buffer");
+			break;
+		}
+		if (getpgid(env.pid) < 0) {


There is no need to limit it to the ESRCH errno case according to the following manual page.
https://linux.die.net/man/2/getpgid
getpgid returns -1 on error and errno is set appropriately.
And there is only one errno for getpgid, which is ESRCH.
Which means, if getpgid fails, the errno should be ESRCH.

Thanks for your question.

eiffel-fl

Hi!

I just found some small nits, but I think we are ready to go:

$ sudo ./doublefree -c /tmp/doublefree_generator                                                                                                                       (remotes/bojun/doublefree) *%
[sudo] Mot de passe de francis : 
Désolé, essayez de nouveau.
[sudo] Mot de passe de francis : 
2024-Feb-29 16:20:13 INFO Execute command: /tmp/doublefree_generator(pid 94846)
Tracing doublefree... Hit Ctrl-C to stop
free(): double free detected in tcache 2

Allocation:
        #1 0x0056477a65b19b foo+0x12
        #2 0x0056477a65b1e3 main+0x27
        #3 0x007f2259229d90 [unknown]

First deallocation:
        #1 0x007f22592a53e0 free+0
        #2 0x0056477a65b1fd main+0x41
        #3 0x007f2259229d90 [unknown]

Second deallocation:
        #1 0x007f22592a53e0 free+0
        #2 0x0056477a65b213 main+0x57
        #3 0x007f2259229d90 [unknown]

Best regards.

libbpf-tools/doublefree.c

eiffel-fl · 2024-02-29T09:10:58Z

libbpf-tools/doublefree.c

+		execve(filepath, argv, NULL);
+		free(argv);
+
+		return -1;


Even though the fork()/exec() is successful, you still return -1 here.
Is this what you want to achieve?

Thank you for the good question.
When execve succeeds, it launches a new program and the current process(child)'s execution flow is not passed to subsequent lines of code. As a result, employing an if statement for error checking is unnecessary; if the execution flow reaches beyond the execve invocation, it inherently signifies an error has occurred.

When execve succeeds, it launches a new program and the current process(child)'s execution flow is not passed to subsequent lines of code. As a result, employing an if statement for error checking is unnecessary; if the execution flow reaches beyond the execve invocation, it inherently signifies an error has occurred.

Indeed! Long time I did not write C code and fork()/exec(), seems I begin to rust 🤣!

libbpf-tools/doublefree.c

eiffel-fl

Hi!

I just have one comment with your latest push, but you can ignore it.

Best regards.

eiffel-fl · 2024-05-13T15:44:45Z

libbpf-tools/doublefree.c

+		if (!err) {
+			if (sinfo.sym_name)
+				printf(" %s+0x%lx", sinfo.sym_name, sinfo.sym_offset);
+			else
+				printf(" [unknown]");
+
+			printf(" (%s+0x%lx)\n", sinfo.dso_name, sinfo.dso_offset);
+		}


Suggested change

if (!err) {

if (sinfo.sym_name)

printf(" %s+0x%lx", sinfo.sym_name, sinfo.sym_offset);

else

printf(" [unknown]");

printf(" (%s+0x%lx)\n", sinfo.dso_name, sinfo.dso_offset);

}

if (err)

continue

if (sinfo.sym_name)

printf(" %s+0x%lx (%s+0x%lx)\n", sinfo.sym_name, sinfo.sym_offset, sinfo.dso_name, sinfo.dso_offset);

else

printf(" [unknown] (%s+0x%lx)\n", sinfo.dso_name, sinfo.dso_offset);

It seems more readable. I'll change the code as you suggested.
Thank you!

I found that the location of newline is not proper in my code.
So I change it. I can find it thanks for your help!

Add doublefree tool to detect double free. It could detect user level double free error currently and can be expanded to detect kernel level double free error. Followings are the usage and example. Usage: $ ./doublefree -h Usage: doublefree [OPTION...] Detect and report doublefree error. -c or -p is a mandatory option EXAMPLES: doublefree -p 1234 # Detect doublefree on process id 1234 doublefree -c a.out # Detect doublefree on a.out doublefree -c 'a.out arg' # Detect doublefree on a.out with argument -c, --command=COMMAND Execute the command and detect doublefree -p, --pid=PID Detect doublefree on the specified process -v, --verbose Verbose debug output -?, --help Give this help list --usage Give a short usage message -V, --version Print program version Mandatory or optional arguments to long options are also mandatory or optional for any corresponding short options. Report bugs to https://github.com/iovisor/bcc/tree/master/libbpf-tools. Example: $ cat doublefree_generator.c #include <unistd.h> #include <stdlib.h> int* foo() { return (int*)malloc(sizeof(int)); } void bar(int* p) { free(p); } int main(int argc, char* argv[]) { sleep(10); int *val = foo(); *val = 33; bar(val); *val = 84; bar(val); return 0; } $ gcc doublefree_generator.c $ sudo ./doublefree -c a.out 2024-May-15 22:30:24 INFO Execute command: a.out(pid 99584) Tracing doublefree... Hit Ctrl-C to stop free(): double free detected in tcache 2 Allocation: iovisor#1 0x00564d1455819b foo+0x12 (/home/bojun/doublefree/libbpf-tools/a.out+0x119b) iovisor#2 0x00564d145581e3 main+0x27 (/home/bojun/doublefree/libbpf-tools/a.out+0x11e3) iovisor#3 0x0070c4e9429d90 [unknown] (/usr/lib/x86_64-linux-gnu/libc.so.6+0x29d90) First deallocation: iovisor#1 0x0070c4e94a53e0 free+0x0 (/usr/lib/x86_64-linux-gnu/libc.so.6+0xa53e0) iovisor#2 0x00564d145581fd main+0x41 (/home/bojun/doublefree/libbpf-tools/a.out+0x11fd) iovisor#3 0x0070c4e9429d90 [unknown] (/usr/lib/x86_64-linux-gnu/libc.so.6+0x29d90) Second deallocation: iovisor#1 0x0070c4e94a53e0 free+0x0 (/usr/lib/x86_64-linux-gnu/libc.so.6+0xa53e0) iovisor#2 0x00564d14558213 main+0x57 (/home/bojun/doublefree/libbpf-tools/a.out+0x1213) iovisor#3 0x0070c4e9429d90 [unknown] (/usr/lib/x86_64-linux-gnu/libc.so.6+0x29d90)

Bojun-Seo requested review from drzaeus77, goldshtn, yonghong-song, 4ast, brendangregg and davemarchevsky as code owners October 19, 2022 06:00

chenhengqi reviewed Oct 20, 2022

View reviewed changes

libbpf-tools/doublefree.bpf.c Outdated Show resolved Hide resolved

libbpf-tools/doublefree.bpf.c Outdated Show resolved Hide resolved

libbpf-tools/doublefree.bpf.c Outdated Show resolved Hide resolved

libbpf-tools/doublefree.c Outdated Show resolved Hide resolved

Bojun-Seo force-pushed the doublefree branch 2 times, most recently from 9125f3c to e3dd3ca Compare October 24, 2022 05:23

Bojun-Seo mentioned this pull request Oct 24, 2022

libbpf-tools: Fix trace_helper symbol search bug #4158

Merged

Bojun-Seo force-pushed the doublefree branch from e3dd3ca to 9d50bcc Compare October 24, 2022 07:23

eiffel-fl reviewed Oct 28, 2022

View reviewed changes

libbpf-tools/doublefree.c Outdated Show resolved Hide resolved

libbpf-tools/doublefree.bpf.c Show resolved Hide resolved

libbpf-tools/doublefree.c Outdated Show resolved Hide resolved

Bojun-Seo force-pushed the doublefree branch 2 times, most recently from a01519a to aa8a849 Compare November 7, 2022 05:08

Bojun-Seo force-pushed the doublefree branch from aa8a849 to 2420682 Compare November 9, 2022 01:44

Bojun-Seo force-pushed the doublefree branch from 2420682 to 0b01ca7 Compare November 21, 2022 05:12

Bojun-Seo changed the title ~~bpf-tools: Add new feature(doublefree)~~ libbpf-tools: Add new feature doublefree Sep 11, 2023

Bojun-Seo force-pushed the doublefree branch 3 times, most recently from ce9f147 to af89da4 Compare December 21, 2023 02:13

Bojun-Seo force-pushed the doublefree branch 2 times, most recently from aae151a to 86e5525 Compare December 21, 2023 04:51

eiffel-fl reviewed Dec 21, 2023

View reviewed changes

Bojun-Seo force-pushed the doublefree branch from 86e5525 to 8cd3199 Compare January 25, 2024 04:39

eiffel-fl reviewed Jan 26, 2024

View reviewed changes

ekyooo reviewed Feb 1, 2024

View reviewed changes

ekyooo reviewed Feb 2, 2024

View reviewed changes

Bojun-Seo commented Feb 6, 2024

View reviewed changes

Bojun-Seo force-pushed the doublefree branch 2 times, most recently from e661c38 to d451f55 Compare February 29, 2024 09:22

eiffel-fl approved these changes Feb 29, 2024

View reviewed changes

Bojun-Seo force-pushed the doublefree branch from d451f55 to 0a4bc52 Compare March 1, 2024 02:30

Bojun-Seo force-pushed the doublefree branch from 0a4bc52 to 0609086 Compare May 2, 2024 05:37

eiffel-fl reviewed May 13, 2024

View reviewed changes

Bojun-Seo force-pushed the doublefree branch from 0609086 to 7d39b93 Compare May 15, 2024 13:39

libbpf-tools: Add new feature doublefree #4286

Are you sure you want to change the base?

libbpf-tools: Add new feature doublefree #4286

Conversation

Bojun-Seo commented Oct 19, 2022 • edited

eiffel-fl left a comment

Choose a reason for hiding this comment

Bojun-Seo commented Nov 7, 2022

Bojun-Seo commented Nov 21, 2022

eiffel-fl commented Sep 1, 2023

Bojun-Seo commented Sep 7, 2023

eiffel-fl commented Sep 7, 2023

Bojun-Seo commented Dec 21, 2023

eiffel-fl left a comment • edited

Choose a reason for hiding this comment

eiffel-fl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bojun-Seo Jan 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekyooo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bojun-Seo left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eiffel-fl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bojun-Seo Feb 29, 2024 • edited

Choose a reason for hiding this comment

eiffel-fl Mar 1, 2024 • edited

Choose a reason for hiding this comment

eiffel-fl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bojun-Seo commented Oct 19, 2022 •

edited

eiffel-fl left a comment •

edited

Bojun-Seo Jan 30, 2024 •

edited

Bojun-Seo left a comment •

edited

Bojun-Seo Feb 29, 2024 •

edited

eiffel-fl Mar 1, 2024 •

edited