impl AsObjectArg <T> makes T-object free

[egsam98]

Hello, recently I've found suspicious behaviour in Godot runtime while using new pattern AsObjectArg in arguments. Godot prints error that object is null on such calls.
For example, I have a function that prints letter textures into RichTextLabel:

fn print(to: &mut Gd<RichTextLabel>, text: &str, font: Font) {
    to.clear();
    let font_pathf = match font {
        Font::Large => font_large_pathf,
        Font::Street => font_street_pathf,
    };
    for char in text.as_bytes() {
        let path = font_pathf(char.to_ascii_uppercase());
        match try_load::<Texture2D>(path) {
            Ok(img) => to.add_image(img),
            Err(err) => godot_error!("print to {}: {}", to, err),
        };
    }
}

Godot logs errors in editor with a reference to this line: https://github.com/godotengine/godot/blob/15073afe3856abd2aa1622492fe50026c7d63dc1/scene/gui/rich_text_label.cpp#L3228
Similar issue was noticed in other functions PhysicsDirectSpaceState3D::like get_rest_info etc.

Here is two options to resolve the issue:

1. Clone a reference counter: to.add_image(img.clone())
2. Borrow: to.add_image(&img)

Is that behaviour expected?

[Bromeon]

No, that shouldn't happen.
You're saying that to.add_image(&img) instead of to.add_image(img) does not trigger the problem?

My current plan is to make all APIs accept references, for a few reasons:

• To address #790
• Consistency: all values that aren't Copy are passed by reference (Gd, Array, Dictionary, GString, Callable, ...)
• It doesn't suggest that passing by value is more efficient ("move"), because it's not

So this would mean removing the value impls for AsObjectArg, which would "solve" this. But yes, it shouldn't happen in the first place 🙂

[egsam98]

Correct, to.add_image(&img) does NOT trigger it. Same for to.add_image(img.clone())

[egsam98]

Another error occurred today from PhysicsDirectSpaceState3D::intersect_shape(params):
https://github.com/godotengine/godot/blob/15073afe3856abd2aa1622492fe50026c7d63dc1/servers/physics_server_3d.cpp#L404
The only safe way is to replace args everywhere to their borrowed analogue in whole project

[Bromeon]

Thanks!

I think I see the problem -- it happens only for methods with default parameters:

// impl RichTextLabel

pub fn add_image(&mut self, image: impl AsObjectArg<crate::classes::Texture2D>) {
    self.add_image_ex(image).done()
}

pub fn add_image_ex(
    &mut self,
    image: impl AsObjectArg<crate::classes::Texture2D>, // <- local variable
) -> ExAddImage<'_> {
    ExAddImage::new(self, image.as_object_arg())
} // <- drop `image`!

pub struct ExAddImage<'a> {
    surround_object: &'a mut re_export::RichTextLabel,
    image: ObjectArg<crate::classes::Texture2D>, // <- only stores pointer
    ...
}

impl< 'a > ExAddImage< 'a > {
    fn new(surround_object: &'a mut re_export::RichTextLabel, image: ObjectArg<Texture2D>) -> Self {
        Self { surround_object, image, ... }
    }
}

I.e. use-after-free error.

For now, I'd recommend changing the calls to take borrows. As mentioned above, we'd likely enforce this soon, which would also address this issue.