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

process_unix: prefer i32::*_be_bytes over manually shifting bytes #74271

Merged
merged 1 commit into from
Jul 14, 2020
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 12 additions & 14 deletions src/libstd/sys/unix/process/process_unix.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::convert::TryInto;
use crate::fmt;
use crate::io::{self, Error, ErrorKind};
use crate::ptr;
Expand All @@ -17,7 +18,7 @@ impl Command {
default: Stdio,
needs_stdin: bool,
) -> io::Result<(Process, StdioPipes)> {
const CLOEXEC_MSG_FOOTER: &[u8] = b"NOEX";
const CLOEXEC_MSG_FOOTER: [u8; 4] = *b"NOEX";

let envp = self.capture_env();

Expand Down Expand Up @@ -52,11 +53,12 @@ impl Command {
drop(input);
let Err(err) = self.do_exec(theirs, envp.as_ref());
let errno = err.raw_os_error().unwrap_or(libc::EINVAL) as u32;
let errno = errno.to_be_bytes();
let bytes = [
(errno >> 24) as u8,
(errno >> 16) as u8,
(errno >> 8) as u8,
(errno >> 0) as u8,
errno[0],
errno[1],
errno[2],
errno[3],
Comment on lines +56 to +61
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm... with this code, the errno is always encoded in big endian into the array. But before, it was always swapped -- even on big endian devices -- right? And I don't see any #[cfg(target_endianess = ...)] attributes anywhere. So your PR changes behavior, right? Which means either your change or the previous code is buggy? Is this code tested on big endian CPUs?

I unfortunately don't know a lot about these unix system APIs. Could you maybe link me some resource which specifies that this has to be big endian? Or is this just used internally? Like... the data is used below. But if it's just internally, why swap any bytes at all? Why not keep it in native endianess?

CLOEXEC_MSG_FOOTER[0],
CLOEXEC_MSG_FOOTER[1],
CLOEXEC_MSG_FOOTER[2],
Expand All @@ -81,12 +83,13 @@ impl Command {
match input.read(&mut bytes) {
Ok(0) => return Ok((p, ours)),
Ok(8) => {
let (errno, footer) = bytes.split_at(4);
assert!(
combine(CLOEXEC_MSG_FOOTER) == combine(&bytes[4..8]),
combine(CLOEXEC_MSG_FOOTER) == combine(footer.try_into().unwrap()),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, why we not just compare 2 slice instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, that would be better indeed.

"Validation on the CLOEXEC pipe failed: {:?}",
bytes
);
let errno = combine(&bytes[0..4]);
let errno = combine(errno.try_into().unwrap());
assert!(p.wait().is_ok(), "wait() should either return Ok or panic");
return Err(Error::from_raw_os_error(errno));
}
Expand All @@ -103,13 +106,8 @@ impl Command {
}
}

fn combine(arr: &[u8]) -> i32 {
let a = arr[0] as u32;
let b = arr[1] as u32;
let c = arr[2] as u32;
let d = arr[3] as u32;

((a << 24) | (b << 16) | (c << 8) | (d << 0)) as i32
fn combine(arr: [u8; 4]) -> i32 {
i32::from_be_bytes(arr)
Comment on lines -106 to +110
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: before it always swapped, now it only swaps on little endian system.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generated asms are the same: https://rust.godbolt.org/z/ezbn7K .
IIUC, the old code is loading Big Endian bytes from arr.
arr[0] is most significant value, arr[3] is the least significant one.

}
}

Expand Down