1

I'm trying to free memory allocated to a CStringand passed to Python using ctypes. However, Python is crashing with a malloc error:

python(30068,0x7fff73f79000) malloc: *** error for object 0x103be2490: pointer being freed was not allocated 

Here are the Rust functions I'm using to pass the pointer to ctypes:

#[repr(C)]
pub struct Array {
    pub data: *const c_void,
    pub len: libc::size_t,
}

// Build &mut[[f64; 2]] from an Array, so it can be dropped
impl<'a> From<Array> for &'a mut [[f64; 2]] {
    fn from(arr: Array) -> Self {
        unsafe { slice::from_raw_parts_mut(arr.data as *mut [f64; 2], arr.len) }
    }
}

// Build an Array from a Vec, so it can be leaked across the FFI boundary
impl<T> From<Vec<T>> for Array {
    fn from(vec: Vec<T>) -> Self {
        let array = Array {
            data: vec.as_ptr() as *const libc::c_void,
            len: vec.len() as libc::size_t,
        };
        mem::forget(vec);
        array
    }
}

// Build a Vec from an Array, so it can be dropped
impl From<Array> for Vec<[f64; 2]> {
    fn from(arr: Array) -> Self {
        unsafe { Vec::from_raw_parts(arr.data as *mut [f64; 2], arr.len, arr.len) }
    }
}

// Decode an Array into a Polyline
impl From<Array> for String {
    fn from(incoming: Array) -> String {
        let result: String = match encode_coordinates(&incoming.into(), 5) {
            Ok(res) => res,
            // we don't need to adapt the error
            Err(res) => res
        };
        result
    }
}

#[no_mangle]
pub extern "C" fn encode_coordinates_ffi(coords: Array) -> *mut c_char {
    let s: String = coords.into();
    CString::new(s).unwrap().into_raw()
}

And the one I'm using to free the pointer when it's returned by Python

pub extern "C" fn drop_cstring(p: *mut c_char) {
    unsafe { CString::from_raw(p) };
}

And the Python function I'm using to convert the pointer to a str:

def char_array_to_string(res, _func, _args):
    """ restype is c_void_p to prevent automatic conversion to str
    which loses pointer access

    """
    converted = cast(res, c_char_p)
    result = converted.value
    drop_cstring(converted)
    return result

And the Python function I'm using to generate the Array struct to pass into Rust:

class _FFIArray(Structure):
    """
    Convert sequence of float lists to a C-compatible void array
    example: [[1.0, 2.0], [3.0, 4.0]]

    """
    _fields_ = [("data", c_void_p),
                ("len", c_size_t)]

    @classmethod
    def from_param(cls, seq):
        """  Allow implicit conversions """
        return seq if isinstance(seq, cls) else cls(seq)

    def __init__(self, seq, data_type = c_double):
        arr = ((c_double * 2) * len(seq))()
        for i, member in enumerate(seq):
            arr[i][0] = member[0]
            arr[i][1] = member[1]
        self.data = cast(arr, c_void_p)
        self.len = len(seq)

argtype and restype definitions:

encode_coordinates = lib.encode_coordinates_ffi
encode_coordinates.argtypes = (_FFIArray,)
encode_coordinates.restype = c_void_p
encode_coordinates.errcheck = char_array_to_string

drop_cstring = lib.drop_cstring
drop_cstring.argtypes = (c_char_p,)
drop_cstring.restype = None

I'm inclined to think it's not the Rust functions, because a dylib crash would cause a segfault (and the FFI tests pass on the Rust side). I can also continue with other operations in Python after calling the FFI functions – the malloc error occurs when the process exits.

