Rustify DOMXPath::quote #13545

divinity76 · 2024-02-27T22:08:13Z

I am fairly certain DOMXPath::quote is bug-free due to the extensive testing done by @nielsdos , quote:

I've let it fuzz to search for crashes for a while now. It tried about 23 million cases and all seems good.
If anyone is interested, here's the fuzz harness: https://gist.github.com/nielsdos/44981a753808129b0222f3ef28429c69

That is impressive, and made me think, what if we take it 1 step further and re-write it in Rust, with it's memory safety properties?

For those not in the know, Rust is a memory-safe high-performance language.

The Linux Kernel and the Windows kernel and Mozilla Firefox has adopted Rust for it's performance and safety, and the Chromium browser developers is looking into doing the same for Chromium,

Maybe PHP can adopt it too? 😄

TimWolla · 2024-02-27T22:14:15Z

Maybe PHP can adopt it too? 😄

Adding a new build-time dependency most certainly requires an RFC, because it will exclude platforms where a rust compiler is not available or not officially supported.

nielsdos

I am a big fan of Rust, and have used it in my own projects.
Adopting this in PHP may be beneficial, but requires the right abstractions to be put in place, and it's not a call I can make on my own. Tim is right that the introduction of any other language into PHP needs an RFC.

ext/dom/xpath_rust.h

nielsdos · 2024-02-27T22:13:18Z

ext/dom/xpath_rust.rs

+    };
+
+    // Update length
+    unsafe {


You should at least be able to get rid of this unsafety here.
I suggest returning a ffi-compatible struct that contains both the string as raw parts, and the length.

nielsdos · 2024-02-27T22:14:02Z

ext/dom/xpath_rust.rs

+    }
+
+    // Convert Vec<u8> to *mut c_char
+    let c_str = CString::new(result).expect("CString::new failed");


Note that this would leak memory, as you're not freeing the (persistently-allocated) memory of the string at the call site. Using RETURN_STRINGL also makes an extra allocation + copy which is unfortunate.

quick question, would it be better to handle this more "smoothly" ? ie using match pattern.

Yeah, this can actually fail because the input may contain \0 bytes. So we must not panic by using expect. And this should probably use a Zend API to avoid the reallocation too, will be much cleaner in the end with the right access and abstractions to Zend APIs.

nielsdos · 2024-02-27T22:16:37Z

ext/dom/xpath_rust.rs

