build: ubsan fixes

[mizvekov]

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.

[mizvekov]

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]

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]

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

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

[mizvekov]

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?

[bnoordhuis]

Suggested change

	static const int64_t WIN_TO_UNIX_TICK_OFFSET = 11644473600 * TICKS_PER_SEC;
	static const int64_t WIN_TO_UNIX_TICK_OFFSET = 11644473600LL * TICKS_PER_SEC;

Otherwise it overflows; the promotion to int64_t is done after multiplication.

You cast to long on line 101 but sizeof(long) < sizeof(int64_t) on Windows, IIRC. If you cast to int64_t instead, multiplication would work okay because then 11644473600 gets implicitly promoted to int64_t too.

[mizvekov]

The literal 11644473600 is already bigger than LONG_MAX (Which on windows is 2,147,483,647).
It needs about 35 bits for a signed representation. This means that on those platforms where long is 32 bit's, the literal should have long long type, as that is the next rank, and the smallest that can represent it.

For reference of how this works, see [6.4.4.1]/5 from the C99 draft standard: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

The type of an integer constant is the first of the corresponding list in which its value can
be represented

And the corresponding list in the table for an unsiffixed decimal constant:

int
long int
long long int

[bnoordhuis]

Arguably easier to read if you make align_offset() return the new value instead of mutating it in place:

Suggested change

	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. */
	align_offset(&cur_off, sizeof(void*));
	cur_off += 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;

I don't understand the TODO, btw. I'm also not 100% sure the alignment computations here are correct but I don't know exactly how the objects are laid out in memory.

@libuv/windows you should take a careful look at this

[mizvekov]

Here we are pessimizing by assuming sockaddr needs pointer alignment, but I have seen that on some platforms it needs only uint16_t alignment or so.

The problem is that C89 does not have a ready made facility for obtaining alignment requirement of a given type.
I felt introducing that on this patch would be bigger change with more involved review than needed, considering existing code already suffers from this limitation, and if we introduced such a facility, there likely would be many other unrelated areas of libuv we would want to use it on.

[vtjnash]

I would guess that C89 can express it as an offsetof computation, roughly as follows: alignof(t) = offsetof(struct { char x; t y }, y) but it does seem pretty annoying

[mizvekov]

That does not work when compiling as C++, and as C it does, but GCC gives bogus warning about missing semicolon at the end of the struct definition. But you can't possibly put a semicolon there anyway: https://godbolt.org/z/GYWorbKMY

[mizvekov]

A bunch of other C compilers don't accept this as well. MSVC, Compcert, tiny c, sdcc, to name some.
https://godbolt.org/z/GYWorbKMY

[vtjnash]

I don't really see why this implicit truncation is better than the previous explicit cast

[mizvekov]

Well there is an assignment from an expression of long long type into a long variable, for sure, but the important detail here is if there is really truncation.
Truncation of a signed integer with with a simple cast like that is still UB, and UBSAN will catch that.

If we want defined behaviour, with either saturation or bit discarding assuming two's complement for example, we have to do something different.

[vtjnash]

IIRC, this field uint64_t for clients that already upgraded to libuv v2 (though it is unreleased) to avoid this exact problem -- as indeed it will become a significant problem in about a decade for existing clients of v1

[vtjnash]

Suggested change

	(unsigned long long)&fds[0]);
	(uintptr_t) &fds[0]);

nit: long long is not the size of a pointer

[mizvekov]

But we are not trying to fit a pointer in an integer per se. We want a random number, to be used for generating a unique pipe name, which avoids collisions.

I think unsigned long long has enough bits here, it's unlikely we would have so many open pipes we would see collisions in practice. The (pre-existing) hack though is trying to use a heap pointer as a source of randomness. Granted, if pointer size were larger than long long, we would be discarding some bits, but do you really think this is a problem?

[vtjnash]

That's right, but the HANDLE pointer needs to be cast first to an uintptr_t-sized object before up-or-downcasting the bytes (to make the compiler happy). We may want to consider using SystemFunction036 or uv_random eventually, since this is indeed a pretty ridiculous hack (which I say as the person who wrote it).

[mizvekov]

While the standard does not say anything directly about pointer sizes wrt integer sizes, unsigned long long is the largest integer available in standard C.

So if for a given platform a pointer could round trip through a suitably large integer, this integer would necessarily be at most as large as ULL.

[vtjnash]

Yes, that is fine, but the compiler won't like it if you cast directly to a different size

[mizvekov]

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]

Hello, just a friendly ping on this MR.

[mizvekov]

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]

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.