Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TRACK_VARS_REQUEST seems to not be stored in http_globals #331

Open
Xenira opened this issue Oct 21, 2024 · 12 comments
Open

TRACK_VARS_REQUEST seems to not be stored in http_globals #331

Xenira opened this issue Oct 21, 2024 · 12 comments
Assignees
Labels
bug Something isn't working

Comments

@Xenira
Copy link
Collaborator

Xenira commented Oct 21, 2024

Rust nightly build detects that the access in

self.http_globals[TRACK_VARS_REQUEST as usize]
results in a out of bounds.

Looking at the C code it seems that TRACK_VARS_REQUEST is not referenced anywhere.

Maybe someone with more knowledge about php internals can point towards where to find the $_REQUEST super global.

@Xenira Xenira added the bug Something isn't working label Oct 21, 2024
Xenira added a commit that referenced this issue Oct 21, 2024
BREAKING CHANGE: If you used `http_request_vars()` before it will now panic until a proper implementation is found.

Refs: #331
@ptondereau
Copy link
Collaborator

Very weird as this constant lives here: https://github.com/php/php-src/blob/master/main/php_globals.h#L46

@Xenira
Copy link
Collaborator Author

Xenira commented Oct 21, 2024

Ye, found the global both in the bindings file and in the zend code https://github.com/php/php-src/blob/50acf5eb15df4be623e01af7926688ce8fc7529b/main/php_globals.h#L46

But looking at the array size (https://github.com/php/php-src/blob/50acf5eb15df4be623e01af7926688ce8fc7529b/main/php_globals.h#L114) it is 6 and therefore out of bounds with index 6.

Searching for TRACK_VARS_REQUEST does not yield anything but the definition.

@ptondereau
Copy link
Collaborator

ptondereau commented Oct 21, 2024

Ok, but it seems that it's related to Rust according to this issue: rust-lang/rust#90534

@Xenira
Copy link
Collaborator Author

Xenira commented Oct 21, 2024

Sure, might be my limited c knowledge, but shouldn't the http_globals be constant length?

@ptondereau
Copy link
Collaborator

So as we know that the 6th index exists, we could tell the compiler to disable this warning on this specific line instead

@Xenira
Copy link
Collaborator Author

Xenira commented Oct 21, 2024

Just created a simple test:

#![cfg_attr(windows, feature(abi_vectorcall))]
use ext_php_rs::prelude::*;
use ext_php_rs::zend::ProcessGlobals;

#[php_function]
pub fn hello_world(name: &str) -> String {
    format!("Hello, {:?}!", ProcessGlobals::get().http_request_vars())
}

#[php_module]
pub fn get_module(module: ModuleBuilder) -> ModuleBuilder {
    module
}

Results in

thread '<unnamed>' panicked at /home/rippi/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ext-php-rs-0.12.0/src/zend/globals.rs:287:9:
index out of bounds: the len is 6 but the index is 6
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
fatal runtime error: failed to initiate panic, error 5
zsh: IOT instruction (core dumped)  php -d extension=./target/debug/libreq_super_global.so test.php

@ptondereau
Copy link
Collaborator

ptondereau commented Oct 21, 2024

Ok thank you for your test!
I've created an issue on php-src repo because IMHO, it's a bug from PHP here php/php-src#16541

Xenira added a commit that referenced this issue Oct 21, 2024
BREAKING CHANGE: If you used `http_request_vars()` before it will now panic until a proper implementation is found.

Refs: #331
Xenira added a commit that referenced this issue Oct 21, 2024
BREAKING CHANGE: If you used `http_request_vars()` before it will now panic until a proper implementation is found.

Refs: #331
ptondereau pushed a commit that referenced this issue Oct 21, 2024
BREAKING CHANGE: If you used `http_request_vars()` before it will now panic until a proper implementation is found.

Refs: #331
@Xenira Xenira self-assigned this Oct 21, 2024
@Xenira
Copy link
Collaborator Author

Xenira commented Oct 21, 2024

Managed to get somewhere. Still only getting a null value, but making progress.

@Xenira
Copy link
Collaborator Author

Xenira commented Oct 21, 2024

Hmmm, from what I can see this might not be initialised all the time and needs some priming.
Logging the symbol table I get

{
    "_GET": Zval {
        type: Array,
        val: Some(
            {},
        ),
    },
    "_POST": Zval {
        type: Array,
        val: Some(
            {},
        ),
    },
    "_COOKIE": Zval {
        type: Array,
        val: Some(
            {},
        ),
    },
    "_FILES": Zval {
        type: Array,
        val: Some(
            {},
        ),
    },
}

which does not include the _REQUEST entry, causing zend_hash_find_* to return a nullpointer.

Gonna do some more testing tomorrow. Pushed my experiments for reference.

@Xenira
Copy link
Collaborator Author

Xenira commented Oct 22, 2024

Ok, apparently $_REQUEST is only populated after calling zend_is_auto_global("_REQUEST") :/

@Xenira
Copy link
Collaborator Author

Xenira commented Oct 22, 2024

@ptondereau got a working solution. Can you have a look at #334, haven't done that much with zend directly.

Would still need to update the docsrs_bindings.rs but will do that once everything else is in place.

@Xenira
Copy link
Collaborator Author

Xenira commented Oct 22, 2024

And of course it works differently on php80...

Xenira added a commit that referenced this issue Oct 22, 2024
Xenira added a commit that referenced this issue Oct 22, 2024
Xenira added a commit that referenced this issue Oct 22, 2024
Xenira added a commit that referenced this issue Oct 22, 2024
Xenira added a commit that referenced this issue Oct 22, 2024
Xenira added a commit that referenced this issue Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants