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

Do not use Vec<u8> as argument type in the binding #3925

Closed
vxgmichel opened this issue Jan 18, 2023 · 0 comments · Fixed by #3934
Closed

Do not use Vec<u8> as argument type in the binding #3925

vxgmichel opened this issue Jan 18, 2023 · 0 comments · Fixed by #3934
Labels
C-Q&A Category: Quality & Assurance issue I-Rust Impact: Rust-related stuff

Comments

@vxgmichel
Copy link
Contributor

vxgmichel commented Jan 18, 2023

See the corresponding issue on pyo3 for more information: PyO3/pyo3#2888

Instead we should take a PyObject as argument and perform an explicit conversion:

fn extract_to_vec_u8(object: PyObject, py: Python) -> PyResult<Vec<u8>> {
    Ok(match object.extract::<&PyByteArray>(py) {
        Ok(x) => x.to_vec(),
        Err(_) => object.extract::<&PyBytes>(py)?.as_bytes().to_vec(),
    })
}

Note: this performs a copy than can usually be avoided but it requires an unsafe block in the case of a bytearray. There's already a couple of such blocks in the codebase, e.g:

let bytes = match data.extract::<&PyByteArray>(py) {
// Using PyByteArray::as_bytes is safe as long as the corresponding memory is not modified.
// Here, the GIL is held during the entire access to `bytes` so there is no risk of another
// python thread modifying the bytearray behind our back.
Ok(x) => unsafe { x.as_bytes() },
Err(_) => data.extract::<&PyBytes>(py)?.as_bytes(),
};

@vxgmichel vxgmichel added I-Rust Impact: Rust-related stuff C-Q&A Category: Quality & Assurance issue labels Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Q&A Category: Quality & Assurance issue I-Rust Impact: Rust-related stuff
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant