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.

Sign up for GitHub

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

Jump to bottom

Fix LibGit2 tests on x86_64-linux-musl #45638

Merged
merged 2 commits into from
Jun 20, 2022
Merged

Fix LibGit2 tests on x86_64-linux-musl #45638

merged 2 commits into from
Jun 20, 2022

Conversation

staticfloat
Copy link
Member

The LibGit2 tests have been failing on musl for a while now; Jameson, Alex and I have been investigating on Slack, and I'm assembling the fixes here.

@giordano giordano requested review from yuyichao and removed request for yuyichao June 10, 2022 18:40
@giordano giordano added test This change adds or pertains to unit tests libgit2 The libgit2 library or the LibGit2 stdlib module compiler:musl Support for musl linked binaries on linux instead of glibc labels Jun 10, 2022
@giordano
Copy link
Contributor

I presume this would fix #28805?

vtjnash
vtjnash reviewed Jun 11, 2022
View reviewed changes
src/sys.c Outdated Show resolved Hide resolved
@staticfloat
Copy link
Member Author

Hmmm, those changes don't seem to work; it appears that on glibc linux now we fail to read in the passphrase.

@staticfloat
Throw ArgumentError if unsafe_SecretBuffer!() is passed NULL
82615de 
Previously, if given a NULL `Cstring` we would blithely call `strlen()`
on it, which resulted in a segfault.  It is better if we throw an
exception instead.
@staticfloat
Add jl_getch(), use it from getpass()
22042be 
This works around the lack of `getch()` on non-windows platforms, such
that we can use the windows-specific `getpass()` on all platforms.  This
was necessary to prevent breakage from `musl` due to a bad interaction
between `with_fake_pty()` and `getpass()`.
@staticfloat staticfloat marked this pull request as ready for review June 17, 2022 17:46
@staticfloat
Copy link
Member Author

Passed locally, running full test on CI now!

@staticfloat
Copy link
Member Author

Wahoo! https://buildkite.com/julialang/julia-master/builds/12961#018172fc-9597-4493-a7d3-07a8ea85b9a7/545-978

There are two mysterious coredumps that I don't understand; looks like we segfault within jl_atexit_hook, but that doesn't prevent us from finishing the test suite successfully. :)

@staticfloat staticfloat added the merge me PR is reviewed. Merge when all tests are passing label Jun 17, 2022
@giordano giordano mentioned this pull request Jun 18, 2022
Merged
@giordano
Copy link
Contributor

There are two mysterious coredumps that I don't understand; looks like we segfault within jl_atexit_hook, but that doesn't prevent us from finishing the test suite successfully. :)

For future readers, this appears to be caused by #45726, addressed by #45765.

@staticfloat staticfloat merged commit 782458c into master Jun 20, 2022
@staticfloat staticfloat deleted the sf/fix_musl_libgit2 branch June 20, 2022 19:04
@giordano giordano mentioned this pull request Jun 20, 2022
Closed
@giordano giordano linked an issue Jun 20, 2022 that may be closed by this pull request
Segfault in LibGit2 tests on Alpine Linux #28805
Closed
@staticfloat
Copy link
Member Author

staticfloat commented Jul 5, 2022 •
edited
Loading

Update: we're still having problems with this, just unreliably, across all Linux platforms now. Joy.

Keno and I diagnosed this down to a race condition in our test suite, paired with a kernel oddity. It appears that special control characters such as EOF (0x04) get internally translated to a NULL (0x00) byte in the kernel buffer when the TTY is in "canonical" mode. However, when in non-canonical mode (which includes the "raw" mode we're setting here) you get unrestricted access to the kernel buffer. This means that if we receive the EOF while in canonical mode but don't read it, switching to raw mode and reading will return NULL instead of EOF because it has already been translated by the kernel upon reception.

The fix is as simple as it is heartbreaking; we must put jl_getch() to sleep and implement jl_getpass() instead. We should already be in "raw" terminal mode once we output the password prompt, so that there is no point in time between our program asking for the password and being ready to receive it. Poor little jl_getch() barely had a chance to learn what life is all about.

@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

Assignees
No one assigned
Labels
compiler:musl Support for musl linked binaries on linux instead of glibc libgit2 The libgit2 library or the LibGit2 stdlib module test This change adds or pertains to unit tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Segfault in LibGit2 tests on Alpine Linux
4 participants
@staticfloat @giordano @vtjnash @DilumAluthge