axboe / liburing

Library providing helpers for the Linux kernel io_uring support
MIT License
2.72k stars 393 forks source link

`io_uring_free_probe` `ptr` not set to `NULL` #1059

Closed YoSTEALTH closed 5 months ago

YoSTEALTH commented 5 months ago

if io_uring_free_probe(probe) is being called multiple times, the second time it seems to hang! from what I can tell probe.ptr is not set to NULL when io_uring_free_probe was called the first time.

ammarfaizi2 commented 5 months ago

Why is this an issue?

Double-free is a programming mistake. You need to fix your code.

What is probe.ptr anyway? struct io_uring_probe is like this, there is no ptr field.

struct io_uring_probe {
    __u8 last_op;   /* last opcode supported */
    __u8 ops_len;   /* length of ops[] array below */
    __u16 resv;
    __u32 resv2[3];
    struct io_uring_probe_op ops[];
};
ammarfaizi2 commented 5 months ago

io_uring_free_probe itself passes the argument directly to free(). I am not sure which function you're talking about.

__cold void io_uring_free_probe(struct io_uring_probe *probe)
{
    free(probe);
}
YoSTEALTH commented 5 months ago

When you free(probe) probe should be set to NULL this indicates the memory has been freed. I am not sure why free() does not do this!

I have accounted for this in my code. https://github.com/YoSTEALTH/Liburing/blob/master/src/liburing/probe.pyx#L24-L26

If user forget to call io_uring_free_probe, its suppose to auto call io_uring_free_probe to free the probe, thus multiple checks. You can see it auto call here https://github.com/YoSTEALTH/Liburing/blob/master/src/liburing/probe.pyx#L7-L10

YoSTEALTH commented 5 months ago

If I use free() directly and not use io_uring_free_probe, I get free(): invalid pointer error. This does not happens with any other code, that uses free(), just with io_uring_get_probe + io_uring_opcode_supported + io_uring_free_probe. So something odd is happening.

ammarfaizi2 commented 5 months ago

When you free(probe) probe should be set to NULL this indicates the memory has been freed. I am not sure why free() does not do this!

That's a local variable; setting it to NULL doesn't mean anything.

__cold void io_uring_free_probe(struct io_uring_probe *probe)
{
    free(probe);
    probe = NULL; // this is useless
}

If you do that, probe = NULL is absolutely useless as it goes out of scope immediately. You don't even understand the very basic pointer thing. Please learn C language.

ammarfaizi2 commented 5 months ago

If user forget to call io_uring_free_probe, its suppose to auto call io_uring_free_probe to free the probe, thus multiple checks. You can see it auto call here https://github.com/YoSTEALTH/Liburing/blob/master/src/liburing/probe.pyx#L7-L10

You can obviously do that, but it's not liburing responsibility to handle that.

If users forget to call free(), it's their mistake, they should fix their code, not liburing.

ammarfaizi2 commented 5 months ago

If I use free() directly and not use io_uring_free_probe, I get free(): invalid pointer error. This does not happens with any other code, that uses free(), just with io_uring_get_probe + io_uring_opcode_supported + io_uring_free_probe. So something odd is happening.

You must always use io_uring_free_probe() to free the resulting probe because liburing doesn't use free() from libc.

YoSTEALTH commented 5 months ago

I agree setting probe = NULL; is useless and so is me setting:

io_uring_free_probe_c(probe.ptr)
if probe.ptr is not NULL:
    probe.ptr = NULL  # <-this is not something I should be setting either!

That's why I am reporting this as a bug there must be something else going on with io_uring_free_probe that isn't freeing the pointer correctly!

ammarfaizi2 commented 5 months ago

That's why I am reporting this as a bug there must be something else going on with io_uring_free_probe that isn't freeing the pointer correctly!

free() doesn't set the pointer to NULL and never did; the pointer still exists, but it becomes a dangling pointer, and the pointer is illegal to be dereferenced after being freed.

Calling free() to the same pointer twice is also illegal; it's a programming mistake called double-free. This is really not a liburing issue.

alviroiskandar commented 5 months ago

When you free(probe) probe should be set to NULL this indicates the memory has been freed. I am not sure why free() does not do this!

@YoSTEALTH bruh, your complaint makes zero sense, free() in C doesn't do that in the first place. You don't understand C programming. The bug is in your code calling io_uring_free_probe() twice to the same ptr. First, you must know, in C that this is wrong

p = malloc(100);
free(p);
free(p); // might explode (crash, random behavior, corrupting something, etc.)!

Just FYI, copied from CWE mitre org:

When a program calls free() twice with the same argument, the program's memory management data structures become corrupted. This corruption can cause the program to crash or, in some circumstances, cause two later calls to malloc() to return the same pointer. If malloc() returns the same value twice and the program later gives the attacker control over the data that is written into this doubly-allocated memory, the program becomes vulnerable to a buffer overflow attack.

Fix your Python wrapper man.

exception: free(NULL) is valid and always fine

YoSTEALTH commented 5 months ago

Ok, talked to few people and looks like this is a special case where I have to manually set the pointer to NULL since its in a conditional statement.

Thanks @ammarfaizi2 @alviroiskandar for your input.