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

Upgrade libgit2 to v0.27.2 #27525

Merged
merged 4 commits into from
Jun 13, 2018
Merged

Upgrade libgit2 to v0.27.2 #27525

merged 4 commits into from
Jun 13, 2018

Conversation

staticfloat
Copy link
Member

This is a possibly-unbroken upgrade path to a newer mbedTLS. It includes a pull request I just opened against libgit2, and in local testing successfully suppresses linking against libssl.so.

staticfloat referenced this pull request Jun 11, 2018
* upgrade libgit2 to 0.27.1
* fix for API changes
@staticfloat
Copy link
Member Author

This may be the first PR I have authored in my life to pass only Windows CI and none of the others.

@ararslan ararslan added the external dependencies Involves LLVM, OpenBLAS, or other linked libraries label Jun 11, 2018
@simonbyrne
Copy link
Contributor

I think my PR was only for libgit2 v0.27.1, but v0.27.2 had now been released, so we should use that.

@staticfloat
Copy link
Member Author

I think my PR was only for libgit2 v0.27.1, but v0.27.2 had now been released, so we should use that.

Yep, that's what this is based off of. I've also switched over to the CollisionDetection SHA1 backend as suggested to me in my PR over on the libgit2 repo, as there's no reason not to.

@simonbyrne
Copy link
Contributor

the libgit2 changes don't appear to be on this branch (or github is doing something funny with the diff).

@staticfloat
Copy link
Member Author

Wow, thanks for that. A rebase went bad I think. I git reflog'ed and rescued out my commits.

@simonbyrne
Copy link
Contributor

simonbyrne commented Jun 12, 2018

You will probably need most of the other changes from #27241. (getting rid of old patches + interface updates)

@@ -0,0 +1,39 @@
diff --git a/.travis.yml b/.travis.yml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a link to the upstream PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@staticfloat staticfloat force-pushed the sf/libgit2_mbedtls branch 4 times, most recently from 9461520 to 166a78f Compare June 12, 2018 07:22
simonbyrne and others added 3 commits June 12, 2018 00:22
* upgrade libgit2 to 0.27.1
* fix for API changes
libgit2 supports SHA1 collision detection [0], which basically
identifies files that have distinctive sequences of bytes that show they
have been hand-crafted to defeat SHA1, and instead alters the SHA1
hashing algorithm to do something different for those bytestreams.  This
"hardens" the SHA1 implementation, and importantly for us, doesn't
introduce any extra dependencies such as libssl.

[0]: https://blog.github.com/2017-03-20-sha-1-collision-detection-on-github-com/
@simonbyrne simonbyrne changed the title Upgrade mbedTLS to v0.27.2 Upgrade libgit2 to v0.27.2 Jun 12, 2018
@@ -0,0 +1 @@
6059f607530c302aa1b74c77fcbb5fa2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can remove this checksum (it was for 0.27.1)

@@ -0,0 +1 @@
6827d19be048be94d87d118c04c37ee9fc8161d1fb5820fd0feadc8b9d99f08835e1c5ccab47ae2406cb4601efce0d30d03bd66d1ec6e25d591c3ba1e0073ec7
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this one too

@simonbyrne
Copy link
Contributor

LGTM. Will need to check that ssh clones of private repos work on Windows (see #27241 (comment))

@simonbyrne
Copy link
Contributor

Is there a way we can run the buildbots without merging? Or just try it and see?

@simonbyrne
Copy link
Contributor

The 64-bit linux failure seems to persist, and has something to do with this line:

err, auth_attempts, p = challenge_prompt(ssh_ex, challenges)

@staticfloat
Copy link
Member Author

Is there a way we can run the buildbots without merging? Or just try it and see?

We can, and we should. We just go to build.julialang.org, navigate to the package_linux64 builder, for instance, click the big blue "package" button, and paste the gitsha of the tip of this branch in.

@staticfloat
Copy link
Member Author

If I remember correctly, that credentials thing is part of what will be fixed by #24738. Essentially, right now a piece of memory is getting overwritten with zeros in a way that can be a little race-conditiony. I've restarted the build one more time just to make sure.

@simonbyrne
Copy link
Contributor

Let's hope so. @mbauman is working on it as we speak.

@staticfloat
Copy link
Member Author

Great, I see that you've logged in and triggered the build. It will run through everything (including all tests) but we should download it locally and ensure it runs (and doesn't have a dependency on libssl.so) before merging this.

@simonbyrne
Copy link
Contributor

Will do. I triggered a windows one as well

@ViralBShah
Copy link
Member

CI straight flush!

@simonbyrne
Copy link
Contributor

simonbyrne commented Jun 13, 2018

Linux looks good:

├ ldd julia-a4797b3bb0/lib/julia/libgit2.so
	linux-vdso.so.1 =>  (0x00007ffe1b7f9000)
	librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007f94c12d8000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f94c10b9000)
	libcurl.so.4 => /home/simonbyrne/julia-a4797b3bb0/lib/julia/libcurl.so.4 (0x00007f94c0e4a000)
	libmbedtls.so.10 => /home/simonbyrne/julia-a4797b3bb0/lib/julia/libmbedtls.so.10 (0x00007f94c0c22000)
	libmbedx509.so.0 => /home/simonbyrne/julia-a4797b3bb0/lib/julia/libmbedx509.so.0 (0x00007f94c0a0e000)
	libmbedcrypto.so.0 => /home/simonbyrne/julia-a4797b3bb0/lib/julia/libmbedcrypto.so.0 (0x00007f94c07bc000)
	libssh2.so.1 => /home/simonbyrne/julia-a4797b3bb0/lib/julia/libssh2.so.1 (0x00007f94c058c000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f94c01ac000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f94c180b000)

though it failed the same test as travis.

@staticfloat
Copy link
Member Author

Yep, I'm calling this one good. Crossing fingers that I don't wake up tomorrow to us borking all travis again. ;)

@staticfloat staticfloat merged commit d011f4e into master Jun 13, 2018
@martinholters martinholters deleted the sf/libgit2_mbedtls branch June 13, 2018 07:47
@KristofferC
Copy link
Member

Does #27386 (comment) have anything to do with this? This was merged very recently though...

@staticfloat
Copy link
Member Author

No, I don't think it could be. This PR doesn't touch libcurl code.

@simonbyrne
Copy link
Contributor

The Windows build didn't seem to work, and we don't have new nightlies, so we can't yet test whether ssh works.

@staticfloat
Copy link
Member Author

I'm giving some TLC to the windows buildbots right now. Filesystem permissions problems, as usual.

@simonbyrne
Copy link
Contributor

FYI: I tried out ssh clone of a private repo on windows and it seemed to work.

@simonbyrne simonbyrne added the libgit2 The libgit2 library or the LibGit2 stdlib module label Jun 14, 2018
@nalimilan
Copy link
Member

I get a ReadOnlyMemoryError when building RPM nightlies since this PR (I've bisected it): https://copr-be.cloud.fedoraproject.org/results/nalimilan/julia-nightlies/fedora-28-x86_64/00766125-julia/build.log.gz

Ideas?

@staticfloat
Copy link
Member Author

Grrr, looks like this also broke ssh on OSX. During cmake time, we get:

-- Checking for module 'libssh2'
--   Package 'libssl', required by 'libssh2', not found
...
-- Enabled features:
 * threadsafe, threadsafe support
 * cURL, cURL for HTTP proxy support
 * HTTPS, using SecureTransport
 * SHA, using CollisionDetection
 * http-parser, http-parser support
 * zlib, using bundled zlib
 * iconv, iconv encoding conversion support

-- Disabled features:
 * debugpool, debug pool allocator
 * tracing, tracing support
 * SSH, SSH transport support
 * SPNEGO, SPNEGO authentication support

This causes this test to fail.

@simonbyrne
Copy link
Contributor

Based on the error message, it means that FIND_PKGLIBRARIES(LIBSSH2 libssh2) is failing somewhere?

I don't understand CMake well enough though.

@staticfloat
Copy link
Member Author

Fixed in #27601

@staticfloat
Copy link
Member Author

@nalimilan do you have any extra patches applied to libgit2, libssh2 or libmbedtls? It looks like we're dying within mbedTLS while initializing mbedTLS while libssh2 is initializing on behalf of libgit2's initialization routines.

@staticfloat
Copy link
Member Author

Also, upstream libgit2 patch has landed, huzzah. Hopefully eventually we can drop these patches.

@nalimilan
Copy link
Member

@nalimilan do you have any extra patches applied to libgit2, libssh2 or libmbedtls? It looks like we're dying within mbedTLS while initializing mbedTLS while libssh2 is initializing on behalf of libgit2's initialization routines.

Not AFAICT. I'm using USE_SYSTEM_LIBGIT2=0 USE_SYSTEM_LIBSSH2=0 USE_SYSTEM_MBEDTLS=0. Could it be related to the zeroed strings error at #27109? It happens on some systems and not others somewhat randomly.

@staticfloat
Copy link
Member Author

I don't think so, because this is dying within libgit2 initialization, whereas that issue is about the finalizers that are applied to certain libgit2 objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external dependencies Involves LLVM, OpenBLAS, or other linked libraries libgit2 The libgit2 library or the LibGit2 stdlib module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants