memchr signature invites mutability bugs

[RalfJung]

pub fn memchr(cx: *const c_void, c: c_int, n: size_t) -> *mut c_void;

The return pointer is derived from the argument pointer cx. If cx is created from a shared reference, that means it is UB to write to the return pointer. Casting const ptr to mut ptr has been involved in a bunch of UB cases, so having that done implicitly by the libc seems pretty bad.

If libc 1.0 APIs can still be changed, I'd strongly recommend using *const for the return type.

[dybucc]

shouldn't that just be the user's responsibility as part of the implicit contract of using unsafe c bindings? much like a rust native crate annotates unsafe functions with documentation on its safety {pre,post}conditions, we assume users read through the manpages and understand the input to be something that can't be aliased non-exclusively and passed across an unsafe boundary.

it seems to me like that type of safety guarantees could be best left to some higher-level crate.

[RalfJung]

One way in which Rust differs from C is that we're not happy with "you just have to be careful" as a safety strategy.

[dybucc]

except this crate is more of a c in rust than a rustified c. if a user requires functionality that happens to only be available in the form of ffi bindings, then they really are on their own. i don't think rust (or any non-c language) is out on a mission to force safety into these possibly non-rust unsound interfaces.

there's a reason why these bindings are marked unsafe and have no documentation. rust may provide higher-level abstractions that stack on top of these interfaces to ensure safety but it should at no point make the c bindings themselves any different than they currently are in whatever standards they're specified in.

considering this is a -sys crate, we're sort of the very base of everything. indeed, replacing foundational aspects can really make for great safety guarantees even on a higher-level setting, but when the provided bindings are to c library functions, that's like saying you'd like to replace all "foundational" c out there with rust. we're not at that point yet (if we ever will be.)

[RalfJung]

None of this is an argument for deliberately losing mutability information in the signature.

[dybucc]

the mutability requirement is part of the signature, even if it doesn't have any abi relevance. has there been any precedent to this type of change?

[RalfJung]

I'm not sure what you mean by "mutability requirement".

The fundamental cause here is a mismatch between *const T in Rust and const T* in C: in Rust, a const pointer is typically a pointer that you cannot write to, never, not even after casting it to *mut T. That's how &T works and that's typically what const in raw pointers hints at.

memchr however just wants to indicate that this function will not write to the pointer -- but it will cast the pointer to *mut and return that and it's fine for the caller to write to it.

[dybucc]

concerning my poor wording; i spoke of mutability requirement to refer to the
need to reproduce the function signature as in c. i myself have made
considerably more minor changes to the libc crate (like changing a u16 for a
c_ushort just to better match upstream.)

concerning the c and rust mismatch; i understand now better what you mean and
have read up on the function's description in draft N3886. in section 7.28.5,
paragraphs 1 and 2, there seems to be mention of these functions both accepting
and returning pointers to an arbitrarily-qualified byte array. iiuc, the
routine returns a non-const qualified pointer to truly be "generic" over the
qualification requirements detailed in paragraph 1.

if that's the case, then, again, i think we should keep this as is. we neither
have the possibility in rust of encoding those semantics; namely, returning a
*const if provided a *const, and returning a *mut if provided a *mut
pointer.

[RalfJung]

the
routine returns a non-const qualified pointer to truly be "generic" over the
qualification requirements

Yes. And then Rust strangely instantiates that with const in the argument position and mut in return position. That does not seem coherent to me.

In previous versions of the standard, the function was declared as void *memchr(size_t n; const void s[n], int c, size_t n); which probably explains the libc signature, but IMO this is a case where the signature is so footgunny that it is worth diverging from the declaration (especially given that it's not the declaration used by the standard any more).

[dybucc]

tl;dr: i think you're completely right and i should have started by reading the
spec. there's a few things i'd like to note.

In previous versions of the standard, the function was declared as void *memchr(size_t n; const void s[n], int c, size_t n); which probably explains the libc signature, but IMO this is a case where the signature is so footgunny that it is worth diverging from the declaration (especially given that it's not the declaration used by the standard any more).

the declaration included in the C standard seems to have remained the same
since C89/90 all the way through C17 ^{1 2 3 4}. the declaration you
mention above is effectively equivalent. in standards prior to C23, no
explanation was provided about the type qualification of the input byte array.

in C23, some routines like this one, where the input byte array's qualification
was to match the output pointer, started to be specified (note the emphasis)
with Q-prepended pointers to plain old data types. these types aren't special
in any way, but are rather meant to convey that the type qualifier of the
output pointer's pointee will be the same as that of the input pointer's
pointee. though the spec doesn't mention how would an implementation achieve
this.

i've looked into glibc's implementation and they use function overloading in
C++ and a mix of an input pointer to a const-qualified void and an output
pointer to a non-const-qualified void (which our Rust bindings mirror.) i'm
assumming then that C programmers are meant to know about those semantics
implicitly.

the C++ solution seems ideal but i'm not sure what the simplest solution would
look like in Rust. simply changing the signature to make both the input and
output pointers be *const seems reasonable to me now as well, considering
*const can act as sort of a "subtype" of *mut in this instance. there's no
unsound scenario if *const is used all throughout the signature; maybe a
better solution goes through one of the following (from simplest to possibly
more costly to add:)

1. adding documentation to the function summarizing the explanation in the C
   spec. this would follow changing the signature to only use *const.
2. having two functions in our exported bindings. one would be part of the ffi
   bindings reflecting whatever signature the libc implementation we're binding
   to in a given module uses, while the other would be renamed to something
   like memchr2 and use the "other" signature (of course, without C external
   linkage.)
3. having a different type be used for these pointers to pseudo-qualified types
   (QVoid, QChar.) these types could be repr(transparent) types generic over a
   zst marker type that would indicate whether an inner union field was a
   *const or a *mut. the wrapper type would paired with a wrapper
   (Rust-native) free function and a pair of traits. the traits would serve to
   have blanket From and Into implementations for raw pointers (which would
   otherwise conflict with the Into impl provided a From impl.) then the user
   would call into a trait method similarly named to Into::into, on both the
   input pointers to be converted to our wrapper type and the output wrapper
   type to be converted back to raw pointers. this would all go into and come
   out of the wrapper (non-FFI) free function with the same name.

whichever way we go, this is not the only routine that would need having its
signature changed. other search functions in section 7.26.5 of draft N3220 also
need tweaking to reflect the C23 updated semantics. prior standard specs didn't
mention anything but the libc crate doesn't seem to differentiate between C
standard versions so this should be applied crate-wide.

let me know if i got anything wrong, and i'll start looking into this.

Footnotes

1. https://nvlpubs.nist.gov/nistpubs/Legacy/FIPS/fipspub160.pdf ↩

2. https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf ↩

3. https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf ↩

4. https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2310.pdf ↩

[RalfJung]

Yeah that sounds plausible - but we should probably hear from @tgross35.