Skip to content

Retain the arrays returned from Features's accessors #68

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

Merged
merged 2 commits into from
Apr 23, 2016

Conversation

mmetcalfe
Copy link
Contributor

Currently, calling accessor methods on a Features object causes a crash due to a double free.

A (reasonably) minimal example of this problem is demonstrated in the following program:

// file: af_bug_mwe.rs
extern crate arrayfire as af;
use std::mem;
fn main() {
    af::set_device(0);
    af::info();

    // Load the test image: http://www.smufl.org/wp-content/uploads/M3.jpeg
    let file_name = String::from("../../smufl-november2-test.jpg");
    let img_grey = af::load_image(file_name, false).unwrap();
    {
        let features = af::fast(&img_grey, 20.0, 9, true, 0.05, 10).unwrap();
        let num_features = features.num_features().unwrap() as usize;
        println!("num_features: {:?}", num_features);

        let af_xpos = features.xpos().unwrap();
        println!("num_elements: {:?}", af_xpos.elements().unwrap());

        // unsafe { mem::forget(af_xpos); }

        println!("End features scope.");
    }
    println!("End program.");
}

Using the current devel branch of arrayfire-rust (f22a5fb) the program outputs the following:

ArrayFire v3.2.2 (OpenCL, 64-bit Mac OSX, build 7507b61)
[0] APPLE   : HD Graphics 4000
-1- APPLE   : GeForce GT 650M
num_features: 3827
num_elements: 3827
End features scope.
af_bug_mwe(18660,0x7fff7aa27300) malloc: *** error for object 0x7facabcf43e0: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
An unknown error occurred

To learn more, run the command again with --verbose.

Un-commenting the unsafe line in the above example prevents the error.

This pull request changes the feat_func_def macro in the vision module to retain the arrays returned from af_get_features_* functions, in the same way that ArrayFire's current c++ implementation does.

With this change applied the output of the above program is as expected:

ArrayFire v3.2.2 (OpenCL, 64-bit Mac OSX, build 7507b61)
[0] APPLE   : HD Graphics 4000
-1- APPLE   : GeForce GT 650M
num_features: 3827
num_elements: 3827
End features scope.
End program.

@9prady9
Copy link
Member

9prady9 commented Apr 22, 2016

good catch 👍 thank you.

@@ -79,7 +79,7 @@ macro_rules! feat_func_def {
let mut temp: i64 = 0;
let err_val = $ffi_name(&mut temp as MutAfArray, self.feat as Feat);
match err_val {
0 => Ok(Array::from(temp)),
0 => Ok(Array::from_retained(temp)),
Copy link
Member

@9prady9 9prady9 Apr 22, 2016

Choose a reason for hiding this comment

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

@mmetcalfe
I don't think we need a new member function for this unless there are more use cases to it. Probably doing just Ok(Array::from(temp).clone()) is enough. Can you please check it and change accordingly.


let temp_array = Array::from(temp);
let retained = temp_array.clone();
unsafe { mem::forget(temp_array); }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just changed the implementation based on your suggestion.
Using mem::forget here feels like a bit of a hack, but doing it this way doesn't require touching other files.

@9prady9 9prady9 self-assigned this Apr 23, 2016
@9prady9 9prady9 added this to the 3.3.0 milestone Apr 23, 2016
@9prady9 9prady9 added the Bug label Apr 23, 2016
@9prady9 9prady9 merged commit 755544e into arrayfire:devel Apr 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants