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

Maybe unsound in set_argv_index and set_omit #24

Open
lwz23 opened this issue Dec 9, 2024 · 2 comments
Open

Maybe unsound in set_argv_index and set_omit #24

lwz23 opened this issue Dec 9, 2024 · 2 comments

Comments

@lwz23
Copy link

lwz23 commented Dec 9, 2024

hello, thank you for your contribution in this project, I am scanning the unsoundness problem in rust project.
I notice the following code:

pub fn set_argv_index(&mut self, i: i32) {
        unsafe { (*self.usage).argvIndex = i };
    }
    pub fn set_omit(&mut self, value: bool) {
        unsafe { (*self.usage).omit = u8::from(value) }
    }

Considering that pub mod table and usage is a pub field, I assume that users can directly manipulate this field, and that set_argv_index is a public function. This potential situation could result in self.usage being a null pointer, and directly dereferencing it might trigger undefined behavior (UB). For safety reasons, I felt it necessary to report this issue. If you have performed checks elsewhere that ensure this is safe, please don’t take offense at my raising this issue.

@lwz23
Copy link
Author

lwz23 commented Dec 9, 2024

here is my PoC:

use std::ptr;

pub struct sqlite3_index_info_sqlite3_index_constraint_usage {
    pub argvIndex: i32,
    pub omit: u8,
}

pub struct Constraint {
    //pub constraint: sqlite3_index_info_sqlite3_index_constraint,
    pub usage: *mut sqlite3_index_info_sqlite3_index_constraint_usage,
    // needed for sqlite3_vtab_* methods
    //index_info: *mut sqlite3_index_info,
    constraint_idx: i32,
}

impl Constraint {
    pub fn set_argv_index(&mut self, i: i32) {
        unsafe { (*self.usage).argvIndex = i };
    }
    pub fn set_omit(&mut self, value: bool) {
        unsafe { (*self.usage).omit = u8::from(value) }
    }
}

fn main(){
    let a:*mut sqlite3_index_info_sqlite3_index_constraint_usage= ptr::null_mut();
    let mut b=Constraint{usage: a, constraint_idx: 0};
    b.set_argv_index(1);
}

I annotated and modified parts of the implementation for convenience, but I believe the overall framework is consistent.

@lwz23
Copy link
Author

lwz23 commented Dec 10, 2024

Maybe same unsoundness problem for

pub fn new(list_value: *mut sqlite3_value) -> Self {

but this is because the new method didn't varify the pointer validity.
and if pass a null pointer it may cause UB in
unsafe { sqlite3ext_vtab_in_first(self.list_value, &mut self.value) }

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

No branches or pull requests

1 participant