Boxed AnyBox::Native potentially causes memory leaks

[sdroege]

See here what I'm talking about.

This variant is used whenever T::uninitialized() is called, which gir does automatically for boxed types if there's an out parameter that is caller allocates.

If such a boxed struct is then dropped, nothing will happen according to the code. So if the FFI struct contains pointers that have to be freed, for example, then we would be leaking them.

I didn't check any code yet if we already have such a case, but it seems likely and is obviously wrong.

I'm not sure yet how to implement the same thing correctly.

[sdroege]

Yes, gst_video::VideoTimeCode if it was not manually implemented for other reasons anyway, glib::String, glib::Error, glib::ValueArray.

[EPashkin]

@sdroege You sure that any boxed type contains pointer? gst_video::VideoTimeCode IMHO not

[EPashkin]

As I know free only freed allocated memory, out parameters usually filled in stack and I don't see way in glib to "free" it values

[sdroege]

@sdroege You sure that any boxed type contains pointer? gst_video::VideoTimeCode IMHO not

See here, which is defined here and has a pointer in there. That pointer is owned and needs to be freed later.

Usually for such types there's an init() and clear() or reset() or unset() function.

As I know free only freed allocated memory, out parameters usually filled in stack

Search for API that has an out parameter that is annotated as caller allocates. glib::Error is not such a case (callee allocates), but glib::Value is for example (exactly same problem btw but we don't have leaks because we manually implement it and the correct Drop impl).

Things like the various rectangle types we have would be other cases (but don't contain pointers so can just be dropped as-is), and graphene has a lot of types like this.

This code in src/writer/to_code.rs would be triggered for such cases before calling the function.

            UninitializedNamed { ref name } => {
                let s = format!("{}::uninitialized()", name);
                vec![s]
            }

and I don't see way in glib to "free" it values

I think the solution for this would be to detect init, reset/unset/clear functions for these types in gir and then add them as anoter variant to the glib_wrapper! macro so that we can create a correct Drop impl and also a correct ::uninitialized() definition.

[EPashkin]

Strange that GstVideoTimeCodeConfig don't contains any function,
only GstVideoTimeCode has clear.
Maybe we needed add/use ZeroedNamed for these types, to be sure that pointers is NULL by default.
Agree that we need change glib_wrapper! to support clear or free_fields,
but instead detecting these function IMHO better just add config for name.

[sdroege]

Maybe we needed add/use ZeroedNamed for these types, to be sure that pointers is NULL by default.

What do you mean with ZeroedNamed?

Strange that GstVideoTimeCodeConfig don't contains any function, only GstVideoTimeCode has clear.

That's just a bit weird API, we can ignore that for the bindings. This specific type can never be autogenerated anyway :)

---

Agree that we need change glib_wrapper! to support clear or free_fields,
but instead detecting these function IMHO better just add config for name.

Ok, or that :) The remaining question is then what to do about types like glib::String where the ::uninitialized() is simply wrong.

[EPashkin]

What do you mean with ZeroedNamed?

I thought about same as UninitializedNamed but generate mem::zeroed,
but seems this better do in Uninitalized implementation for object,
so code generation only changed in generating Boxed<...> part.
This can also solve problem with glib::String.

[sdroege]

This can also solve problem with glib::String.

The problem with glib::String is not the initialization, but that if you allocate it via the Box<_> case via ::uninitialized() then freeing it later will leak memory as there simply is no clear function. glib::String must not be allocated by anything else than GLib.

[EPashkin]

Oh, now I understand what you meant about glib::String,
I has no idea what we can do,
with exception don't allow use this type as output parameter 😉

[sdroege]

We could place a panic!() into the init() / clear() function for such types.

[sdroege]

with exception don't allow use this type as output parameter wink

It's not out parameters that are the problem per-se, but out parameters where the caller allocates.

I don't think we have an instance of these currently so would suggest to simply remove this code for boxed types until a need for it arrives. And then we can think of a solution with the concrete case at hand.

[sdroege]

Good, so it's actually used currently. For gtk::TreeIter, gtk::Border and gtk::TextIter. Those all need no special cleanup functions so the previous code was fine for them.

Now the question is how to distinguish these cases automatically. What we have are the following variants

1. No clear/init functions but storing no pointers or anything else that needs to be freed (the cases above)
2. Clear/init functions (various things in GLib, GStreamer, etc.)
3. No clear/init functions but storing pointers or other things that need to be freed (GString and various other boxed types)

1. and 3. can't be distinguished automatically. So my proposed solution would be

1. init/clear functions in the glib_wrapper macro, and only if they are defined we allow for the case where we allocate memory
2. gir autodetects those functions and makes use of them
3. gir configuration to opt-in (!) for types without them, in which case it would define the init/clear function to be an empty closure

Opinions?

[sdroege]

CC @GuillaumeGomez @EPashkin :)

[GuillaumeGomez]

The solution 2. would be the best because it'd allow to finally generate some types which don't provide memory management functions. However I have no idea how difficult it'd be to add it... Otherwise the solution 1.

[sdroege]

I only proposed one solution :) And that does not cause any new types to be generated. It also does not fix any bugs in existing types we generate.

[EPashkin]

Agree that we need add init clear functions to BoxedMemoryManager<T>
and implement implement them in glib_wrapper!

[sdroege]

See #496 for the first part.

As you can see, even for GDate some manual code is needed to do something with these functions. As such, I'd suggest to not actually try to autodetect them (most work different in slightly different ways...) but to require the user to configure expressions for them. Opinions?

init_expression = "|ptr| foo_sys::foo_bar_init(ptr)"
clear_expression = "|ptr| foo_sys::foo_bar_clear(ptr)"

[EPashkin]

Agree with only configured init_expression.

[sdroege]

Going with that then, thanks :)

[GuillaumeGomez]

I'm not a big fan of the current syntax. I'd prefer to provide a given set of instructions instead of providing strings like that.

EDIT: And I didn't comment at the good place I guess since it was on gir that this new syntax was added...

[sdroege]

I'm not a big fan of the current syntax. I'd prefer to provide a given set of instructions instead of providing strings like that.

I can't think of a nicer syntax that covers all cases. In one way or another you need the ability to provide arbitrary Rust code here as the differences in behaviour are too big.