Skip to content

dc_array: panic on null pointers and out of range indexes #333

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.

Sign up for GitHub

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

Merged
link2xt merged 1 commit into from
Aug 13, 2019
Merged

dc_array: panic on null pointers and out of range indexes #333

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 53 additions & 92 deletions src/dc_array.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,120 +160,81 @@ impl From<Vec<dc_location>> for dc_array_t {
}

pub unsafe fn dc_array_unref(array: *mut dc_array_t) {
if array.is_null() {
return;
}
assert!(!array.is_null());
Box::from_raw(array);
}

pub unsafe fn dc_array_add_uint(array: *mut dc_array_t, item: uintptr_t) {
if !array.is_null() {
(*array).add_uint(item);
}
assert!(!array.is_null());
(*array).add_uint(item);
}

pub unsafe fn dc_array_add_id(array: *mut dc_array_t, item: uint32_t) {
if !array.is_null() {
(*array).add_id(item);
}
assert!(!array.is_null());
(*array).add_id(item);
}

pub unsafe fn dc_array_add_ptr(array: *mut dc_array_t, item: *mut libc::c_void) {
dc_array_add_uint(array, item as uintptr_t);
}

pub unsafe fn dc_array_get_cnt(array: *const dc_array_t) -> size_t {
if array.is_null() {
0
} else {
(*array).len()
}
assert!(!array.is_null());
(*array).len()
}

pub unsafe fn dc_array_get_uint(array: *const dc_array_t, index: size_t) -> uintptr_t {
if array.is_null() || index >= (*array).len() {
0
} else {
(*array).get_uint(index)
}
assert!(!array.is_null());
Copy link
Contributor

@Jikstra Jikstra Aug 13, 2019

Choose a reason for hiding this comment

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

Panicing on a null pointer changes the behaviour of the ffi, i think we should avoid this. If it's null, we should return a 0

https://github.com/deltachat/deltachat-core/blob/e4ce281dc2c8cb1d0765f0e2e73f7846cbba3faa/src/dc_array.c#L252

Copy link
Contributor

@Jikstra Jikstra Aug 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@r10s what do you think about this? Is it likely that we break android/ios at some points with this change?

<Jikstra> we have to be careful with panics in the ffi just because array is a null pointer
<Jikstra> the api allowed us to do weird stuff, panicing will break all apps based on -core-rust
<link2xt[m]> yes, but such panics can be easily debugged
<link2xt[m]> if FFI call panics, it panics immediately
<link2xt[m]> and then you know you should not pass invalid value into it
<link2xt[m]> a simple if is enough to fix it in this case, but it may happen to be an actual bug

Copy link
Collaborator Author

@link2xt link2xt Aug 13, 2019

Choose a reason for hiding this comment

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

See #291 for previous discussion of similar changes. No input from @r10s there, but it got merged.

(*array).get_uint(index)
}

pub unsafe fn dc_array_get_id(array: *const dc_array_t, index: size_t) -> uint32_t {
if array.is_null() || index >= (*array).len() {
0
} else {
(*array).get_id(index)
}
assert!(!array.is_null());
(*array).get_id(index)
}

pub unsafe fn dc_array_get_ptr(array: *const dc_array_t, index: size_t) -> *mut libc::c_void {
if array.is_null() || index >= (*array).len() {
std::ptr::null_mut()
} else {
(*array).get_ptr(index)
}
assert!(!array.is_null());
(*array).get_ptr(index)
Copy link

@KAction KAction Aug 11, 2019

Choose a reason for hiding this comment

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

Here you remove index >= (*array).len() check.

Copy link
Collaborator Author

@link2xt link2xt Aug 11, 2019

Choose a reason for hiding this comment

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

get_ptr() does this check. It is safe, actually.

}

pub unsafe fn dc_array_get_latitude(array: *const dc_array_t, index: size_t) -> libc::c_double {
if array.is_null() || index >= (*array).len() {
0.0
} else {
(*array).get_latitude(index)
}
assert!(!array.is_null());
(*array).get_latitude(index)
}

pub unsafe fn dc_array_get_longitude(array: *const dc_array_t, index: size_t) -> libc::c_double {
if array.is_null() || index >= (*array).len() {
0.0
} else {
(*array).get_longitude(index)
}
assert!(!array.is_null());
(*array).get_longitude(index)
}

pub unsafe fn dc_array_get_accuracy(array: *const dc_array_t, index: size_t) -> libc::c_double {
if array.is_null() || index >= (*array).len() {
0.0
} else {
(*array).get_accuracy(index)
}
assert!(!array.is_null());
(*array).get_accuracy(index)
}