urschrei
  • 25,123
  • 12
  • 43
  • 84
  • @Shepmaster see edit. – urschrei Jul 16 '16 at 18:45
  • 2
    How is `coords.into` implemented? I'm trying to get you to create a [MCVE] so that anyone else can reproduce the error, debug it and tell you the answer. Very few people on SO are magic code mindreaders... Maybe you don't even need the `Array` aspect at all; have you tried removing that and seeing if the crash continues? Maybe it's *only* the `Array`; what about removing the `CString`? – Shepmaster Jul 16 '16 at 19:10
  • @Shepmaster I've added the rest of the traits. The encode function is in an [external crate](https://github.com/urschrei/rust-polyline/blob/master/src/lib.rs#L51-L74) – urschrei Jul 16 '16 at 20:08
  • 1
    *The encode function is in an external crate* — Why don't you create a [MCVE] that inlines the code from the crate, removes unnecessary trait implementations, and someone could paste it directly into a new project and **reproduce** the error? – Shepmaster Jul 17 '16 at 13:37
  • @shepmaster I'm not directly disagreeing with you, but in this case I wouldn't just need to inline a single function, since `encode` refers to other functions in its crate, so I'd be adding ~100 loc to the example. What if I reproduce the cargo.toml, since the `libc` crate needs to be included, too? – urschrei Jul 17 '16 at 13:50
  • *but in this case I wouldn't just need to inline a single function, since encode refers to other functions in its crate* — if the problem is too big for you to reduce it, then it's not appropriate for Stack Overflow. Realize that because you won't do this work, you are instead forcing *multiple* other people to do the work. Note that I'm **not** telling you to put all the code in one file, but to spend the time to replace function calls with stub values, remove unused conditionals, remove modules, etc. I'd believe that this problem could be completely reduced to ~100 lines of code. – Shepmaster Jul 17 '16 at 13:54
  • 2
    [this documentation](https://doc.rust-lang.org/std/vec/struct.Vec.html) for `from_raw_parts` states that "ptr needs to have been previously allocated via String/Vec" which is not the case here. – J.J. Hakala Jul 17 '16 at 14:12

2 Answers2

2

I think that the Rust side of the code assumes ownership of the data and tries to deallocate the data when the process exits, so the Python code is not to be blamed. As a proof, the following C code that calls encode_coordinates_ffi and drop_cstring also causes a segmentation fault.

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>

typedef struct {
    double longitude;
    double latitude;
} coord_t;

typedef struct {
    coord_t * data;
    size_t count;
} points_t;

char * encode_coordinates_ffi(points_t points);
void drop_cstring(void * str);

int main(void)
{
   points_t data;
   coord_t * points;
   char * res;

   data.data = malloc(sizeof(coord_t) * 2);
   data.count = 2;
   points = (coord_t *)data.data;

   points[0].latitude = 1.0;
   points[0].longitude = 2.0;
   points[1].latitude = 3.0;
   points[1].longitude = 4.0;

   res = encode_coordinates_ffi(data);
   printf("%s\n", res);
   free(data.data);
   drop_cstring(res);

   return 0;
}

valgrind -v gives the following information

Invalid free() / delete / delete[] / realloc()
   at 0x4C2CDFB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x4007EB: main (in /tmp/rusti/a.out)
 Address 0x5ea8040 is 0 bytes inside a block of size 32 free'd
   at 0x4C2CDFB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x4EE7FB4: alloc::heap::deallocate::h74ff05db8ae4652e (heap.rs:113)
   by 0x4EE7F52: _$LT$alloc..raw_vec..RawVec$LT$T$GT$$u20$as$u20$std..ops..Drop$GT$::drop::ha72c57f32dae0328 (raw_vec.rs:567)
   by 0x4EE7E5D: alloc..raw_vec..RawVec$LT$$u5b$f64$u3b$$u20$2$u5d$$GT$::drop.6367::h05166e3a96ef1f41 (in /tmp/rusti/polyline_ffi/target/debug/libpolyline_ffi.so)
   by 0x4EE7E45: std..vec..Vec$LT$$u5b$f64$u3b$$u20$2$u5d$$GT$::drop_contents.6364::h68f73d9e22af548c (in /tmp/rusti/polyline_ffi/target/debug/libpolyline_ffi.so)
   by 0x4EE7C69: std..vec..Vec$LT$$u5b$f64$u3b$$u20$2$u5d$$GT$::drop.6314::h68f73d9e22af548c (in /tmp/rusti/polyline_ffi/target/debug/libpolyline_ffi.so)
   by 0x4EE7B9B: polyline_ffi::_$LT$impl$u20$std..convert..From$LT$Array$GT$$u20$for$u20$std..string..String$GT$::from::h3b597d62ca6eb863 (lib.rs:46)
   by 0x4EE84D9: _$LT$T$u20$as$u20$std..convert..Into$LT$U$GT$$GT$::into::h996bdd9d6ba87f7b (convert.rs:209)
   by 0x4EE83EB: encode_coordinates_ffi (lib.rs:57)
   by 0x4007D8: main (in /tmp/rusti/a.out)
 Block was alloc'd at
   at 0x4C2BBCF: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x400795: main (in /tmp/rusti/a.out)

If that free(data.data) is left out, the program finishes without a segmentation fault and valgrind doesn't find any memory leaks.

I would try to implement the interface so that it corresponds to

typedef struct {
    double longitude;
    double latitude;
} coord_t;

int coordinates_ffi(char * dst, size_t n, coord_t * points, size_t npoints);

where dst would be for the encoded string (length limit n, some approximation based on the number of coordinates npoints) so there would be no need for the caller to deallocate the Rust string.

J.J. Hakala
  • 6,136
  • 6
  • 27
  • 61
  • I'm not so familiar with rust so I don't know if that kind of passing of string is possible, or is it easier to return a string owned by rust. – J.J. Hakala Jul 17 '16 at 10:21
  • @Shepmaster I edited my answer to include relevant definitions from that file I had created. – J.J. Hakala Jul 17 '16 at 13:52
  • 2
    My attempt to create a C friendly library for polylines can be found [here](https://github.com/jjhoo/rust-polylines-ffi). If there is some odd looking code, it is due to my near total lack of Rust related experience. – J.J. Hakala Jul 17 '16 at 13:56
2

Thanks to the effort made in J.J. Hakala's answer, I was able to produce a MCVE in pure Rust:

extern crate libc;

use std::ffi::CString;
use libc::c_void;

fn encode_coordinates(coordinates: &Vec<[f64; 2]>) -> String {
    format!("Encoded coordinates {:?}", coordinates)
}

struct Array {
    data: *const c_void,
    len: libc::size_t,
}

impl From<Array> for Vec<[f64; 2]> {
    fn from(arr: Array) -> Self {
        unsafe { Vec::from_raw_parts(arr.data as *mut [f64; 2], arr.len, arr.len) }
    }
}

impl From<Array> for String {
    fn from(incoming: Array) -> String {
        encode_coordinates(&incoming.into())
    }
}

fn encode_coordinates_ffi(coords: Array) -> CString {
    CString::new(String::from(coords)).unwrap()
}

fn main() {
    for _ in 0..10 {
        let i_own_this = vec![[1.0, 2.0], [3.0, 4.0]];

        let array = Array {
            data: i_own_this.as_ptr() as *const _,
            len: i_own_this.len(),
        };

        println!("{:?}", encode_coordinates_ffi(array))
    }
}

This prints:

"Encoded coordinates [[1, 2], [3, 4]]"
"Encoded coordinates [[1, 2], [3, 4]]"
"Encoded coordinates [[0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000012169663452665325, 213780573330512200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000], [3.0000002417770535, 4]]"
"Encoded coordinates [[0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000012169663452665325, 213780573330512200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000], [3.0000002417770535, 4]]"
"Encoded coordinates [[0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000012169663452665325, 213780573330512200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000], [3.0000002417770535, 4]]"
"Encoded coordinates [[0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000012169663452665325, 213780573330512200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000], [3.0000002417770535, 4]]"
"Encoded coordinates [[0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000012169663452665325, 213780573330512200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000], [3.0000002417770535, 4]]"
"Encoded coordinates [[0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000012169663452665325, 213780573330512200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000], [3.0000002417770535, 4]]"
"Encoded coordinates [[0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000012169663452665325, 213780573330512200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000], [3.0000002417770535, 4]]"
"Encoded coordinates [[0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000012169663452665325, 213780573330512200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000], [3.0000002417770535, 4]]"

The primary issue is here:

impl From<Array> for Vec<[f64; 2]> {
    fn from(arr: Array) -> Self {
        unsafe { Vec::from_raw_parts(arr.data as *mut [f64; 2], arr.len, arr.len) }
    }
}

Let's check out the documentation for Vec::from_raw_parts:

This is highly unsafe, due to the number of invariants that aren't checked:

  • ptr needs to have been previously allocated via String/Vec<T> (at least, it's highly likely to be incorrect if it wasn't).
  • length needs to be the length that less than or equal to capacity.
  • capacity needs to be the capacity that the pointer was allocated with.

Violating these may cause problems like corrupting the allocator's internal datastructures.

However, the original code as shown violates the first point - the pointer was allocated by malloc.

Why does this come into play? When you call Vec::from_raw_parts, it takes ownership of the pointer. When the Vec goes out of scope, the pointed-at memory is deallocated. That means that you are attempting to deallocate that pointer multiple times.

Because the safety of the function is determined by what is passed in, the entire function should be marked unsafe. In this this case, that would violate the trait's interface, so you would need to move it elsewhere.

More sanely, you could convert the Array to a slice. This is still unsafe because it depends on the pointer passed in, but it doesn't own the underlying pointer. You can then make the slice into a Vec, allocating new memory and copying the contents over.

Since you are in control of encode_coordinates, you should also change its signature. &Vec<T> is useless in 99.99% of the cases and may actually be less efficient: it requires two pointer dereferences instead of one. Instead, accept a &[T]. This allows a wider range of types to be passed, including arrays and Vecs.

Community
  • 1
  • 1
Shepmaster
  • 388,571
  • 95
  • 1,107
  • 1,366
  • Thanks for the detailed explanation, as ever. I'll make an effort to post reproducible code in future. – urschrei Jul 18 '16 at 15:34