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

Bugfix release v0.27.2 #4632

Merged
merged 28 commits into from Jun 10, 2018
Merged

Bugfix release v0.27.2 #4632

merged 28 commits into from Jun 10, 2018

Conversation

pks-t
Copy link
Member

@pks-t pks-t commented Apr 20, 2018

It's been a month since v0.27.0 was released and we have accumulated some fixes on top of master again. To avoid the mess that was v0.26.4, I'm inclined to release v0.27.1 much faster than the previous bugfix releases. Not necessarily this month, but I'd vote to get it out no later than May 23rd, which would be exactly two months after the previous release. I'd be fine with releasing this earlier, though.

Changes or improvements

  • Fix builds with LibreSSL 2.7.

  • Fix for git_diff_status_char() not returning the correct mapping for
    GIT_DELTA_TYPECHANGE.

  • Fix for the submodules API not reporting errors when parsing the ".gitmodules"
    file.

  • Fix for hiding references in a graph walk not always limiting the graph
    correctly.

  • Fix for directory patterns with trailing spaces in attribute files not being
    handled correctly.

  • Fix SSH transports not properly disconnecting from the server.

  • Update our copy of SHA1DC to fix errors with endianess on some platforms.

@csware
Copy link
Contributor

csware commented Apr 20, 2018

What about #4633 and #4577?

@pks-t
Copy link
Member Author

pks-t commented Apr 20, 2018

They both look like they should be included. Up to now, I only added already merged PRs. Yours will probably be merged soonish, I'll then append them here. (I don't have enough time to review them right now, but will try to do so soon)

@pks-t pks-t force-pushed the pks/v0.27.1 branch 2 times, most recently from f40552b to 4444d1e Compare April 20, 2018 19:39
@pks-t
Copy link
Member Author

pks-t commented May 7, 2018

I'm still holding this off for some fixes, most importantly the build fixes around pkg-config.

@bgermann
Copy link
Contributor

Hi! Will this be released today as announced?

@pks-t
Copy link
Member Author

pks-t commented May 25, 2018

Thanks for reaching out, @bgermann. Unfortunately we have to delay that release a bit further. I'd like to get that submodule stuff in #4641 merged first, but I ran into some issues with Windows CI. I was quite loaded with work recently, but I'll try to tackle that soon and then finally do a release.

I'm sorry if this is causing inconveniences for you

@tiennou
Copy link
Contributor

tiennou commented May 25, 2018

Also, #4656 should be in there as well ? I'll try to redo a run of the (other) pkg-config/iOS changes as well, hopefully tonight…

pks-t and others added 22 commits May 30, 2018 08:12
The function `ssh_stream_free` takes over the responsibility of closing
channels and streams just before freeing their memory, but it does not
do so for the session. In fact, we never disconnect the session
ourselves at all, as libssh2 will not do so itself upon freeing the
structure. Quoting the documentation of `libssh2_session_free`:

    > Frees all resources associated with a session instance. Typically
    > called after libssh2_session_disconnect_ex,

The missing disconnect probably stems from a misunderstanding what it
actually does. As we are already closing the TCP socket ourselves, the
assumption was that no additional disconnect is required. But calling
`libssh2_session_disconnect` will notify the server that we are cleanly
closing the connection, such that the server can free his own resources.

Add a call to `libssh2_session_disconnect` to fix that issue.

[1]: https://www.libssh2.org/libssh2_session_free.html
The function `libssh2_session_startup` has been deprecated since libssh2
version 1.2.8 in favor of `libssh2_session_handshake` introduced in the
same version. libssh2 1.2.8 was released in April 2011, so it is already
seven years old. It is available in Debian Wheezy, Ubuntu Trusty and
CentOS 7.4, so the most important and conservative distros already have
it available. As such, it seems safe to just use the new function.
This updates the version of SHA1DC to c3e1304ea3.
When a ODB mempack gets free'd, we take no measures at all to free its
contents, most notably the objects added to the database, resulting in a
memory leak. Call `git_mempack_reset` previous to freeing the ODB
structures themselves, which takes care of releasing all associated
data structures.
When fetching into a repository which has symbolic references via the
"local" transport we run into an assert. The assert is being triggered
while we negotiate the packfile between the two repositories. When
hiding known revisions from the packbuilder revwalk, we unconditionally
hide all references of the local refdb. In case one of these references
is a symbolic reference, though, this means we're trying to hide a
`NULL` OID, which triggers the assert.

Fix the issue by only hiding OID references from the revwalk. Add a test
to catch this issue in the future.
When comparing whether a path matches a directory rule, we pass the
both the path and directory name to `fnmatch` with
`GIT_ATTR_FNMATCH_DIRECTORY` being set. `fnmatch` expects the pattern to
contain no trailing directory '/', which is why we try to always strip
patterns of trailing slashes. We do not handle that case correctly
though when the pattern itself has trailing spaces, causing the match to
fail.

Fix the issue by stripping trailing spaces and tabs for a rule previous
to checking whether the pattern is a directory pattern with a trailing
'/'. This replaces the whitespace-stripping in our ignore file parsing
code, which was stripping whitespaces too late. Add a test to catch
future breakage.
on 32-bit systems with 64-bit file descriptor offsets enabled
(added -D_FILE_OFFSET_BITS=64 when compiling the test suite)
This fixes a segfault in git_reference_owner on references returned from git_reference__read_head and git_reference_dup ones.
Signed-off-by: Sven Strickroth <email@cs-ware.de>
When we want to limit our graphwalk, we use the heuristic of checking
whether the newest limiting (uninteresting) revision is newer than the
oldest interesting revision. We do so by inspecting whether the first
item's commit time of the user-supplied list of revisions is newer than
the last added interesting revision. This is wrong though, as the user
supplied list is in no way guaranteed to be sorted by increasing commit
dates. This could lead us to abort the revwalk early before applying all
relevant limiting revisions, outputting revisions which should in fact
have been hidden.

Fix the heuristic by instead checking whether _any_ of the limiting
commits was made earlier than the last interesting commit. Add a test.
This adds the 'T' status character to git_diff_status_char() for diff
entries that change type.
LibreSSL 2.7 adds OpenSSL 1.1 API

Signed-off-by: Bernard Spil <brnrd@FreeBSD.org>
There are multiple references to undefined functions in the Microsoft
builds. Add headers to make them known.
Our CI builds have intermittent failures in our online tests, e.g. with
the message "A provided buffer was too small". This is not a programming
error in libgit2 but rather an error in the SChannel component of
Windows. Under certain circumstances involving Diffie-Hellman key
exchange, SChannel is unable to correctly handle input from the server.
This bug has already been fixed in recent patches for Windows 10 and
Windows Server 2016, but they are not yet available for AppVeyor.

Manually pamper over that issue by disabling all ciphersuites using DHE
via the registry. While this disables more ciphers than necessary, we
really don't care for that at all but just want to avoid build failures
due to that bug.

See [1], [2] or [3] for additional information.

1: aws/aws-sdk-cpp#671
2: https://github.com/dotnet/corefx/issues/7812
3: https://support.microsoft.com/en-us/help/2992611/ms14-066-vulnerability-in-schannel-could-allow-remote-code-execution-n
Commit 723e1e9 (appveyor: disable DHE to avoid spurious failures,
2018-03-29) added a workaround to fix spurious test failures due to a
bug in Windows' SChannel implementation. The workaround only worked by
accident, though, as the registry key was in fact mistyped. Fix the
typo.
CID:1383993, "In git_refspec__dwim_one: All paths that lead to this null pointer comparison already dereference the pointer earlier (CWE-476)"
Signed-off-by: Sven Strickroth <email@cs-ware.de>
As per CID:1378747, we might be called with a NULL repo, which would be deferenced in write_add_refspec
Valgrind log :

