ada-url / rust

Rust bindings for Ada URL parser
https://crates.io/crates/ada-url
Apache License 2.0
86 stars 12 forks source link

Does Url::origin leak? #63

Closed pratikpc closed 6 months ago

pratikpc commented 7 months ago

In Url origin, ffi::ada_get_origin is called

Within the C++ code, ada_get_origin allocates an ada_owned_string using new[]

It is supposed to be freed using ada_free_owned_string.

However, the call to the function seems to be missing here.

anonrig commented 7 months ago

I think it leaks yes. Can you open a PR?

pratikpc commented 7 months ago

@anonrig sure but all changes required to fix this are potentially breaking changes (at least the ones I can think of) because they would require a breaking change of the return type

  1. String.
    This would probably require creating a copy of the slice and then returning it and dropping the ada_owned_string
  2. Returning ada_owned_string directly
    To ensure safe management of memory, possibly implement a drop function in ada_owned_string because it would be the RAII way.
    But that would be a breaking change for the user space as well because ada_owned_string is not dropped.

Both String and ada_owned_string are already convertible to str with as_str

Which approach would you prefer?

anonrig commented 7 months ago

We can store the reference to ada_owned_string in URL and still use RAII, without changing the API of course.

pratikpc commented 6 months ago

Hi

Would this approach involve storing the ada_owned_string returned by ffi::ada_get_origin?

If so, we would have to either use:-

  1. Vector
  2. Option

In case a user can mutate the origin, so multiple calls are capable of returning different values, then in this case Vector is the best approach.

Otherwise, we could use Option as only a single value would be needed.

The approach while it would work and be leak free does sound a bit weird though no? Because the scope of the memory allocated has increased to cover the entire scope of the Url object which could cause unexpected hikes of memory consumption in user code.

Or am I a bit confused?

anonrig commented 6 months ago

I'm fine with both options. I can release a new major