pub unsafe fn dc_array_get_timestamp(array: *const dc_array_t, index: size_t) -> i64 {
if array.is_null() || index >= (*array).len() {
0
} else {
(*array).get_timestamp(index)
}
assert!(!array.is_null());
(*array).get_timestamp(index)
}

pub unsafe fn dc_array_get_chat_id(array: *const dc_array_t, index: size_t) -> uint32_t {
if array.is_null() || index >= (*array).len() {
0
} else {
(*array).get_chat_id(index)
}
assert!(!array.is_null());
(*array).get_chat_id(index)
}

pub unsafe fn dc_array_get_contact_id(array: *const dc_array_t, index: size_t) -> uint32_t {
if array.is_null() || index >= (*array).len() {
0
} else {
(*array).get_contact_id(index)
}
assert!(!array.is_null());
(*array).get_contact_id(index)
}

pub unsafe fn dc_array_get_msg_id(array: *const dc_array_t, index: size_t) -> uint32_t {
if array.is_null() || index >= (*array).len() {
0
} else {
(*array).get_msg_id(index)
}
assert!(!array.is_null());
(*array).get_msg_id(index)
}

pub unsafe fn dc_array_get_marker(array: *const dc_array_t, index: size_t) -> *mut libc::c_char {
if array.is_null() || index >= (*array).len() {
return std::ptr::null_mut();
}
assert!(!array.is_null());

if let dc_array_t::Locations(v) = &*array {
if let Some(s) = &v[index].marker {
Expand All @@ -282,7 +243,7 @@ pub unsafe fn dc_array_get_marker(array: *const dc_array_t, index: size_t) -> *m
std::ptr::null_mut()
}
} else {
std::ptr::null_mut()
panic!("Not an array of locations");
}
}

Expand All @@ -297,9 +258,7 @@ pub unsafe fn dc_array_get_marker(array: *const dc_array_t, index: size_t) -> *m
* 1=Location was reported independently.
*/
pub unsafe fn dc_array_is_independent(array: *const dc_array_t, index: size_t) -> libc::c_int {
if array.is_null() || index >= (*array).len() {
return 0;
}
assert!(!array.is_null());

if let dc_array_t::Locations(v) = &*array {
v[index].independent as libc::c_int
Expand All @@ -313,9 +272,8 @@ pub unsafe fn dc_array_search_id(
needle: uint32_t,
ret_index: *mut size_t,
) -> bool {
if array.is_null() {
return false;
}
assert!(!array.is_null());

if let Some(i) = (*array).search_id(needle as uintptr_t) {
if !ret_index.is_null() {
*ret_index = i
Expand All @@ -327,9 +285,8 @@ pub unsafe fn dc_array_search_id(
}

pub unsafe fn dc_array_get_raw(array: *const dc_array_t) -> *const uintptr_t {
if array.is_null() {
return 0 as *const uintptr_t;
}
assert!(!array.is_null());

if let dc_array_t::Uint(v) = &*array {
v.as_ptr()
} else {
Expand All @@ -346,27 +303,24 @@ pub fn dc_array_new_locations(initsize: size_t) -> *mut dc_array_t {
}

pub unsafe fn dc_array_empty(array: *mut dc_array_t) {
if array.is_null() {
return;
}
assert!(!array.is_null());

(*array).clear()
}

pub unsafe fn dc_array_duplicate(array: *const dc_array_t) -> *mut dc_array_t {
if array.is_null() {
std::ptr::null_mut()
} else {
(*array).clone().into_raw()
}
assert!(!array.is_null());

(*array).clone().into_raw()
}

pub unsafe fn dc_array_get_string(
array: *const dc_array_t,
sep: *const libc::c_char,
) -> *mut libc::c_char {
if array.is_null() || sep.is_null() {
return dc_strdup(b"\x00" as *const u8 as *const libc::c_char);
}
assert!(!array.is_null());
assert!(!sep.is_null());

if let dc_array_t::Uint(v) = &*array {
let cnt = v.len();
let sep = as_str(sep);
Expand Down Expand Up @@ -412,10 +366,6 @@ mod tests {
);
}

assert_eq!(dc_array_get_id(arr, -1i32 as size_t), 0);
assert_eq!(dc_array_get_id(arr, 1000 as size_t), 0);
assert_eq!(dc_array_get_id(arr, 1001 as size_t), 0);

dc_array_empty(arr);

assert_eq!(dc_array_get_cnt(arr), 0);
Expand Down Expand Up @@ -443,4 +393,15 @@ mod tests {
dc_array_unref(arr);
}
}

#[test]
#[should_panic]
fn test_dc_array_out_of_bounds() {
let arr = dc_array_new(7);
for i in 0..1000 {
unsafe { dc_array_add_id(arr, (i + 2) as uint32_t) };
}
unsafe { dc_array_get_id(arr, 1000) };
}

}