Bindings introduce UB for references

[Caellian]

I wasn't entirely sure how to use Request::header_map(), expecting something akin to C API due to Option<&mut T> argument, so I looked at the implementation both in cef-rs and cef.

Description

The way the function is currently implemented allows UB to occur from safe Rust by passing None as argument:

fn header_map(&self, header_map: Option<&mut CefStringMultimap>) {
  //...
  let arg_header_map = arg_header_map
    .map(|arg| arg.into_raw())
    .unwrap_or(std::ptr::null_mut());
  f(arg_self_, arg_header_map)
  //...
}

Here, in impl ImplRequest for Request, f can be invoked with a null pointer.

GetHeaderMap is defined as:

public virtual void GetHeaderMap( HeaderMap& headerMap )= 0;

A reference is required to be initialized to refer to a valid object or function: see reference initialization.

Source: C++ Reference

In CEF implementation this reference is directly assigned to.

So bindings effectively invoke *nullptr = headermap_ when None value is passed as argument which is UB (that results in segfault). This is impossible to achieve using the C++ API and reference is only degraded into a pointer in the C ABI exposed by bindgen.

Solution

header_map signature should be:

fn header_map(&self, header_map: &mut CefStringMultimap) {...}
// or better yet, given that argument will be replaced (deep copied into)
fn header_map(&self) -> CefStringMultimap {...}

Related Issues

I described in detail only this one issue I stumbled across.

This is UB every time an argument which is a reference declaration in the original C++ API is defaulted as null. All of those shouldn't be wrapped in Option in the first place because C++ API doesn't allow it. There's ~950 occurences/binding I found using regex: \.unwrap_or\(std::ptr::null(_mut)?\(\)\) (some are false positives)

[wravery]

Thanks! I need to review the C++ generator a bit to make sure the pattern holds, but it does sound like it could map that particular pattern to a return value. It's a little complicated since we generate the Rust from the C API and can't see the C++.

[wravery]

The specific pattern I think we can use is getter names (get_..., which is already detected and stripped) with a *mut parameter at the end. If it has that, then it might even be able to map void vs. int/bool return values into T or Option<T>.

[Caellian]

That won't cover all cases - e.g. ImplRequest::set does this with first two and last arguments. Which are:

• Rust: url: Option<&CefString>, method: Option<&CefString>, post_data: Option<&mut PostData>, header_map: Option<&mut CefStringMultimap>
• C++: const CefString& url, const CefString& method, CefRefPtr<CefPostData> postData, const HeaderMap& headerMap) (source)

So this counterexample invalidates all your assumptions - it's not just *mut in the end of functions with get_, it can be any pointer argument in any position of any member or non-member function. Heuristics won't work because bindgen code doesn't contain the necessary information to detect these properly.

After putting a bit more thought into this, it's really bindgen's job to wrap C++ references in NonNull, so I created a PR there. But NonNull doesn't differentiate between *mut and *const, so you won't be able to infer mutability of arguments from that.

[csmoe]

Did you really encounter any crash or UB while using those apis in real code, or just figuring out how cef-rs works?

cef already verified those pointers in ctocpp/cpptoc translation like request_ctocpp.cc and request_cpptoc.cc.

The conversions are removed from cef's repo months ago, and generated in releasing stage, you can checkout them in the older versions https://github.com/chromiumembedded/cef/blob/4324/libcef_dll/cpptoc/request_cpptoc.cc, or the blob from export-cef-dirk's downloads.

Your bingen pr won't help here since the rust code is generated with cef's capi, not cpp. There is no reference in c as you know, cef-rs doesn't need to care about this.

I have been using those apis massively in https://github.com/csmoe/iced-webview, everything works fine.

[Caellian]

Did you really encounter any crash or UB while using those apis in real code, or just figuring out how cef-rs works?

I was figuring out how cef-rs works and found this by going though cef and cef-rs sources. I did actually attempt calling with None now and it doesn't crash as it should - my guess is because some internal wrapper deals with null values already.

But yeah, request.header_map(None) is not only not idiomatic - there's no reason to allow calling it. It doesn't crash, but it should really be RequestImpl::header_map(&self) -> CefStringMultimap

So to track back...

The specific pattern I think we can use is getter names (get_..., which is already detected and stripped) with a *mut parameter at the end. If it has that, then it might even be able to map void vs. int/bool return values into T or Option.

I think this could work for getters. There's still a lot of code that would benefit from update-bindings using clang-sys on C++ headers.

While bindgen can't translate STL types, bare-bones clang should be able to scavenge C++ AST for extra metadata that will surely prove useful for other applications besides just this one.

Anyway, closing this as those ~950 potential UBs are handled by some glue code in CEF somewhere.

[csmoe]

I think this could work for getters. There's still a lot of code that would benefit from update-bindings using clang-sys on C++ headers.
While bindgen can't translate STL types, bare-bones clang should be able to scavenge C++ AST for extra metadata that will surely prove useful for other applications besides just this one.

As @wravery and I mentioned, bindings are from c-api, there's nothing to do with cpp at all, the "UB" you claimed is caused by mismatching between c ptr and cpp ref, every c-api for a cpp library have to deal with it inside the wrapper or leave it to the c-api users, cef did the former.

cef-rs is talking to cef c-api, it doesn't care about cpp ref, so there's no UB.

[wravery]

Yes, and unfortunately, the directives which inform the translator script in the comments are stripped from the C API headers.

That being said, if there are patterns like this that are reliable enough to infer a better Rust API, I'm all for it. I'll also keep thinking about whether we could somehow extract more information from the original C++ headers. Perhaps we could work out an export format directly from the translator script within the CEF project.