Repository: servo/core-foundation-rs (language: rust)

Issue:
Missing null checks
In the `core-foundation` crate, there appears to be a *lot* of functions missing NULL checks in the allocation, potentially causing segfaults when trying to over-allocate (instead of panicking or returning a Result). Examples include [dictionary creation](https://github.com/servo/core-foundation-rs/blob/master/core-foundation/src/dictionary.rs#L46), [array creation](https://github.com/servo/core-foundation-rs/blob/master/core-foundation/src/array.rs#L68) or [string creation](https://github.com/servo/core-foundation-rs/blob/master/core-foundation/src/string.rs#L111). Those functions can all return NULL according to Apple's documentation (e.g. [see return value doc here](https://developer.apple.com/documentation/corefoundation/1516791-cfdictionarycreatemutable?language=objc)

It would seem wise to add some railguards against this somewhere - perhaps `wrap_under_create_rule` should have an explicit NULL-check, causing a panic if it is encountered?


Discussion / hints:
Asserting inside wrap_under_create_rule sounds like a useful first step.
Another potential source of problem is in `CFDictionary::get`. It returns an Option<T>, either `None` if the key is not found or `Some` otherwise. But even if a value is found, it may be null!

Furthermore, `CFMutableDictionary` has a couple soundness holes. Take the following compiling program:

```rust
#[test]
fn this_is_very_ub() {
    let dict: CFMutableDictionary<CFString, CFString> = CFMutableDictionary::new();
    let mut dict_untyped = dict.to_untyped();
    let test = CFString::new("test");
    dict_untyped.add(&test.to_void(), &std::ptr::null());
    let val = dict.get(&test);
    println!("{}", val.char_len());
}
```

This can cause an UB in one of two ways:

- `CFDictionary` internally calls various functions on the value pointer (those are passed as callbacks to the CFDictionaryCreateMutable, core-foundation gives the default values). Having the ability to pass null or even arbitrary pointers can easily cause the internal Core Foundation functions to segfault or otherwise UB. In particular, the `add` function will call `CFRetain` on the null pointer and cause a crash.
- Even if those functions weren't a problem, the `to_untyped()` function allows creating  CFString (or otherwise) from an arbitrary pointer.

**EDIT**: Split into #362
Thinking about this some more, I think a lot of the current collection wrappers are unsound. Most of them (NSArray, NSDictionary, NSSet) expect their arguments to be relatively well-formed, so you simply cannot safely allow the user to pass arbitrary pointers to it. In practice, only NSDictionary is affected since it's the only collection with mutation implemented.

---

Yet another soundness hole: `CFArray::from_copyable` is really, really wrong, as it allows reading uninitialized memory.

```rust
#[test]
fn this_is_very_ub() {
    let arr = CFArray::from_copyable(&[1u8; 2]);
    assert_eq!(arr.get_all_values(), vec![1 as _, 1 as _]);
}
// thread 'array::tests::this_is_very_ub' panicked at 'assertion failed: `(left == right)`
//  left: `[0x600000101, 0x500000000]`,
// right: `[0x1, 0x1]`', core-foundation/src/array.rs:290:9
```

This is because CFArray can only hold pointer-sized values, but `from_copyable` allows filling it with any data that implements `Copy`, no matter the size. I can imagine this causing some really nasty bugs.

Not to mention, I suspect the very idea behind this API to be wrong, as again, CFArray really expects valid pointers to be inside. But that's not readily exploitable from the current API surface afaict.

EDIT: Just realized this is #291

Fix patch:
diff --git a/core-foundation/src/base.rs b/core-foundation/src/base.rs
index 1a09ee6b3..f08f2b2e8 100644
--- a/core-foundation/src/base.rs
+++ b/core-foundation/src/base.rs
@@ -243,6 +243,7 @@ impl TCFType for CFType {
 
     #[inline]
     unsafe fn wrap_under_get_rule(reference: CFTypeRef) -> CFType {
+        assert!(!reference.is_null(), "Attempted to create a NULL object.");
         let reference: CFTypeRef = CFRetain(reference);
         TCFType::wrap_under_create_rule(reference)
     }
@@ -254,6 +255,7 @@ impl TCFType for CFType {
 
     #[inline]
     unsafe fn wrap_under_create_rule(obj: CFTypeRef) -> CFType {
+        assert!(!obj.is_null(), "Attempted to create a NULL object.");
         CFType(obj)
     }
 
diff --git a/core-foundation/src/lib.rs b/core-foundation/src/lib.rs
index c3c612278..eaab9a039 100644
--- a/core-foundation/src/lib.rs
+++ b/core-foundation/src/lib.rs
@@ -95,6 +95,7 @@ macro_rules! impl_TCFType {
 
             #[inline]
             unsafe fn wrap_under_get_rule(reference: $ty_ref) -> Self {
+                assert!(!reference.is_null(), "Attempted to create a NULL object.");
                 let reference = $crate::base::CFRetain(reference as *const ::std::os::raw::c_void) as $ty_ref;
                 $crate::base::TCFType::wrap_under_create_rule(reference)
             }
@@ -106,6 +107,7 @@ macro_rules! impl_TCFType {
 
             #[inline]
             unsafe fn wrap_under_create_rule(reference: $ty_ref) -> Self {
+                assert!(!reference.is_null(), "Attempted to create a NULL object.");
                 // we need one PhantomData for each type parameter so call ourselves
                 // again with @Phantom $p to produce that
                 $ty(reference $(, impl_TCFType!(@Phantom $p))*)
diff --git a/core-foundation/src/propertylist.rs b/core-foundation/src/propertylist.rs
index 14ae173d6..40b834508 100644
--- a/core-foundation/src/propertylist.rs
+++ b/core-foundation/src/propertylist.rs
@@ -121,6 +121,7 @@ impl CFPropertyList {
 
     #[inline]
     pub unsafe fn wrap_under_get_rule(reference: CFPropertyListRef) -> CFPropertyList {
+        assert!(!reference.is_null(), "Attempted to create a NULL object.");
         let reference = CFRetain(reference);
         CFPropertyList(reference)
     }
@@ -147,6 +148,7 @@ impl CFPropertyList {
 
     #[inline]
     pub unsafe fn wrap_under_create_rule(obj: CFPropertyListRef) -> CFPropertyList {
+        assert!(!obj.is_null(), "Attempted to create a NULL object.");
         CFPropertyList(obj)
     }
 
diff --git a/io-surface/src/lib.rs b/io-surface/src/lib.rs
index 44a41c3cb..cc49c7a64 100644
--- a/io-surface/src/lib.rs
+++ b/io-surface/src/lib.rs
@@ -73,6 +73,7 @@ impl TCFType for IOSurface {
 
     #[inline]
     unsafe fn wrap_under_create_rule(obj: IOSurfaceRef) -> IOSurface {
+        assert!(!obj.is_null(), "Attempted to create a NULL object.");
         IOSurface {
             obj: obj,
         }
@@ -92,6 +93,7 @@ impl TCFType for IOSurface {
 
     #[inline]
     unsafe fn wrap_under_get_rule(reference: IOSurfaceRef) -> IOSurface {
+        assert!(!reference.is_null(), "Attempted to create a NULL object.");
         let reference = CFRetain(reference as *const c_void) as IOSurfaceRef;
         TCFType::wrap_under_create_rule(reference)
     }


Test patch: