The signature of "execv" | "execve" | "execvp" | "fexecve" is incorrect #1272
Comments
This cleans up the build.rs of `libc-test` for apple targets. I wanted to update the docker containers of some targets so that we can start testing newer currently-skipped APIs properly, but it is impossible to figure out which headers and APIs are skipped for each target. This PR separates the testing of apple targets into its own self-contained function. This allows seeing exactly which headers are included, and which items are skipped. A lot of work will be required to separate the testing of all major platforms and make the script reasonable. During the clean up, I discovered that, at least for apple targets, deprecated but not removed APIs are not tested. I re-enabled testing for those, and fixed `daemon`, which was not properly linking its symbol. I also added the `#[deprecated]` attribute to the `#[deprecated]` APIs of the apple targets. The attribute is available since Rust 1.9.0 and the min. Rust version we support is Rust 1.13.0. Many other APIs are also currently not tested "because they are weird" which I interpret as "the test failed for an unknown reason", as a consequence: * the signatures of execv, execve, and execvp are incorrect (see rust-lang#1272) * the `sig_t` type is called `sighandler_t` in libc for some reason: rust-lang#1273 This probably explains why some other things, like the `sa_handler`/`sa_sigaction` fields of `sigaction` were skipped. The field is actually a union, which can be either a `sig_t` for the `sa_handler` field, or some other type for the `sa_sigaction` field, but because the distinction was not made, the field was not checked. The latest ctest version can check volatile pointers, so a couple of skipped tests are now tested using this feature.
This cleans up the build.rs of `libc-test` for apple targets. I wanted to update the docker containers of some targets so that we can start testing newer currently-skipped APIs properly, but it is impossible to figure out which headers and APIs are skipped for each target. This PR separates the testing of apple targets into its own self-contained function. This allows seeing exactly which headers are included, and which items are skipped. A lot of work will be required to separate the testing of all major platforms and make the script reasonable. During the clean up, I discovered that, at least for apple targets, deprecated but not removed APIs are not tested. I re-enabled testing for those, and fixed `daemon`, which was not properly linking its symbol. I also added the `#[deprecated]` attribute to the `#[deprecated]` APIs of the apple targets. The attribute is available since Rust 1.9.0 and the min. Rust version we support is Rust 1.13.0. Many other APIs are also currently not tested "because they are weird" which I interpret as "the test failed for an unknown reason", as a consequence: * the signatures of execv, execve, and execvp are incorrect (see rust-lang#1272) * the `sig_t` type is called `sighandler_t` in libc for some reason: rust-lang#1273 This probably explains why some other things, like the `sa_handler`/`sa_sigaction` fields of `sigaction` were skipped. The field is actually a union, which can be either a `sig_t` for the `sa_handler` field, or some other type for the `sa_sigaction` field, but because the distinction was not made, the field was not checked. The latest ctest version can check volatile pointers, so a couple of skipped tests are now tested using this feature.
Clean libc-test for apple targets This cleans up the build.rs of `libc-test` for apple targets. I wanted to update the docker containers of some targets so that we can start testing newer currently-skipped APIs properly, but it is impossible to figure out which headers and APIs are skipped for each target, which has to change if we update the glibc version in one Linux container but not the other (updating them all at once is just madness). This PR separates the testing of apple targets into its own self-contained function. This allows seeing exactly which headers are included, and which items are skipped. A lot of work will be required to separate the testing of all major platforms and make the script reasonable. During the clean up, I discovered that, at least for apple targets, deprecated but not removed APIs are not tested. I re-enabled testing for those, and fixed `daemon`, which was not properly linking its symbol. I also added the `#[deprecated]` attribute to the `#[deprecated]` APIs of the apple targets. The attribute is available since Rust 1.9.0 and the min. Rust version we support is Rust 1.13.0. Many other APIs are also currently not tested "because they are weird" which I interpret as "the test failed for an unknown reason", as a consequence: * the signatures of execv, execve, and execvp are incorrect (see #1272) * the `sig_t` type is called `sighandler_t` in libc for some reason: #1273 This probably explains why some other things, like the `sa_handler`/`sa_sigaction` fields of `sigaction` were skipped. The field is actually a union, which can be either a `sig_t` for the `sa_handler` field, or some other type for the `sa_sigaction` field, but because the distinction was not made, the field was not checked. The latest ctest version can check volatile pointers, so a couple of skipped tests are now tested using this feature.
|
@asomers it would be better to start fixing these in freebsd @semarie it would be good to know if their signature is also incorrect in openbsd and netbsd (e.g. by trying to remove their "skip" in I'll try to roll in a fix for Android. After that is done, we can fix these on Linux, and do an update upstream, crater run, etc. |
|
Why do you think it would be best to start with FreeBSD? It seems that the same problem exists on Linux. |
|
We could try rolling this change for all targets at once as well. These APIs are available on all tier-1 targets, so I just thought that it might be "safer" to try to roll the change for less used targets first, and if there aren't any problems, only then attempt that for the tier-1 targets. AFAICT libstd does not use these APIs, so it is unclear to me how many people are using them (is anybody using them at all?) and how (e.g. whether changing the mutability of that pointer would be a breaking change for them or not). |
|
Well, Nix uses https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html |
|
Doesn't the "except as a consequence of replacing the process image." imply that those can actually be modified ? (EDIT: I thought replacing the process image was the main point of calling these functions). |
|
I interpret that as meaning that they may be modified in the child's image, but not in the parent's image. The constness of the pointer only matters from the perspective of the parent. |
|
Makes sense. The question still is, why should we do something different than C here ? If the C headers have a bug, then that would be a good enough reason, but I am not sure if that's the case here. |
|
Regarding openbsd, the signature is incorrect. correct definitions for openbsd are:
|
|
@semarie thanks, yes that is the signature that POSIX prescribes so basically all targets right now have this "issue". ABI-wise, a pointer is a pointer, and whether it is |
|
If we were writing libc from scratch, then we should probably just copy the C headers and require users to deal with the wrong constness. However, "fixing" libc at this point would break released versions of crates like Nix. I don't think the payoff is worth the pain. |
|
I tend to agree. Do you think that if we ever do a new breaking release of libc, we should probably fix this ? We probably have to that at some point anyways, to be able to properly use |
|
I suppose it wouldn't hurt if libc were breaking many more things at the same time. But it would have to be a major version bump. |
|
DragonFly BSD implemented While attempting to duplicate the FreeBSD definition, I stumbled across this issue. What's the desired signature for new Unix implementations of Is it preferable to match the C definition, or the existing rust definitions? |
|
I tend to think that you should stick with the current wrong, but really right, rust definition. That will certainly cause less pain for Rust users, at least. |
JohnTitor
changed the title
Add fexecve() to DragonFly DragonFly 6.0 added support for `fexecve(2)`. Implementing it with a mismatched signature from what C exposes, as outlined in #1272, for consistency with other platforms. Tested with nix-rust/nix#1577
|
Same issue applies to posix_spawn*. As far as I see exec* are fixed on x86_64-unknown-linux-gnu, but posix_spawn* are not |
Yes, this makes the prototypes harder to use. And less intuitive. But this makes them match headers, and thus now we can properly test them. This fixes rust-lang#1272
Yes, this makes the prototypes harder to use. And less intuitive. But this makes them match headers, and thus now we can properly test them. This fixes rust-lang#1272
Yes, this makes the prototypes harder to use. And less intuitive. But this makes them match headers, and thus now we can properly test them. This fixes rust-lang#1272
Yes, this makes the prototypes harder to use. And less intuitive. But this makes them match headers, and thus now we can properly test them. This fixes rust-lang#1272 Also we fix return types for some Windows exec* functions
Yes, this makes the prototypes harder to use. And less intuitive. But this makes them match headers, and thus now we can properly test them. This fixes rust-lang#1272 Also we fix return types for some Windows exec* functions
The type of
argvis*const *const c_charbut should be*const *mut c_char. With the current incorrect signatures, libc-test fails for these APIs, but this does not surface because these tests are skipped. With the fixed signatures, all tests pass.AFAICT, this affects all unix targets.
The text was updated successfully, but these errors were encountered: