build: ubsan fixes by mizvekov · Pull Request #4254 · libuv/libuv

mizvekov · 2023-12-17T01:52:00Z

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.

bnoordhuis

I don't have time to look at this in detail today but apropos the linux and macos test failures:

 # exit code 6
# Output from process `ipc_send_zero`:
# /Users/runner/work/libuv/libuv/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 
# exit_cb
# Assertion failed in /Users/runner/work/libuv/libuv/test/test-ipc.c on line 95: `term_signal == 0` (6 == 0)

Does that mean ubsan wasn't running before? The failure looks legitimate; uv__write_req_update() should check buf->base != NULL.

CMakeLists.txt

mizvekov · 2023-12-19T15:25:14Z

Does that mean ubsan wasn't running before?

It was running, but in recovery mode, which means the default ubsan runtime, with the default settings, was only printing the problems, but not crashing the program.

The test-runner hides output unless the test fails, so that is why I believe you didn't see these failures before.

The failure looks legitimate; uv__write_req_update() should check buf->base != NULL.

Yep, fixed locally, just didn't push the commit yet.

I am investigating one more macOS ubsan failure I see running on my machine, but not on CI:

build/dbg/uv_run_tests_a osx_select_many_fds osx_select_many_fds
got some input
with a couple of lines
feel pretty happy
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/include/sys/_types/_fd_def.h:93:4: runtime error: index 47 out of bounds for type '__int32_t[32]' (aka 'int[32]')
    #0 0x100847a64 in uv__stream_osx_select stream.c:174
    #1 0x1873b2030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
    #2 0x1873ace38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/include/sys/_types/_fd_def.h:93:4 in

mizvekov · 2023-12-20T13:42:39Z

Is it possible to get macos-arm64 github workflow working?
No runners are picking up the job, so perhaps there is something missing on the project setup side?

bnoordhuis · 2023-12-20T14:43:48Z

Is it possible to get macos-arm64 github workflow working?

Are those even available when you're not on a paid plan?

mizvekov · 2023-12-20T16:27:17Z

Nevermind, I see that the closest to my machine (macOS 14 arm64) we have CI available for free is macOS 13 intel.

Adding it does expose a different test failure I had seen locally, but it's not ubsan related, so I will split that off for another PR.

And about the ubsan failure on osx_select_many_fds, I failed to realize before that this test is even skipped on CI due to lack of /dev/tty.

bnoordhuis

I have to admit, while I appreciate the work you've done, a number of these changes don't look Obviously Correct(TM) to me.

As in: they quite possibly are, but not obviously so, and that makes me hesitant to sign off on it. Maybe you can elaborate on why you made specific changes, and why you took specific approaches instead of others?

src/win/fs.c

src/win/getaddrinfo.c

src/win/process-stdio.c

src/unix/stream.c

src/win/fs.c

src/win/pipe.c

src/win/process-stdio.c

src/win/getaddrinfo.c

mizvekov · 2024-01-11T02:19:14Z

I have to admit, while I appreciate the work you've done, a number of these changes don't look Obviously Correct(TM) to me.

As in: they quite possibly are, but not obviously so, and that makes me hesitant to sign off on it. Maybe you can elaborate on why you made specific changes, and why you took specific approaches instead of others?

Thanks for the input. I gave it another round of edits, but otherwise please point to specific commits if you want more explanation on them.

mizvekov · 2024-01-23T02:20:05Z

Hello, just a friendly ping on this MR.

mizvekov · 2024-02-11T21:33:33Z

Hello, another ping.

Would you prefer we expedite this by splitting off less controversial commits to another MR? If you think that's possible, please let me know which commits.

vtjnash · 2024-02-12T02:28:39Z

I have honestly completely forgotten where we left off on this, after a very busy month. If there are small chunks you think would be easier to merge separately, by all means we can do those first.

vtjnash · 2024-07-29T20:49:31Z

Bump? Can you remind me where we are with this if nothing is left to change, rebase it, or split into smaller chunks so that each can be merged quickly?

mizvekov · 2024-07-29T20:52:06Z

I think this just got stalled on review from libuv side.

That's why I asked if there was a list of changes you would consider more quickly, so that we could reduce the diff.

Now I realize this needs to be rebased. I may have time to do that this weekend.

src/win/getaddrinfo.c

vtjnash · 2024-07-29T23:56:37Z

Thanks. I reviewed and identified only 1 commit that seemed it should be split out. As well as some minor nits that would be appreciate to clean up so that this can be merged soon.

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.

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 ```

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 ```

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 ```

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 ```

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 ```

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 ```

mizvekov · 2024-08-05T03:20:15Z

All done.

As you can see, we will be failing Windows UBSAN CI for now, as the fix for that failure in src/win/fs.c will be added in another PR.

vtjnash

Some style nits. I will apply those directly as this otherwise looks ready to merge to me

src/win/getaddrinfo.c

test/test-ref.c

src/win/getaddrinfo.c

The logic in #4254 is incorrect, and results in the addrinfo linked list only having a single result. Fix this by correcting the logic. Closes #4577

mizvekov marked this pull request as draft December 17, 2023 01:52

mizvekov force-pushed the ubsan branch 7 times, most recently from afc0330 to e1854ce Compare December 18, 2023 01:43

bnoordhuis reviewed Dec 19, 2023

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

mizvekov force-pushed the ubsan branch 4 times, most recently from 7186d90 to ec938b9 Compare December 20, 2023 12:52

mizvekov force-pushed the ubsan branch 2 times, most recently from dd67964 to 065d56d Compare December 20, 2023 15:54

mizvekov force-pushed the ubsan branch from 065d56d to cde7f8f Compare December 20, 2023 16:35

mizvekov marked this pull request as ready for review December 20, 2023 16:36

mizvekov changed the title ~~Draft: build: ubsan fixes~~ build: ubsan fixes Dec 20, 2023

mizvekov force-pushed the ubsan branch 2 times, most recently from 77d2342 to d05df52 Compare December 20, 2023 16:38

bnoordhuis force-pushed the ubsan branch from d05df52 to 48b7493 Compare December 22, 2023 11:43

bnoordhuis reviewed Jan 5, 2024

View reviewed changes

src/win/fs.c Outdated Show resolved Hide resolved

src/win/getaddrinfo.c Outdated Show resolved Hide resolved

src/win/getaddrinfo.c Outdated Show resolved Hide resolved

src/win/process-stdio.c Show resolved Hide resolved