Skip to content
Open
Show file tree
Hide file tree
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
12 changes: 12 additions & 0 deletions library/std/src/sys/pal/windows/c.rs
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,18 @@ compat_fn_with_fallback! {
}
}

#[cfg(target_vendor = "win7")]
compat_fn_with_fallback! {
pub static WS2_32: &CStr = c"ws2_32";

#[cfg(target_vendor = "win7")]
Copy link
Member

Choose a reason for hiding this comment

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

this seems redundant with the outer cfg attr...

pub fn GetHostNameW(name: PWSTR, namelen: i32) -> i32 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it could be nice to link the original issue somewhere around here.

unsafe {
crate::sys::winsock::hostname_fallback(name, namelen)
}
}
}

cfg_select! {
target_vendor = "uwp" => {
windows_targets::link_raw_dylib!("ntdll.dll" "system" fn NtCreateFile(filehandle : *mut HANDLE, desiredaccess : FILE_ACCESS_RIGHTS, objectattributes : *const OBJECT_ATTRIBUTES, iostatusblock : *mut IO_STATUS_BLOCK, allocationsize : *const i64, fileattributes : FILE_FLAGS_AND_ATTRIBUTES, shareaccess : FILE_SHARE_MODE, createdisposition : NTCREATEFILE_CREATE_DISPOSITION, createoptions : NTCREATEFILE_CREATE_OPTIONS, eabuffer : *const core::ffi::c_void, ealength : u32) -> NTSTATUS);
Expand Down
2 changes: 2 additions & 0 deletions library/std/src/sys/pal/windows/c/bindings.txt
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ CONSOLE_MODE
CONSOLE_READCONSOLE_CONTROL
CONTEXT
CopyFileExW
CP_ACP
CP_UTF8
CREATE_ALWAYS
CREATE_BREAKAWAY_FROM_JOB
Expand Down Expand Up @@ -2170,6 +2171,7 @@ GetFileType
GETFINALPATHNAMEBYHANDLE_FLAGS
GetFinalPathNameByHandleW
GetFullPathNameW
gethostname
GetHostNameW
GetLastError
GetModuleFileNameW
Expand Down
2 changes: 2 additions & 0 deletions library/std/src/sys/pal/windows/c/windows_sys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ windows_targets::link!("ws2_32.dll" "system" fn closesocket(s : SOCKET) -> i32);
windows_targets::link!("ws2_32.dll" "system" fn connect(s : SOCKET, name : *const SOCKADDR, namelen : i32) -> i32);
windows_targets::link!("ws2_32.dll" "system" fn freeaddrinfo(paddrinfo : *const ADDRINFOA));
windows_targets::link!("ws2_32.dll" "system" fn getaddrinfo(pnodename : PCSTR, pservicename : PCSTR, phints : *const ADDRINFOA, ppresult : *mut *mut ADDRINFOA) -> i32);
windows_targets::link!("ws2_32.dll" "system" fn gethostname(name : PSTR, namelen : i32) -> i32);
windows_targets::link!("ws2_32.dll" "system" fn getpeername(s : SOCKET, name : *mut SOCKADDR, namelen : *mut i32) -> i32);
windows_targets::link!("ws2_32.dll" "system" fn getsockname(s : SOCKET, name : *mut SOCKADDR, namelen : *mut i32) -> i32);
windows_targets::link!("ws2_32.dll" "system" fn getsockopt(s : SOCKET, level : i32, optname : i32, optval : PSTR, optlen : *mut i32) -> i32);
Expand Down Expand Up @@ -444,6 +445,7 @@ pub struct CONTEXT_0_0 {
pub type CONTEXT_FLAGS = u32;
pub type COPYFILE_FLAGS = u32;
pub type COPYPROGRESSROUTINE_PROGRESS = u32;
pub const CP_ACP: u32 = 0u32;
pub const CP_UTF8: u32 = 65001u32;
pub const CREATE_ALWAYS: FILE_CREATION_DISPOSITION = 2u32;
pub const CREATE_BREAKAWAY_FROM_JOB: PROCESS_CREATION_FLAGS = 16777216u32;
Expand Down
43 changes: 43 additions & 0 deletions library/std/src/sys/pal/windows/winsock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,3 +78,46 @@ where
{
cvt(f())
}

#[cfg(target_vendor = "win7")]
pub unsafe fn hostname_fallback(name: c::PWSTR, namelen: i32) -> i32 {
assert!(namelen >= 1);
Copy link
Member

Choose a reason for hiding this comment

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

I think this should return SOCKET_ERROR and set the error to WSAEFAULT, just to be safe.


// The documentation of gethostname says that a buffer size of 256 is
// always enough.
let mut buffer = [const { mem::MaybeUninit::<u8>::uninit() }; 256];

// SAFETY: these parameters specify a valid, writable region of memory.
unsafe {
if c::gethostname(buffer.as_mut_ptr().cast(), buffer.len() as i32) == c::SOCKET_ERROR {
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be worth documenting that we're using this function in the "Underlying system calls" section of library/std/src/net/hostname.rs?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we document syscalls for the win7 target anywhere(?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's technically not a syscall either.

Copy link
Member

Choose a reason for hiding this comment

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

The rust documentation has a habit of using the term "syscall" when it means OS API (technically they're not even syscalls on Linux since we go through libc). But yes, I agree it makes sense to document it. E.g. perhaps similarly to SystemTime.

return c::SOCKET_ERROR;
}
Comment on lines +90 to +94
Copy link
Member

Choose a reason for hiding this comment

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

The safety comment seems to suffice for these lines, but the unsafe block continues. I think it would be better if each unsafe call had its own unsafe block with its own safety comments.


// Subtract one to leave space for the null terminator, as MultiByteToWideChar doesn't terminate the output buffer
// if the number of output characters is equal to the buffer length.
let len = c::MultiByteToWideChar(
c::CP_ACP,
c::MB_ERR_INVALID_CHARS,
buffer.as_mut_ptr().cast(),
-1,
name,
namelen - 1,
);

if len <= 0 {
Copy link
Member

Choose a reason for hiding this comment

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

MultiByteToWideChar always returns zero on error

Suggested change
if len <= 0 {
if len == 0 {

// GetHostNameW reports WSAEFAULT if the buffer is too small
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// GetHostNameW reports WSAEFAULT if the buffer is too small
// gethostname reports WSAEFAULT if the buffer is too small

if it indeed still applies here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was intentional - if GetHostNameW had a too small buffer, it'd report WSAEFAULT. This provides a fallback implementation, so it matches that error behavior.

if c::GetLastError() == c::ERROR_INSUFFICIENT_BUFFER {
Copy link
Member

Choose a reason for hiding this comment

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

If the error is not ERROR_INSUFFICIENT_BUFFER, this will not set the socket error variable to indicate a reason for SOCKET_ERROR. I'm not sure what error value is correct, through...

c::SetLastError(c::WSAEFAULT as _);
Copy link
Member

Choose a reason for hiding this comment

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

This should call WSASetLastError.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically unnecessary, as WSASetLastError has been the same as SetLastError for the last decades.

}
return c::SOCKET_ERROR;
}

// Ensure the output is always null terminated.
// If MultiByteToWideChar has already written a null terminator, that null terminator will be included in len
// and this will add a second one, but writing a zero is cheap enough to omit the length comparison.
name.add(len as _).write(0);
}

// Success
0
}
Loading