+
+#[no_mangle]
+pub extern "C" fn domxpath_quote_literal(input: *const c_char, len: *mut usize) -> *mut c_char {
+    let slice = unsafe { std::slice::from_raw_parts(input as *const u8, *len as usize) };


This should preferably contain a "safety requirement" comment, i.e. that it is expected that the caller does not hold any mutable references to the input. I know it's a bit trivial in this case, but when using unsafe I'd rather see it documented, and the caller must know about it too to avoid soundness issues.

TimWolla · 2024-02-27T22:16:18Z

ext/dom/xpath_rust.rs

+    let double_quote_absent = !slice.contains(&b'"');
+
+    let result = if single_quote_absent {
+        let mut res = Vec::with_capacity(slice.len() + 2);


Without additional infrastructure this bypasses PHP's memory manager and thus PHP's memory limit.

Good point. I wonder how difficult it is to give rust access to php's memory manager 🤔

The easiest way I see is to implement GlobalAlloc to use emalloc/efree.

I can submit a separate PR for this, and possibly an RFC, if anyone's interested.

Using GlobalAlloc makes sense, request-based allocations are the most common case anyway.
If you go for an RFC then we'd need more than just the allocation interface, but we also need proper access to the Zend data structures. So this is more work than we might expect at first, but my personal opinion is that it's a good idea to allow memory safe languages inside PHP.

@danog please do. I won't have time/energy to contribute for some time.

TimWolla · 2024-02-27T22:17:10Z

ext/dom/xpath_rust.h

@@ -0,0 +1 @@
+extern char* domxpath_quote_literal(const char *const input, uintptr_t *const len);


The choice of uintptr_t looks odd for a size.

yeah, that's adopted from rust's usize, quote

Primitive Type usize: The pointer-sized unsigned integer type.

php-src's uintptr_t should be equivalent to rust's usize

php-src's uintptr_t should be equivalent to rust's usize

Nope. size_t is the type to use for sizes.

Okay, apparently Rust's usize maps to uintptr_t indeed, but this isn't really accurate from a C perspective 🤔 I expect both types to be the same on modern platforms, but it bothers me.

oh there actually is a core::ffi::c_size_t in Rust-nightly, but it's not released/stable yet, quoting https://doc.rust-lang.org/core/ffi/type.c_size_t.html :

🔬This is a nightly-only experimental API. (c_size_t #88345)

I would assume we'd depend on libc at some point anyway, so libc::size_t can probably be used.

TimWolla · 2024-02-27T22:19:02Z

ext/dom/xpath.c

+	const char *ouput = domxpath_quote_literal(input, &input_len);
+	RETURN_STRINGL(ouput, input_len);


Typo: ouput. This construct will also require a full allocation + copy into a new zend_string().

divinity76 · 2024-02-27T22:26:24Z

RFC

I know :) but I had some questions, like

how difficult is it to add .rs / rust files to the build system? buildconf/configure/make all that stuff
how difficult is it to pass strings to/from rust/c?

and this is a cool, functional proof of concept :) (at least it works locally on my machine. The CI's need rustc)

nielsdos · 2024-02-27T22:28:25Z

how difficult is it to add .rs / rust files to the build system? buildconf/configure/make all that stuff

For Makefiles, it's just a matter of invoking rustc. For buildconf and configure I'm not too sure, better ask the build system gurus :-)

how difficult is it to pass strings to/from rust/c?

Ideally, we'd be able to use zend_string both inside Rust and C.
Do you know about this? https://github.com/davidcole1340/ext-php-rs I haven't used it personally but it might be interesting to look at. They expose the Zend types.

divinity76 · 2024-02-27T22:53:54Z

Ideally, we'd be able to use zend_string both inside Rust and C.

Yeah that would be ideal

Do you know about this? https://github.com/davidcole1340/ext-php-rs I haven't used it personally but it might be interesting to look at. They expose the Zend types.

Nope! Neat! Wonder if i could get those guys' input here, ill try contacting them

Crell · 2024-02-28T15:37:58Z

Adding a new build-time dependency most certainly requires an RFC, because it will exclude platforms where a rust compiler is not available or not officially supported.

Are there Rust-unsupported platforms that PHP for-reals supports? (Not just claimed to 20 years ago and forgot to check.)

This would definitely require an RFC as it's a big policy change, though I'd be supportive, depending on the specifics.

TimWolla · 2024-02-28T16:03:42Z

Are there Rust-unsupported platforms that PHP for-reals supports? (Not just claimed to 20 years ago and forgot to check.)

As per https://doc.rust-lang.org/nightly/rustc/platform-support.html:

OpenBSD is in Tier 3 support for Rust, which means:

Tier 3 targets are those which the Rust codebase has support for, but which the Rust project does not build or test automatically, so they may or may not work.

Solaris / illumos is in Tier 2 without Host Tools which means:

Automated tests are not always run so it's not guaranteed to produce a working build, but tier 2 targets often work to quite a good degree and patches are always welcome!

I'm fairly confident that PHP works fine on both and I expect both of them to be in production use.

rustify DOMXPath::quote

98f1d26

TimWolla added the Status: Requires RFC label Feb 27, 2024

github-actions bot added Category: Build System Extension: dom labels Feb 27, 2024

nielsdos reviewed Feb 27, 2024

View reviewed changes

TimWolla reviewed Feb 27, 2024

View reviewed changes

review feedback

a30c1a4

divinity76 mentioned this pull request Feb 27, 2024

How to expose PHP's memory manager in Rust? davidcole1340/ext-php-rs#312

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rustify DOMXPath::quote #13545

Rustify DOMXPath::quote #13545

divinity76 commented Feb 27, 2024 •

edited

Loading

TimWolla commented Feb 27, 2024

nielsdos left a comment

nielsdos Feb 27, 2024

nielsdos Feb 27, 2024

devnexen Feb 28, 2024

nielsdos Feb 28, 2024

nielsdos Feb 27, 2024

TimWolla Feb 27, 2024

divinity76 Feb 27, 2024

danog Feb 28, 2024

nielsdos Feb 29, 2024

divinity76 Mar 4, 2024

TimWolla Feb 27, 2024

divinity76 Feb 27, 2024

TimWolla Feb 27, 2024

TimWolla Feb 27, 2024

divinity76 Feb 27, 2024 •

edited

Loading

morrisonlevi Feb 28, 2024

TimWolla Feb 27, 2024

divinity76 commented Feb 27, 2024

nielsdos commented Feb 27, 2024

divinity76 commented Feb 27, 2024

Crell commented Feb 28, 2024

TimWolla commented Feb 28, 2024

		@@ -0,0 +1 @@
		extern char* domxpath_quote_literal(const char const input, uintptr_t const len);

		const char *ouput = domxpath_quote_literal(input, &input_len);
		RETURN_STRINGL(ouput, input_len);

Rustify DOMXPath::quote #13545

Are you sure you want to change the base?

Rustify DOMXPath::quote #13545

Conversation

divinity76 commented Feb 27, 2024 • edited Loading

TimWolla commented Feb 27, 2024

nielsdos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divinity76 Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divinity76 commented Feb 27, 2024

nielsdos commented Feb 27, 2024

divinity76 commented Feb 27, 2024

Crell commented Feb 28, 2024

TimWolla commented Feb 28, 2024

divinity76 commented Feb 27, 2024 •

edited

Loading

divinity76 Feb 27, 2024 •

edited

Loading