==17702== 18 bytes in 1 blocks are indirectly lost in loss record 69 of 1,123
==17702==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==17702==    by 0x5FDBB49: strdup (strdup.c:42)
==17702==    by 0x632B3E: git__strdup (util.h:106)
==17702==    by 0x632D2C: git_reference__alloc_symbolic (refs.c:64)
==17702==    by 0x62E0AF: loose_lookup (refdb_fs.c:408)
==17702==    by 0x62E636: refdb_fs_backend__iterator_next (refdb_fs.c:565)
==17702==    by 0x62CD8E: git_refdb_iterator_next (refdb.c:147)
==17702==    by 0x6347F2: git_reference_next (refs.c:838)
==17702==    by 0x6345CB: git_reference_foreach (refs.c:748)
==17702==    by 0x66BE62: local_download_pack (local.c:579)
==17702==    by 0x5DB48F: git_fetch_download_pack (fetch.c:148)
==17702==    by 0x639028: git_remote_download (remote.c:932)
==17702==    by 0x63919A: git_remote_fetch (remote.c:969)
==17702==    by 0x4ABEDD: test_fetchhead_nonetwork__fetch_into_repo_with_symrefs (nonetwork.c:362)
==17702==    by 0x4125D9: clar_run_test (clar.c:222)
==17702==    by 0x41287C: clar_run_suite (clar.c:286)
==17702==    by 0x412DDE: clar_test_run (clar.c:433)
==17702==    by 0x4105E1: main (main.c:24)
pks-t added 2 commits May 30, 2018 08:18
Libraries found by CMake modules are usually handled with their full
path. This makes linking against those libraries a lot more robust when
it comes to libraries in non-standard locations, as otherwise we might
mix up libraries from different locations when link directories are
given.

One excemption are libraries found by PKG_CHECK_MODULES. Instead of
returning libraries with their complete path, it will return the
variable names as well as a set of link directories. In case where
multiple sets of the same library are installed in different locations,
this can lead the compiler to link against the wrong libraries in the
end, when link directories of other dependencies are added.

To fix this shortcoming, we need to manually resolve library paths
returned by CMake against their respective library directories. This is
an easy task to do with `FIND_LIBRARY`.
With the recent change of always resolving pkg-config libraries to their
full path, we do not have to manage the LIBGIT2_LIBDIRS variable
anymore. The only other remaining user of LIBGIT2_LIBDIRS is winhttp,
which is a CMake-style library target and can thus be resolved by CMake
automatically.

Remove the variable to simplify our build system a bit.
@pks-t pks-t changed the title Bugfix release v0.27.1 Bugfix release v0.27.2 May 30, 2018
@pks-t
Copy link
Member Author

pks-t commented May 30, 2018

No, #4656 shouldn't be part of v0.27.2 as maint/v0.27 does not have any mbedTLS support at all right now.

@pks-t
Copy link
Member Author

pks-t commented May 30, 2018

Now that the security release is out we can get this back on track. This is now going to become v0.27.2

pks-t added 4 commits June 6, 2018 08:39
When loading submodule names, we build a map of submodule paths and
their respective names. While looping over the configuration keys,
we do not check though whether a submodule path was seen already. This
leads to a memory leak in case we have multiple submodules with the same
path, as we just overwrite the old value in the map in that case.

Fix the error by verifying that the path to be added is not yet part of
the string map. Git does not allow to have multiple submodules for a
path anyway, so we now do the same and detect this duplication,
reporting it to the user.
The test submodule::lookup::duplicated_path, which tries to verify that
we detect submodules with duplicated paths, currently relies on the
gitmodules file of "submod2_target". While this file has two gitmodules
with the same path, one of these gitmodules has an empty name and thus
does not pass `git_submodule_name_is_valid`. Because of this, the test
is in fact dependent on the iteration order in which we process the
submodules. In fact the "valid" submodule comes first, the "invalid"
submodule will cause the desired error. In fact the "invalid" submodule
comes first, it will be skipped due to its name being invalid, and we
will not see the desired error. While this works on the master branch
just right due to the refactoring of our config code, where iteration
order is now deterministic, this breaks on all older maintenance
branches.

Fix the issue by simply using `cl_git_rewritefile` to rewrite the
gitmodules file. This greatly simplifies the test and also makes the
intentions of it much clearer.
@pks-t
Copy link
Member Author

pks-t commented Jun 6, 2018

This now finally has the submodule fixes. I would like to release v0.27.2 on Sunday 10th, if nobody raises any concerns.

/cc @ethomson
/cc @carlosmn

@pks-t
Copy link
Member Author

pks-t commented Jun 10, 2018 via email

@ethomson
Copy link
Member

:shipit:

@pks-t pks-t merged commit 8d36dc6 into libgit2:maint/v0.27 Jun 10, 2018
@pks-t pks-t deleted the pks/v0.27.1 branch June 10, 2018 16:06
@pks-t
Copy link
Member Author

pks-t commented Jun 10, 2018

Thanks @ethomson. I've just released v0.27.2.

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

Successfully merging this pull request may close these issues.

None yet

8 participants