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

Have working TTY tests on CI #4262

Open
mizvekov opened this issue Dec 20, 2023 · 4 comments
Open

Have working TTY tests on CI #4262

mizvekov opened this issue Dec 20, 2023 · 4 comments

Comments

@mizvekov
Copy link
Contributor

Some tests try to open /dev/tty, but that doesn't work on CI environment.

For example, osx_select_many_fds built with UBSAN fails with:

/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 0x100e2ff74 in uv__stream_osx_select stream.c:177
    #1 0x18ab96030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
    #2 0x18ab90e38 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

But even if we fix that, we can't prevent regressions as CI does not cover this test.

@bnoordhuis
Copy link
Member

The bug here is that fd_set on macos is limited to file descriptors < 1,024 and it's trying to add file descriptor 1,500 or thereabouts.

It's possible to redefine that limit with -DFD_SETSIZE=<n> but that's at best a stop-gap measure. It's an inescapable fact of life that fd_sets have an upper limit.

We can't switch to poll(2) however because not all file descriptors work correctly with it (and that's exactly the reason why uv__stream_osx_select exists, to handle file descriptors that don't work well with poll or kqueue) so stop-gap measure it is: #4273

@vtjnash
Copy link
Member

vtjnash commented Jan 5, 2024

Looks like posix-compatibility requires it to be limited, but -D_DARWIN_UNLIMITED_SELECT will disable that limit, which is non-compliant with posix but clearly better

@bnoordhuis
Copy link
Member

So, it turns out libuv is already compiled with -D_DARWIN_UNLIMITED_SELECT=1 (and has been since practically forever.)

My best guess is that ubsan's runtime component is unaware of that fact and assumes fd_set only has room for 1,024 file descriptors. I don't have a macbook myself anymore to test on but the warning can probably be suppressed. PR welcome.

@mizvekov
Copy link
Contributor Author

I don't think ubsan is hardcoded with specific array sizes used in OS implementation.

It tests common undefined behavior based on language rules. It has a very low chance of having a false negative, the code often is invalid and can likely be trivially fixed, but happenstance and compiler implementation details might make it so an issue is not observed in practice.

I might have bandwidth to take a look at this UBSAN issue in detail.

But I apologize for the confusion: this ticket was created with the intention of getting to the bottom of why we don't run TTY tests on CI. I mentioned the UBSAN problem as a motivation, if we fix the UBSAN issue, we can better demontrate the fix, and prevent future regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants