1319163 - Eliminate unsafe code from rust-url-capi

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

As the current code already assumes that every `type rusturl_ptr = *const libc::c_void` is in fact a pointer to a Url object, it would be simpler to instead replace the function parameter types with `Option<&Url>` and `Option<&mut Url>`. This would also allow the functions to no longer be `unsafe`, as they would be fully safe to use from within rust (from C++ of course everything is unsafe).

Comment 1

[Nika Layzell [:nika] (ni? for response)]

(The free method would remain unsafe of course)

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Eliminate unsafe code from rust_url_capi

MozReview-Commit-ID: 5KyfPQCsRNA

Comment 3

[Manish Goregaokar [:manishearth]]

Comment on attachment 8814217 [details] [diff] [review]
Eliminate unsafe code from rust_url_capi

Review of attachment 8814217 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/rust-url-capi/src/rust-url-capi.h
@@ +15,3 @@
>  //   by rusturl_new, and must be freed with rusturl_free.
>  
> +// The `rusturl` opaque type is equivalent to the rust type `::url::Url`

Might want to mention that pointers to this are allowed to be null

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(In reply to Manish Goregaokar [:manishearth] from comment #3)
> Might want to mention that pointers to this are allowed to be null

The numbers are _safe_ if they are null, but they aren't exactly "allowed" to be null. All of these functions will return the InvalidArg error if they are passed a null rusturl pointer. It seems a bit silly to mention that they are "allowed to be null" given that.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8814217 [details] [diff] [review]
Eliminate unsafe code from rust_url_capi

Review of attachment 8814217 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following fixed.

::: netwerk/base/rust-url-capi/src/lib.rs
@@ +62,5 @@
>  }
>  
>  #[no_mangle]
> +pub extern "C" fn rusturl_get_spec(urlptr: Option<&Url>, cont: &mut nsACString) -> i32 {
> +  let url = if let Some(url) = urlptr { url } else {

Hard to read style. Please use:
let url = if let Some(url) = urlptr {
  url
} else {

Make it the same for all other methods too.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c6411235af9
Eliminate unsafe code from rust_url_capi, r=valentin

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c6411235af9