Skip to content

Commit

Permalink
build: ubsan fixes (libuv#4254)
Browse files Browse the repository at this point in the history
MSVC does not actually support ubsan. There is a long-standing ticket
requesting this:
https://developercommunity.visualstudio.com/t/add-support-for-ubsan/840750

There are no known compilers that currently accept the
`/fsanitize=undefined` spelling. clang-cl accepts `-fsanitize...`,
same as regular clang.

Also passes no-sanitizer-recover so that tests actually fail.

Fix various ubsan-detected errors, including:

* win: fix req-inl.h ubsan failure

Don't use CONTAINING_RECORD macro from WinSDK, as it doesn't use the
right trick which avoids member access on null pointer.

Fixes:
```
src/win/req-inl.h:86:10: runtime error: member access within null pointer of type 'uv_req_t' (aka 'struct uv_req_s')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior D:/a/libuv/libuv/src/win/req-inl.h:86:10
```

* test: fix ubsan failure on udp_ref3

Don't call functions through different function type.

Fixes:
```
src/win/udp.c:537:5: runtime error: call to function req_cb through pointer to incorrect function type 'void (*)(struct uv_udp_send_s *, int)'
test\test-ref.c:66: note: req_cb defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/win/udp.c:537:5 in
```

* win: fix process-stdio.c ubsan failure

When accessing HANDLEs within the stdio buffer, use memcpy / memset in order to respect alignment.

Fixes:
```
src/win/process-stdio.c:197:5: runtime error: store to misaligned address 0x0230ee72d107 for type 'HANDLE' (aka 'void *'), which requires 8 byte alignment
0x0230ee72d107: note: pointer points here
  00 00 cd cd cd  cd cd cd cd cd cd cd cd  cd cd cd cd cd cd cd cd  cd cd cd cd cd cd cd fd  fd fd fd
              ^
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/win/process-stdio.c:197:5 in
```

* win: fix getaddrinfo.c ubsan failure

Reworks buffer alignment handling to respect requirements.

Fixes:
```
src/win/getaddrinfo.c:157:23: runtime error: member access within misaligned address 0x0290e4c6a17c for type 'struct addrinfo', which requires 8 byte alignment
0x0290e4c6a17c: note: pointer points here
  00 00 00 00 cd cd cd cd  cd cd cd cd cd cd cd cd  cd cd cd cd cd cd cd cd  cd cd cd cd cd cd cd cd
              ^
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/win/getaddrinfo.c:157:23 in
```

* win: fix pipe.c ubsan failure

Changes "random" representation from pointer to number.

Fixes:
```
src/win/pipe.c:234:11: runtime error: applying non-zero offset to non-null pointer 0xffffffffffffffff produced null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/win/pipe.c:234:11 in
```

* unix: fix stream.c ubsan failure

Avoids performing pointer arithmetic on null pointer.

Fixes:
```
src/unix/stream.c:701:15: runtime error: applying zero offset to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /Users/runner/work/libuv/libuv/src/unix/stream.c:701:15 in
```
  • Loading branch information
mizvekov authored Aug 5, 2024
1 parent a6a987c commit 9b3b61f
Show file tree
Hide file tree
Showing 8 changed files with 110 additions and 93 deletions.
3 changes: 1 addition & 2 deletions .github/workflows/CI-win.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ jobs:
run:
cmake -S . -B build -DBUILD_TESTING=ON
-G "${{ matrix.config.toolchain }}" -A ${{ matrix.config.arch }}
${{ matrix.config.config == 'ASAN' && '-DASAN=on -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded' ||
matrix.config.config == 'UBSAN' && '-DUBSAN=on' || '' }}
${{ matrix.config.config == 'ASAN' && '-DASAN=on -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded' || '' }}

cmake --build build --config RelWithDebInfo

Expand Down
17 changes: 11 additions & 6 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -81,15 +81,20 @@ if(TSAN)
endif()

if(UBSAN)
cmake_minimum_required(VERSION 3.13)
list(APPEND uv_defines __UBSAN__=1)
if(CMAKE_C_COMPILER_ID MATCHES "AppleClang|GNU|Clang")
set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fno-omit-frame-pointer -fsanitize=undefined")
set (CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -fno-omit-frame-pointer -fsanitize=undefined")
set (CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -fno-omit-frame-pointer -fsanitize=undefined")
elseif(MSVC)
set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /fsanitize=undefined")
add_compile_options("-fsanitize=undefined" "-fno-sanitize-recover=undefined")
if (NOT WIN32)
add_link_options("-fsanitize=undefined")
endif()
if(MSVC)
add_compile_options("/Oy-")
else()
add_compile_options("-fno-omit-frame-pointer")
endif()
else()
message(SEND_ERROR "UndefinedBehaviorSanitizer support requires clang, gcc, or msvc. Try again with -DCMAKE_C_COMPILER.")
message(SEND_ERROR "UndefinedBehaviorSanitizer support requires clang or gcc. Try again with -DCMAKE_C_COMPILER.")
endif()
endif()

Expand Down
3 changes: 2 additions & 1 deletion src/unix/stream.c
Original file line number Diff line number Diff line change
Expand Up @@ -698,7 +698,8 @@ static int uv__write_req_update(uv_stream_t* stream,

do {
len = n < buf->len ? n : buf->len;
buf->base += len;
if (buf->len != 0)
buf->base += len;
buf->len -= len;
buf += (buf->len == 0); /* Advance to next buffer if this one is empty. */
n -= len;
Expand Down
122 changes: 63 additions & 59 deletions src/win/getaddrinfo.c
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,9 @@ int uv__getaddrinfo_translate_error(int sys_err) {
DECLSPEC_IMPORT void WSAAPI FreeAddrInfoW(PADDRINFOW pAddrInfo);
#endif


/* Adjust size value to be multiple of 4. Use to keep pointer aligned.
* Do we need different versions of this for different architectures? */
#define ALIGNED_SIZE(X) ((((X) + 3) >> 2) << 2)
static size_t align_offset(size_t off, size_t alignment) {
return ((off + alignment - 1) / alignment) * alignment;
}

#ifndef NDIS_IF_MAX_STRING_SIZE
#define NDIS_IF_MAX_STRING_SIZE IF_MAX_STRING_SIZE
Expand Down Expand Up @@ -103,17 +102,7 @@ static void uv__getaddrinfo_work(struct uv__work* w) {
* Each size calculation is adjusted to avoid unaligned pointers.
*/
static void uv__getaddrinfo_done(struct uv__work* w, int status) {
uv_getaddrinfo_t* req;
size_t addrinfo_len = 0;
ssize_t name_len = 0;
size_t addrinfo_struct_len = ALIGNED_SIZE(sizeof(struct addrinfo));
struct addrinfoW* addrinfow_ptr;
struct addrinfo* addrinfo_ptr;
char* alloc_ptr = NULL;
char* cur_ptr = NULL;
int r;

req = container_of(w, uv_getaddrinfo_t, work_req);
uv_getaddrinfo_t* req = container_of(w, uv_getaddrinfo_t, work_req);

/* release input parameter memory */
uv__free(req->alloc);
Expand All @@ -126,34 +115,44 @@ static void uv__getaddrinfo_done(struct uv__work* w, int status) {
}

if (req->retcode == 0) {
char* alloc_ptr = NULL;
size_t cur_off = 0;
size_t addrinfo_len;
/* Convert addrinfoW to addrinfo. First calculate required length. */
addrinfow_ptr = req->addrinfow;
struct addrinfoW* addrinfow_ptr = req->addrinfow;
while (addrinfow_ptr != NULL) {
addrinfo_len += addrinfo_struct_len +
ALIGNED_SIZE(addrinfow_ptr->ai_addrlen);
cur_off = align_offset(cur_off, sizeof(void*));
cur_off += sizeof(struct addrinfo);
/* TODO: This alignment could be smaller, if we could
portably get the alignment for sockaddr. */
cur_off = align_offset(cur_off, sizeof(void*));
cur_off += addrinfow_ptr->ai_addrlen;
if (addrinfow_ptr->ai_canonname != NULL) {
name_len = uv_utf16_length_as_wtf8(addrinfow_ptr->ai_canonname, -1);
ssize_t name_len =
uv_utf16_length_as_wtf8(addrinfow_ptr->ai_canonname, -1);
if (name_len < 0) {
req->retcode = name_len;
goto complete;
}
addrinfo_len += ALIGNED_SIZE(name_len + 1);
cur_off += name_len + 1;
}
addrinfow_ptr = addrinfow_ptr->ai_next;
}

/* allocate memory for addrinfo results */
alloc_ptr = (char*)uv__malloc(addrinfo_len);
addrinfo_len = cur_off;
alloc_ptr = uv__malloc(addrinfo_len);

/* do conversions */
if (alloc_ptr != NULL) {
cur_ptr = alloc_ptr;
struct addrinfo *addrinfo_ptr = (struct addrinfo *)alloc_ptr;
cur_off = 0;
addrinfow_ptr = req->addrinfow;

while (addrinfow_ptr != NULL) {
for (;;) {
cur_off += sizeof(struct addrinfo);
assert(cur_off <= addrinfo_len);
/* copy addrinfo struct data */
assert(cur_ptr + addrinfo_struct_len <= alloc_ptr + addrinfo_len);
addrinfo_ptr = (struct addrinfo*)cur_ptr;
addrinfo_ptr->ai_family = addrinfow_ptr->ai_family;
addrinfo_ptr->ai_socktype = addrinfow_ptr->ai_socktype;
addrinfo_ptr->ai_protocol = addrinfow_ptr->ai_protocol;
Expand All @@ -163,35 +162,37 @@ static void uv__getaddrinfo_done(struct uv__work* w, int status) {
addrinfo_ptr->ai_addr = NULL;
addrinfo_ptr->ai_next = NULL;

cur_ptr += addrinfo_struct_len;

/* copy sockaddr */
if (addrinfo_ptr->ai_addrlen > 0) {
assert(cur_ptr + addrinfo_ptr->ai_addrlen <=
alloc_ptr + addrinfo_len);
memcpy(cur_ptr, addrinfow_ptr->ai_addr, addrinfo_ptr->ai_addrlen);
addrinfo_ptr->ai_addr = (struct sockaddr*)cur_ptr;
cur_ptr += ALIGNED_SIZE(addrinfo_ptr->ai_addrlen);
cur_off = align_offset(cur_off, sizeof(void *));
addrinfo_ptr->ai_addr = (struct sockaddr *)(alloc_ptr + cur_off);
cur_off += addrinfo_ptr->ai_addrlen;
assert(cur_off <= addrinfo_len);
memcpy(addrinfo_ptr->ai_addr,
addrinfow_ptr->ai_addr,
addrinfo_ptr->ai_addrlen);
}

/* convert canonical name to UTF-8 */
if (addrinfow_ptr->ai_canonname != NULL) {
name_len = alloc_ptr + addrinfo_len - cur_ptr;
r = uv__copy_utf16_to_utf8(addrinfow_ptr->ai_canonname,
-1,
cur_ptr,
(size_t*)&name_len);
ssize_t name_len = addrinfo_len - cur_off;
addrinfo_ptr->ai_canonname = alloc_ptr + cur_off;
int r = uv__copy_utf16_to_utf8(addrinfow_ptr->ai_canonname,
-1,
addrinfo_ptr->ai_canonname,
(size_t*)&name_len);
assert(r == 0);
addrinfo_ptr->ai_canonname = cur_ptr;
cur_ptr += ALIGNED_SIZE(name_len + 1);
cur_off += name_len + 1;
assert(cur_off <= addrinfo_len);
}
assert(cur_ptr <= alloc_ptr + addrinfo_len);

/* set next ptr */
addrinfow_ptr = addrinfow_ptr->ai_next;
if (addrinfow_ptr != NULL) {
addrinfo_ptr->ai_next = (struct addrinfo*)cur_ptr;
}
if (addrinfow_ptr == NULL)
break;
cur_off = align_offset(cur_off, sizeof(void *));
addrinfo_ptr = (struct addrinfo *)(alloc_ptr + cur_off);
addrinfo_ptr->ai_next = addrinfo_ptr;
}
req->addrinfo = (struct addrinfo*)alloc_ptr;
} else {
Expand Down Expand Up @@ -242,10 +243,12 @@ int uv_getaddrinfo(uv_loop_t* loop,
const char* service,
const struct addrinfo* hints) {
char hostname_ascii[256];
size_t off = 0;
size_t nodesize = 0;
size_t servicesize = 0;
size_t serviceoff = 0;
size_t hintssize = 0;
char* alloc_ptr = NULL;
size_t hintoff = 0;
ssize_t rc;

if (req == NULL || (node == NULL && service == NULL)) {
Expand All @@ -268,51 +271,52 @@ int uv_getaddrinfo(uv_loop_t* loop,
return rc;
nodesize = strlen(hostname_ascii) + 1;
node = hostname_ascii;
off += nodesize * sizeof(WCHAR);
}

if (service != NULL) {
rc = uv_wtf8_length_as_utf16(service);
if (rc < 0)
return rc;
servicesize = rc;
off = align_offset(off, sizeof(WCHAR));
serviceoff = off;
off += servicesize * sizeof(WCHAR);
}

if (hints != NULL) {
hintssize = ALIGNED_SIZE(sizeof(struct addrinfoW));
off = align_offset(off, sizeof(void *));
hintoff = off;
hintssize = sizeof(struct addrinfoW);
off += hintssize;
}

/* allocate memory for inputs, and partition it as needed */
alloc_ptr = uv__malloc(ALIGNED_SIZE(nodesize * sizeof(WCHAR)) +
ALIGNED_SIZE(servicesize * sizeof(WCHAR)) +
hintssize);
if (!alloc_ptr)
req->alloc = uv__malloc(off);
if (!req->alloc)
return UV_ENOMEM;

/* save alloc_ptr now so we can free if error */
req->alloc = (void*) alloc_ptr;

/* Convert node string to UTF16 into allocated memory and save pointer in the
* request. The node here has been converted to ascii. */
if (node != NULL) {
req->node = (WCHAR*) alloc_ptr;
uv_wtf8_to_utf16(node, (WCHAR*) alloc_ptr, nodesize);
alloc_ptr += ALIGNED_SIZE(nodesize * sizeof(WCHAR));
req->node = (WCHAR*) req->alloc;
uv_wtf8_to_utf16(node, req->node, nodesize);
} else {
req->node = NULL;
}

/* Convert service string to UTF16 into allocated memory and save pointer in
* the req. */
if (service != NULL) {
req->service = (WCHAR*) alloc_ptr;
uv_wtf8_to_utf16(service, (WCHAR*) alloc_ptr, servicesize);
alloc_ptr += ALIGNED_SIZE(servicesize * sizeof(WCHAR));
req->service = (WCHAR*) ((char*) req->alloc + serviceoff);
uv_wtf8_to_utf16(service, req->service, servicesize);
} else {
req->service = NULL;
}

/* copy hints to allocated memory and save pointer in req */
if (hints != NULL) {
req->addrinfow = (struct addrinfoW*) alloc_ptr;
req->addrinfow = (struct addrinfoW*) ((char*) req->alloc + hintoff);
req->addrinfow->ai_family = hints->ai_family;
req->addrinfow->ai_socktype = hints->ai_socktype;
req->addrinfow->ai_protocol = hints->ai_protocol;
Expand Down
17 changes: 11 additions & 6 deletions src/win/pipe.c
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,8 @@ static int includes_nul(const char *s, size_t n) {
}


static void uv__unique_pipe_name(char* ptr, char* name, size_t size) {
snprintf(name, size, "\\\\?\\pipe\\uv\\%p-%lu", ptr, GetCurrentProcessId());
static void uv__unique_pipe_name(unsigned long long ptr, char* name, size_t size) {
snprintf(name, size, "\\\\?\\pipe\\uv\\%llu-%lu", ptr, GetCurrentProcessId());
}


Expand Down Expand Up @@ -208,7 +208,7 @@ static void close_pipe(uv_pipe_t* pipe) {

static int uv__pipe_server(
HANDLE* pipeHandle_ptr, DWORD access,
char* name, size_t nameSize, char* random) {
char* name, size_t nameSize, unsigned long long random) {
HANDLE pipeHandle;
int err;

Expand Down Expand Up @@ -249,7 +249,7 @@ static int uv__pipe_server(
static int uv__create_pipe_pair(
HANDLE* server_pipe_ptr, HANDLE* client_pipe_ptr,
unsigned int server_flags, unsigned int client_flags,
int inherit_client, char* random) {
int inherit_client, unsigned long long random) {
/* allowed flags are: UV_READABLE_PIPE | UV_WRITABLE_PIPE | UV_NONBLOCK_PIPE */
char pipe_name[64];
SECURITY_ATTRIBUTES sa;
Expand Down Expand Up @@ -357,7 +357,12 @@ int uv_pipe(uv_file fds[2], int read_flags, int write_flags) {
/* TODO: better source of local randomness than &fds? */
read_flags |= UV_READABLE_PIPE;
write_flags |= UV_WRITABLE_PIPE;
err = uv__create_pipe_pair(&readh, &writeh, read_flags, write_flags, 0, (char*) &fds[0]);
err = uv__create_pipe_pair(&readh,
&writeh,
read_flags,
write_flags,
0,
(uintptr_t) &fds[0]);
if (err != 0)
return err;
temp[0] = _open_osfhandle((intptr_t) readh, 0);
Expand Down Expand Up @@ -421,7 +426,7 @@ int uv__create_stdio_pipe_pair(uv_loop_t* loop,
}

err = uv__create_pipe_pair(&server_pipe, &client_pipe,
server_flags, client_flags, 1, (char*) server_pipe);
server_flags, client_flags, 1, (uintptr_t) server_pipe);
if (err)
goto error;

Expand Down
Loading

0 comments on commit 9b3b61f

Please sign in to comment.