feat: allows ZendStr to contain null bytes

[ju1ius]

Closes #200

Rationale

In PHP zend_strings are binary strings with no encoding information. They can contain any byte at any position.
The current implementation use CString to transfer zend_strings between Rust and PHP, which prevents zend_strings containing null-bytes to roundtrip through the ffi layer. Moreover, ZendStr::new() accepts only a &str, which is incorrect since a zend_string is not required to be valid UTF8.

When reading the PHP source code, it is apparent that most functions marked with ZEND_API that accept a const *char are convenience wrappers that convert the const *char to a zend_string and delegate to another function. For example zend_throw_exception() takes a const *char message, and just converts it to a zend_string before delegating to zend_throw_exception_zstr().

I kept this PR focused around ZendStr and it's usages in the library, but it should be seen as the first step of a more global effort to remove usages of CString everywhere possible.

Also, I didn't change the return type of the string related methods of Zval (e.g. I could have made Zval::set_string()
accept an impl AsRef<[u8]> instead of &str and return () instead of Result<()>). If I get feedback that it should be done in this PR, I'll do it.

Summary of the changes:

ZendStr

• [BC break]: ZendStr::new() and ZendStr::new_interned() now accept an impl AsRef<[u8]> instead of just &str, and are therefore infaillible (outside of the cases where we panic, e.g. when allocation fails). This is a BC break, but it's impact shouldn't be huge (users will most likely just have to remove a bunch of ? or add a few Ok()).
• [BC break]: Conversely, ZendStr::as_c_str() now returns a Result<&CStr> since it can fail on strings containing null bytes.
• [BC break]: ZensStr::as_str() now returns a Result<&str> instead of an Option<&str> since we have to return an error in case of invalid UTF8.
• adds method ZendStr::as_bytes() to return the underlying byte slice.
• adds convenience methods ZendStr::as_ptr() and ZendStr::as_mut_ptr() to return raw pointers to the zend_string.

ZendStr conversion traits

• adds impl AsRef<[u8]> for ZendStr
• [BC break]: replaces impl TryFrom<String> for ZBox<ZendStr> by impl From<String> for ZBox<ZendStr>.
• [BC break]: replaces impl TryFrom<&str> for ZBox<ZendStr> by impl From<&str> for ZBox<ZendStr>.
• [BC break]: replaces impl From<&ZendStr> for &CStr by impl TryFrom<&ZendStr> for &CStr.

Error

• adds new enum member Error::InvalidUtf8 used when converting a ZendStr to String or &str

[ju1ius]

A few off-topic remarks:

1. since persistent zend_strings are almost never what you want, and there is a ZendStr::new_interned constructor, shouldn't we add a ZendStr::new_persistent() constructor and remove the persistent parameter in ZendStr::new() ?
2. we don't currently tackle the special cases: when the length of the string is either 0 or 1 like in zend_string_init_fast()
3. I've seen a few well-known frameworks reserving a memory buffer for their error handlers (like so). I guess the goal is for them to be able to run the handlers even when the allowed memory limit is reached. I don't know if this applies for extensions, but maybe panicking on failed allocations is not good citizenship?

WDYT?

[ju1ius]

@ptondereau I overlooked some of the impls, so this still need a bit of work.

[ptondereau]

1. since persistent zend_strings are almost never what you want, and there is a ZendStr::new_interned constructor, shouldn't we add a ZendStr::new_persistent() constructor and remove the persistent parameter in ZendStr::new() ?

IMHO, this is a code smell to bring a builder, but I agree that we can have a specific constructor for a not so used context.

2. we don't currently tackle the special cases: when the length of the string is either 0 or 1 like in zend_string_init_fast()

It should be done then.

3. I've seen a few well-known frameworks reserving a memory buffer for their error handlers (like so). I guess the goal is for them to be able to run the handlers even when the allowed memory limit is reached. I don't know if this applies for extensions, but maybe panicking on failed allocations is not good citizenship?

I guess PHP will crash before Rust have the allocation state and this leads to an unresolved error (that I need to create issue) that leak memory of Rust's allocated stuff. I would go for a wrapping with zend_try+zend_catch in our C wrapper, but maybe this isn't the best solution.

[ju1ius]

I'm wary that we might have a Schlemiel the Painter problem here.
If I remember correctly, there's a flag somewhere at the C-level to indicate that the string has already been utf8-validated (i.e. by the mbstring or pcre extensions), need to investigate...

[ju1ius]

Yep, it's here and also here.
So we could do something like:

if GC_FLAGS(self) & IS_STR_VALID_UTF8 {
   return std::str::from_utf8_unchecked(...);
}
let s = std::str::from_utf8(self.as_bytes()).map_err(|_| Error::InvalidUtf8)?;
GC_ADD_FLAG(self, IS_STR_VALID_UTF8):
return s;

WDYT?

[ju1ius]

This requires adding stuff to the ffi bindings though (the gc flags related stuff in zend_types.h).
I'm going to leave this off for now and do it in a separate PR.

[ptondereau]

Dors this require mbstring to be installed

[ju1ius]

Dors this require mbstring to be installed

No, the flag is used by mbstring and PCRE but it is defined in zend_types.h.

See this PR for historical context. 😉

[ptondereau]

IMHO, I think this is a requirement here because It can lead to the same perf regression as in the PHP issue.

[ju1ius]

Yes I think so too! I proposed to do this in another PR to keep this one focused (and maximize the chances to get it merged quickly), but if you think it should be done here I'm OK with that.
Do you confirm?

[ptondereau]

Yes, this was my intention. Sorry and thank you

[ju1ius]

It shall be done then! 😉

[ju1ius]

IMHO, this is a code smell to bring a builder, but I agree that we can have a specific constructor for a not so used context.

The idea wasn't to bring a builder, just to remove the persistent parameter and introduce a new constructor function:

impl ZendStr {
  pub fn new(value: impl AsRef<[u8]>) -> Self {}
  pub fn new_persistent(value: impl AsRef<[u8]>) -> Self {}
  pub fn new_interned(value: impl AsRef<[u8]>) -> Self {}
}

This is more a DX improvement. For example this would allow removing the persistent parameter from the conversion traits (FromZval et al.). Since it is only ever use for strings and even then rarely so, I find it cumbersome to have to specify it hen it is never needed (like true.into_zval(false)...).

But this is not needed for this PR, so let's keep things the way they are for now.

[ju1ius]

3. we don't currently tackle the special cases: when the length of the string is either 0 or 1 like in zend_string_init_fast()

It should be done then.

This requires adding symbols to the ffi bindings (see related constants in zend_string.h).
I'm going to leave this off for now and do it in a separate PR.

[ptondereau]

LGTM!