Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

report unsound issue in bcc #2042

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

llooFlashooll
Copy link

For more information, see: rust-bcc/issues/200
In the meantime, bcc will no longer be maintained. Users are encouraged to migrate to libbpf-rs.

Copy link
Contributor

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fn null_or_mut_ptr<T>(s: &mut Vec<u8>) -> *mut T {
    if s.capacity() == 0 {
        ptr::null_mut()
    } else {
        s.as_mut_ptr() as *mut T
    }
}

This is safe function containing safe code. I do not see any credible way this could be unsound.

@llooFlashooll
Copy link
Author

llooFlashooll commented Aug 21, 2024

Hi, I think this safe function contains unsafe code which is the raw pointer. And conversion between raw pointer is unsound since it bypasses the Rust safety guarantee. It can create a misaligned pointer.

For example, by calling this function, we can construct this example. Besides, the generic T can be any other possible types. The u8 type can be cast into any other type, not just this case.

fn main() {
    let mut vec = vec![1u8];
    let ptr = null_or_mut_ptr::<u64>(&mut vec);
    unsafe {
        let value: u64 = *ptr;
    }
}

@Skgland
Copy link
Contributor

Skgland commented Aug 21, 2024

Your example while indeed incorrect rust does not discredit null_or_mut_ptr.
The source of the incorrectness is the unsafe block added in your example not null_or_mut_ptr.
Pointers created in a safe manner aren't inherently unsafe, even if they might be nonsensical or easy to misuse in unsafe code. It's the responsibility of the unsafe code to not use the pointer incorrectly.

As fare as I can tell null_or_mut_ptr in rust-bcc is only used thrice.
Twice as an argument to bpf_prog_load and once as an argument to bcc_prog_load.
At a quick glance (didn't check all versions) the relevant argument is of type c_char in each case which is an alias for either u8 or i8 and as such layout compatible with u8.

Not involving a generic i.e. using c_char instead of a generic T would be better to not accidentally cause UB/unsoudness, but I don't see an immediate problem as is.

@llooFlashooll
Copy link
Author

llooFlashooll commented Aug 22, 2024

Hi, thanks for your detailed and reasonable reply! I agree with your comment, and the unsafe consequence would depend on how the users use the code. Based on your description, can I switch to apply for an ID under the unmaintained category? Seems like this repo still needs further maintenance while doesn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants