Avoid calling slice::from_raw_parts with a null pointer

[saethlin]

slice::from_raw_parts requires that the pointer not be null, even if the length is zero. This is because the pointer is converted into a reference, and references must not be null. This is detected by the standard library debug assertions, which are compiled out in all released toolchains but you can use cargo careful if you want to enable them and some other nightly-only opt-in checks.

This is probably the third instance of this bug I've fixed, I think it's pretty common for C libraries to return a pointer and length, and it is very tempting to just convert that to a Rust slice. But unfortunately you need a null check.

I'm not really sure what to do about testing this. I feel like there should be CI to guard against making this mistake again, but I'm not sure how to jam cargo careful or cargo +nightly test -Zbuild-std into your xtask setup.

[messense]

I'm not sure how to jam cargo careful or cargo +nightly test -Zbuild-std into your xtask setup.

Opened #2688

[saethlin]

The PR template says to consider adding something to newsfragments. I considered it, and decided this PR is not newsworthy. Is the PR template supposed to be explaining this requirement?

[messense]

Maintainers can add CI-skip-changelog label to bypass that check if it's deemed not newsworthy.

[saethlin]

Sounds good. I can always drop the commit if a maintainer agrees with me.

[adamreichold]

The from_raw_parts call to create kwargs a bit further down is not affected as the pointer will never be null if kwnames isn't?

[davidhewitt]

I think UB / crashes are generally changelog-worthy.

Additional CI for soundness checks is always welcome. @adamreichold configured some for rust-numpy which might be a starting point.

However, I want to discuss, is this change really necessary? The # Safety documentation for extract_arguments_fastcall has this invariant required:

/// - `args` must be a pointer to a C-style array of valid `ffi::PyObject` pointers.

I wrote this implying that args must be non-null for the pointer to be valid. Two questions:

• Should the safety note be made more direct? e.g. "must be a non-null pointer"
• If !args.is_null() is an invariant of the function, does it really need to do a null check internally?

[mejrs]

However, I want to discuss, is this change really necessary?

Well, our test suite hits it, apparently. https://asan.saethlin.dev/ub?crate=pyo3&version=0.17.2

[adamreichold]

If !args.is_null() is an invariant of the function, does it really need to do a null check internally?

Well, our test suite hits it, apparently. https://asan.saethlin.dev/ub?crate=pyo3&version=0.17.2

I think that if the test suite hits it, then we do need a fix, but it might be that it should be higher up in the call chain.

[saethlin]

higher up in the call chain

Where? I'm a novice in this codebase, so I just don't see anywhere in the caller that makes sense to place this.

[saethlin]

Should the safety note be made more direct

To be clear, I simply didn't read this safety comment. I agree that the implementation is fine as-is and the caller should be adjusted.

[davidhewitt]

So after trawling through Python docs for a while, I finally found this line here:

args is a C array consisting of the positional arguments followed by the
values of the keyword arguments. This can be NULL if there are no arguments.

Which makes it clear to me that there is a design mismatch between what I'd implemented for extract_arguments_fastcall and what Python actually does - args can be null but only if nargs is 0.

So in my opinion:

• The safety note for extract_arguments_fastcall is incorrect and should be changed to match the invariant from Python that args may be null only in the case when nargs is 0.
• The implementation should be updated accordingly. The patch proposed here is fine, however I also wonder whether a larger patch which skips more code when nargs == 0 could lead to efficiency gains. Maybe that's for another PR, and @saethlin this is good to merge after correcting the safety note for extract_arguments_fastcall?

[saethlin]

Done.

[davidhewitt]

Thanks. I'm... not sure what's up with CI, will have to try to investigate that when I can find a moment.

[saethlin]

No worries. I figured you'd get to it when you have time 😄

[mejrs]

Thanks. I'm... not sure what's up with CI, will have to try to investigate that when I can find a moment.

Looks like mmastrac/rust-ctor#241

[davidhewitt]

Aha good find - that's since been fixed, and CI now passes, so let's merge!