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

NameResolutionPal.Unix enabled async name resolution #34633

Merged
merged 37 commits into from
Jan 14, 2021
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
fc09ca1
Native implementation
gfoidl Apr 6, 2020
8c955bc
Native tests
gfoidl Apr 6, 2020
cf13c70
Managed implementation
gfoidl Apr 6, 2020
4614267
Managed tests
gfoidl Apr 6, 2020
1b2daab
Revert Interop.GetHostName change -- it's needed at other places too
gfoidl Apr 7, 2020
b9629e9
Fixed builds failures due to "unused parameter"
gfoidl Apr 7, 2020
6f64c86
Native: move struct addrinfo hint from stack-alloc to heap-alloc
gfoidl Apr 7, 2020
e7df0cc
PR feedback
gfoidl Apr 8, 2020
d7cb2be
Fixed leak in tests.c according to valgrind's run
gfoidl Apr 9, 2020
74b3031
Fixed bug due to marshalled string argument
gfoidl Apr 9, 2020
775ff72
More managed PalTests
gfoidl Apr 9, 2020
ec8522f
Revert "Native tests"
gfoidl Apr 9, 2020
298cf03
Handle the case of HostName="" and tests for this
gfoidl Apr 9, 2020
458579b
Use flexible array member to hold the address in struct state
gfoidl Apr 9, 2020
8f69164
Nits in native layer
gfoidl Apr 10, 2020
8cec5fb
Merge branch 'master' into unix-async-name-resolution
gfoidl Jul 1, 2020
f3ebd6a
Merge branch 'master' into unix-async-name-resolution
gfoidl Aug 13, 2020
cf45cc9
Merge branch 'master' into unix-async-name-resolution
gfoidl Dec 15, 2020
43f889f
Fixed native merge conflict + added AddressFamily-handling
gfoidl Dec 15, 2020
bd16c74
Fixed managed merge conflicts + added AddressFamily-handling
gfoidl Dec 15, 2020
ed5c39f
Updated NameResolutionPalTests for AddressFamily
gfoidl Dec 15, 2020
b8efde5
Removed EnsureSocketsAreInitialized
gfoidl Dec 15, 2020
04dbd8e
Fixed bug at native side
gfoidl Dec 15, 2020
37e4dd3
Refactor setting of the address family into TrySetAddressFamily
gfoidl Dec 15, 2020
7176378
Fixed unused parameter warning / failure
gfoidl Dec 15, 2020
b64867b
Little cleanup
gfoidl Dec 16, 2020
900834e
Fixed (unrelated) test failures
gfoidl Dec 16, 2020
198717e
Made LoggingTest async instead of GetAwaiter().GetResult()
gfoidl Dec 16, 2020
f85c316
Use function pointer
gfoidl Dec 16, 2020
438c75a
Use OperatinngSystem instead of RuntimeInformation in tests
gfoidl Dec 18, 2020
89c266f
Merge branch 'master' into unix-async-name-resolution
gfoidl Dec 18, 2020
33da8da
PR Feedback for CMake
gfoidl Dec 18, 2020
fa9c6f9
Update src/libraries/Native/Unix/System.Native/extra_libs.cmake
VSadov Dec 18, 2020
8266dd3
Revert "Update src/libraries/Native/Unix/System.Native/extra_libs.cmake"
gfoidl Jan 8, 2021
75e52c7
Another to build single file host
gfoidl Jan 8, 2021
89a7c5a
Merge branch 'master' into unix-async-name-resolution
gfoidl Jan 14, 2021
dd1e245
Test for !Windows instead excluding several Unix-flavors
gfoidl Jan 14, 2021
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
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Runtime.InteropServices;

internal static partial class Interop
Expand Down Expand Up @@ -33,9 +32,17 @@ internal unsafe struct HostEntry
internal int IPAddressCount; // Number of IP addresses in the list
}

internal unsafe delegate void GetHostEntryForNameCallback(HostEntry* entry, int status);
gfoidl marked this conversation as resolved.
Show resolved Hide resolved

[DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_PlatformSupportsGetAddrInfoAsync")]
internal static extern bool PlatformSupportsGetAddrInfoAsync();

[DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetHostEntryForName")]
internal static extern unsafe int GetHostEntryForName(string address, HostEntry* entry);

[DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetHostEntryForNameAsync")]
internal static extern unsafe int GetHostEntryForNameAsync(string address, HostEntry* entry, GetHostEntryForNameCallback callback);

[DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_FreeHostEntry")]
internal static extern unsafe void FreeHostEntry(HostEntry* entry);
}
Expand Down
1 change: 1 addition & 0 deletions src/libraries/Native/Unix/Common/pal_config.h.in
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#cmakedefine01 HAVE_F_DUPFD_CLOEXEC
#cmakedefine01 HAVE_O_CLOEXEC
#cmakedefine01 HAVE_GETIFADDRS
#cmakedefine01 HAVE_GETADDRINFO_A
#cmakedefine01 HAVE_UTSNAME_DOMAINNAME
#cmakedefine01 HAVE_STAT64
#cmakedefine01 HAVE_VFORK
Expand Down
4 changes: 4 additions & 0 deletions src/libraries/Native/Unix/System.Native/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ if (GEN_SHARED_LIB)
endif ()
endif ()
install_with_stripped_symbols (System.Native PROGRAMS .)

if (HAVE_GETADDRINFO_A)
target_link_libraries(System.Native anl)
gfoidl marked this conversation as resolved.
Show resolved Hide resolved
endif ()
endif ()

add_library(System.Native-Static
Expand Down
179 changes: 150 additions & 29 deletions src/libraries/Native/Unix/System.Native/pal_networking.c
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@
#if HAVE_LINUX_CAN_H
#include <linux/can.h>
#endif
#if HAVE_GETADDRINFO_A
#include <stdatomic.h>
#include <signal.h>
#endif
#if HAVE_KQUEUE
#if KEVENT_HAS_VOID_UDATA
static void* GetKeventUdata(uintptr_t udata)
Expand Down Expand Up @@ -253,32 +257,14 @@ static int32_t CopySockAddrToIPAddress(sockaddr* addr, sa_family_t family, IPAdd
return -1;
}

int32_t SystemNative_GetHostEntryForName(const uint8_t* address, HostEntry* entry)
static int32_t GetHostEntries(const uint8_t* address, struct addrinfo* info, HostEntry* entry)
{
if (address == NULL || entry == NULL)
{
return GetAddrInfoErrorFlags_EAI_BADARG;
}

int32_t ret = GetAddrInfoErrorFlags_EAI_SUCCESS;

struct addrinfo* info = NULL;
#if HAVE_GETIFADDRS
struct ifaddrs* addrs = NULL;
#endif

// Get all address families and the canonical name
struct addrinfo hint;
memset(&hint, 0, sizeof(struct addrinfo));
hint.ai_family = AF_UNSPEC;
hint.ai_flags = AI_CANONNAME;

int result = getaddrinfo((const char*)address, NULL, &hint, &info);
if (result != 0)
{
return ConvertGetAddrInfoAndGetNameInfoErrorsToPal(result);
}

entry->CanonicalName = NULL;
entry->Aliases = NULL;
entry->IPAddressList = NULL;
Expand Down Expand Up @@ -306,7 +292,8 @@ int32_t SystemNative_GetHostEntryForName(const uint8_t* address, HostEntry* entr

#if HAVE_GETIFADDRS
char name[_POSIX_HOST_NAME_MAX];
result = gethostname((char*)name, _POSIX_HOST_NAME_MAX);

int result = gethostname((char*)name, _POSIX_HOST_NAME_MAX);

bool includeIPv4Loopback = true;
bool includeIPv6Loopback = true;
Expand Down Expand Up @@ -356,6 +343,8 @@ int32_t SystemNative_GetHostEntryForName(const uint8_t* address, HostEntry* entr
}
}
}
#else
(void)address;
gfoidl marked this conversation as resolved.
Show resolved Hide resolved
#endif

if (entry->IPAddressCount > 0)
Expand Down Expand Up @@ -432,12 +421,144 @@ int32_t SystemNative_GetHostEntryForName(const uint8_t* address, HostEntry* entr
return ret;
}

#if HAVE_GETADDRINFO_A
struct GetAddrInfoAsyncState
{
struct gaicb gai_request;
struct gaicb* gai_requests;
struct sigevent sigevent;

uint8_t* address;
HostEntry* entry;
GetHostEntryForNameCallback callback;
};

static void GetHostEntryForNameAsyncComplete(sigval_t context)
{
struct GetAddrInfoAsyncState* state = (struct GetAddrInfoAsyncState*)context.sival_ptr;

atomic_thread_fence(memory_order_acquire);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the fence necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

This callback comes in on another thread. It ensures visibility to this thread of everything in state.

getaddrinfo_a probably does something like this already, so it might be safe to remove, but it isn't documented.


GetHostEntryForNameCallback callback = state->callback;

int ret = ConvertGetAddrInfoAndGetNameInfoErrorsToPal(gai_error(&state->gai_request));

if (ret == 0)
{
const uint8_t* address = state->address;
struct addrinfo* info = state->gai_request.ar_result;

ret = GetHostEntries(address, info, state->entry);
}

if (callback != NULL)
gfoidl marked this conversation as resolved.
Show resolved Hide resolved
{
callback(state->entry, ret);
}

free(state->address);
free(state);
}
#endif

int32_t SystemNative_PlatformSupportsGetAddrInfoAsync()
{
return HAVE_GETADDRINFO_A;
}

// Get all address families and the canonical name
static const struct addrinfo s_hostEntryForNameHints = {.ai_family = AF_UNSPEC, .ai_flags = AI_CANONNAME};

int32_t SystemNative_GetHostEntryForName(const uint8_t* address, HostEntry* entry)
{
if (address == NULL || entry == NULL)
{
return GetAddrInfoErrorFlags_EAI_BADARG;
}

struct addrinfo* info = NULL;

int result = getaddrinfo((const char*)address, NULL, &s_hostEntryForNameHints, &info);
if (result != 0)
{
return ConvertGetAddrInfoAndGetNameInfoErrorsToPal(result);
}

return GetHostEntries(address, info, entry);
}

int32_t SystemNative_GetHostEntryForNameAsync(const uint8_t* address, HostEntry* entry, GetHostEntryForNameCallback callback)
{
#if HAVE_GETADDRINFO_A
if (address == NULL)
{
return GetAddrInfoErrorFlags_EAI_BADARG;
}

struct GetAddrInfoAsyncState* state = malloc(sizeof(*state));

if (state == NULL)
{
return GetAddrInfoErrorFlags_EAI_MEMORY;
}

char* localAddress = strdup((const char*)address);
Copy link
Contributor

Choose a reason for hiding this comment

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

malloc() result is NULL-checked but strdup() isn't

Copy link
Contributor

@lpereira lpereira Apr 9, 2020

Choose a reason for hiding this comment

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

You could do both allocations here in a single one by using a flexible member in struct GetAddrInfoAsyncState:

struct GetAddrInfoAsyncState {
  ...
  char address[];
};

size_t addrlen = strlen(address);
struct GetAddrInfoAsyncState* state = malloc(sizeof(*state) + addrlen + 1);

memcpy(state->address, address, addlen+1);

Also, I'd bail out if addrlen > 253 (hostnames can't be larger than that per the RFC; otherwise, you'd have to check for overflow before passing the len to malloc).


*state = (struct GetAddrInfoAsyncState) {
.gai_request = {
.ar_name = localAddress,
.ar_service = NULL,
.ar_request = &s_hostEntryForNameHints,
.ar_result = NULL
},
.gai_requests = &state->gai_request,
.sigevent = {
.sigev_notify = SIGEV_THREAD,
.sigev_value = {
.sival_ptr = state
},
.sigev_notify_function = GetHostEntryForNameAsyncComplete
},
.address = (uint8_t*)localAddress,
.entry = entry,
.callback = callback
};

atomic_thread_fence(memory_order_release);

int32_t result = getaddrinfo_a(GAI_NOWAIT, &state->gai_requests, 1, &state->sigevent);

if (result != 0)
{
free(localAddress);
free(state);
return ConvertGetAddrInfoAndGetNameInfoErrorsToPal(result);
}

return result;
lpereira marked this conversation as resolved.
Show resolved Hide resolved
#else
(void)address;
(void)entry;
(void)callback;

// GetHostEntryForNameAsync is not supported on this platform.
return -1;
#endif
}

void SystemNative_FreeHostEntry(HostEntry* entry)
{
if (entry != NULL)
{
free(entry->CanonicalName);
free(entry->IPAddressList);
if (entry->CanonicalName != NULL)
{
free(entry->CanonicalName);
}

if (entry->IPAddressList != NULL)
{
free(entry->IPAddressList);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: You don't need to null-check before calling free(). free() can take any value returned by malloc(), including NULL. (Change is harmless, but something to keep in mind.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the valuable input!

Maybe it was an artefact of the bug hunt, but Valgrind complained this. That's why I changed this. Will revert to the cleaner version.


entry->CanonicalName = NULL;
entry->IPAddressList = NULL;
Expand Down Expand Up @@ -468,13 +589,13 @@ static inline NativeFlagsType ConvertGetNameInfoFlagsToNative(int32_t flags)
}

int32_t SystemNative_GetNameInfo(const uint8_t* address,
int32_t addressLength,
int8_t isIPv6,
uint8_t* host,
int32_t hostLength,
uint8_t* service,
int32_t serviceLength,
int32_t flags)
int32_t addressLength,
int8_t isIPv6,
uint8_t* host,
int32_t hostLength,
uint8_t* service,
int32_t serviceLength,
int32_t flags)
{
assert(address != NULL);
assert(addressLength > 0);
Expand Down
6 changes: 5 additions & 1 deletion src/libraries/Native/Unix/System.Native/pal_networking.h
Original file line number Diff line number Diff line change
Expand Up @@ -310,10 +310,14 @@ typedef struct
uint32_t Padding; // Pad out to 8-byte alignment
} SocketEvent;

PALEXPORT int32_t SystemNative_PlatformSupportsGetAddrInfoAsync(void);

PALEXPORT int32_t SystemNative_GetHostEntryForName(const uint8_t* address, HostEntry* entry);

PALEXPORT void SystemNative_FreeHostEntry(HostEntry* entry);
typedef void (*GetHostEntryForNameCallback)(HostEntry* entry, int status);
PALEXPORT int32_t SystemNative_GetHostEntryForNameAsync(const uint8_t* address, HostEntry* entry, GetHostEntryForNameCallback callback);

PALEXPORT void SystemNative_FreeHostEntry(HostEntry* entry);

PALEXPORT int32_t SystemNative_GetNameInfo(const uint8_t* address,
int32_t addressLength,
Expand Down
15 changes: 14 additions & 1 deletion src/libraries/Native/Unix/configure.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ include(CheckPrototypeDefinition)
include(CheckStructHasMember)
include(CheckSymbolExists)
include(CheckTypeSize)

include(CMakePushCheckState)

if (CLR_CMAKE_TARGET_ANDROID)
set(PAL_UNIX_NAME \"ANDROID\")
Expand Down Expand Up @@ -822,6 +822,19 @@ check_symbol_exists(
HAVE_INOTIFY_RM_WATCH)
set (CMAKE_REQUIRED_LIBRARIES ${PREVIOUS_CMAKE_REQUIRED_LIBRARIES})

if (CLR_CMAKE_TARGET_LINUX)
cmake_push_check_state(RESET)
set (CMAKE_REQUIRED_DEFINITIONS "-D_GNU_SOURCE")
set (CMAKE_REQUIRED_LIBRARIES "-lanl")

check_symbol_exists(
getaddrinfo_a
netdb.h
HAVE_GETADDRINFO_A)

cmake_pop_check_state()
endif ()

set (HAVE_INOTIFY 0)
if (HAVE_INOTIFY_INIT AND HAVE_INOTIFY_ADD_WATCH AND HAVE_INOTIFY_RM_WATCH)
set (HAVE_INOTIFY 1)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ private static void ValidateHostName(string hostName)
const int MaxHostName = 255;

if (hostName.Length > MaxHostName ||
(hostName.Length == MaxHostName && hostName[MaxHostName - 1] != '.')) // If 255 chars, the last one must be a dot.
(hostName.Length == MaxHostName && hostName[MaxHostName - 1] != '.')) // If 255 chars, the last one must be a dot.
{
throw new ArgumentOutOfRangeException(nameof(hostName),
SR.Format(SR.net_toolong, nameof(hostName), MaxHostName.ToString(NumberFormatInfo.CurrentInfo)));
Expand Down
Loading