Release v0.26.8 by pks-t · Pull Request #4865 · libgit2/libgit2 · GitHub
Skip to content

Release v0.26.8 - #4865

Merged
pks-t merged 119 commits into
libgit2:maint/v0.26from
pks-t:pks/v0.26.8
Oct 26, 2018
Merged

pks-t merged 119 commits into
libgit2:maint/v0.26from
pks-t:pks/v0.26.8

Conversation

@pks-t

@pks-t pks-t commented Oct 26, 2018

Copy link
Copy Markdown
Member

No description provided.

pks-t and others added 30 commits October 19, 2018 13:09
All our tests running against a local SSH server usually read the
server's URL from environment variables. But online::clone::ssh_cert
test fails to do so and instead always connects to
"ssh://localhost/foo". This assumption breaks whenever the SSH server is
not running on the standard port, e.g. when it is running as a user.

Fix the issue by using the URL provided by the environment.

(cherry picked from commit c2c95ad)
Some tests of ours require to be running against an SSH server.
Currently, we simply run against the SSH server provided and started by
Travis itself. As our Linux tests run in a sudo-less environment, we
have no control over its configuration and startup/shutdown procedure.
While this has been no problem until now, it will become a problem as
soon as we migrate over to newer Precise images, as the SSH server does
not have any host keys set up. Luckily, we can simply set up our own
unpriviledged SSH server. This has the benefit of us being able to
modify its configuration even in a sudo-less environment.

This commit sets up the unpriviledged SSH server on port 2222.

(cherry picked from commit 0661990)
Ubuntu 12.04 (Precise Pangolin) reaches end of life on April 28th, 2017.
As such, we should update our build infrastructure to use the next
available LTS release, which is Ubuntu 14.04 LTS (Trusty Tahr). Note
that Trusty is still considered beta quality on Travis. But considering
we are able to correctly build and test libgit2, this seems to be a
non-issue for us.

Switch over our default distribution to Trusty. As Precise still has
extended support for paying customers, add an additional job which
compiles libgit2 on the old release.

(cherry picked from commit 7c8d460)
Ubuntu trusty has a bug in curl when using NTLM credentials in a proxy,
dereferencing a null pointer and causing segmentation faults.  Use a
custom-patched version of libcurl that avoids this issue.

(cherry picked from commit f031e20)
(cherry picked from commit 4da3819)
(cherry picked from commit 697583e)
By default, CMake will not build our examples directory. As we do not
instruct either the MinGW or MSVC builds on AppVeyor to enable building
these examples, we cannot verify that those examples at least build on
Windows systems.

Fix that by passing `-DBUILD_EXAMPLES=ON` to AppVeyor's CMake
invocation.

(cherry picked from commit 0b98a66)
The getline(3) function call is not part of ISO C and, most importantly,
it is not implemented on Microsoft Windows platforms. As our networking
example code makes use of getline, this breaks builds on MSVC and MinGW.
As this code wasn't built prior to the previous commit, this was never
noticed.

Fix the error by instead implementing a `readline` function, which
simply reads the password from stdin until it reads a newline
character.

(cherry picked from commit bf15dbf)
The macOS tests are by far the slowest right now. This attempts to remedy the
situation somewhat by asking clar to put its test data on a ramdisk.

(cherry picked from commit 37bb151)
(cherry picked from commit 71ba464)
Move back to Travis's VM infrastructure for efficiency.

(cherry picked from commit 9dc21ef)
The trusty dependencies are now hosted on Bintray.

(cherry picked from commit da9898a)
The VM on Travis apparently will still proceed, but it's good practice.

(cherry picked from commit 6e74813)
Ubuntu Precise is end of life since April 2017. At that point in time,
Precise was still the main distro on which Travis CI built upon, with
the Trusty-based images still being in a beta state. But since June
21st, Trusty has officially moved out of beta and is now the default
image for all new builds. Right now, we build on both old and new images
to assure we support both.

Unfortunately, this leaves us with the highest minimum version for CMake
being 2.8.7, as Precise has no greater version in its repositories. And
because of this limitation, we cannot actually use object libraries in
our build instructions. But considering Precise is end of life and
Trusty is now the new default for Travis, we can and should drop support
for this old and unmaintained distribution. And so we do.

(cherry picked from commit c17c3f8)
Our tracing architecture is not built by default, causing the Travis CI
to not execute some code and skip several tests. As AppVeyor has already
enabled the tracing architecture when building the code, we should do
the same for Travis CI to have this code being tested on macOS and
Linux.

Add "-DENABLE_TRACE=ON" to our release-build options of Travis.

(cherry picked from commit 8999f6a)
Some function bodies of tests which are not applicable to the Win32
platform are completely #ifdef'd out instead of calling `cl_skip()`.
This leaves us with no indication that these tests are not being
executed at all and may thus cause decreased scrutiny when investigating
skipped tests. Improve the situation by calling `cl_skip()` instead of
just doing nothing.

(cherry picked from commit 72c28ab)
When the function `expect_iterator_items` surpasses the number of
expected items, we simply break the loop. This causes us to trigger an
assert later on which has message attached, which is annoying when
trying to locate the root error cause. Instead, directly assert that the
current count is still smaller or equal to the expected count inside of
the loop.

(cherry picked from commit 9aba763)
The test `iterator::workdir::filesystem_gunk` is usually not executed,
as it is guarded by the environment variable "GITTEST_INVASIVE_SPEED"
due to its effects on speed. As such, it has become stale and does not
account for new references which have meanwhile been added to the
testrepo, causing it to fail. Fix this by raising the number of expected
references to 15.

(cherry picked from commit b8c1449)
Our performance tests (or to be more concrete, our single performance
test) are not built by default, as they are always #ifdef'd out. While
it is true that we don't want to run performance tests by default, not
compiling them at all may cause code rot and is thus an unfavorable
approach to handle this.

We can easily improve this situation: this commit removes the #ifdef,
causing the code to always be compiled. Furthermore, we add `-xperf` to
the default command line parameters of `generate.py`, thus causing the
tests to be excluded by default.

Due to this approach, we are now able to execute the performance tests
by passing `-sperf` to `libgit2_clar`. Unfortunately, we cannot execute
the performance tests on Travis or AppVeyor as they rely on history
being available for the libgit2 repository. As both do a shallow clone
only, though, this is not given.

(cherry picked from commit 543ec14)
We support two types of passing credentials to the proxy, either via the
URL or explicitly by specifying user and password. We test these types
by modifying the proxy URL and executing the tests twice, which is
in fact unnecessary and requires us to maintain the list of environment
variables and test executions across multiple CI infrastructures.

To fix the situation, we can just always pass the host, port, user and
password to the tests. The tests can then assemble the complete URL
either with or without included credentials, allowing us to test both
cases in-process.

(cherry picked from commit fea6092)
Right now, we test our credential callback code twice, once via SSH on
localhost and once via a non-existent GitHub repository. While the first
URL makes sense to be configurable, it does not make sense to hard-code
the non-existing repository, which requires us to call tests multiple
times. Instead, we can just inline the URL into another set of tests.

(cherry picked from commit 54a1bf0)
Some tests shall be run against our own SSH server we spin up in Travis.
As those need to be run separate from our previous tests which run
against git-daemon, we have to do this in a separate step. Instead of
bundling all that knowledge in the CI script, move it into the test
build instructions by creating a new test target.

(cherry picked from commit 5874e15)
AppVeyor currently does provide three standard build worker images with
VS2013, VS2015 and VS2017. Right now, we are using the implicitly, which
is the VS2015 one. We want to be more explicit about this, so that we
can easily switch build images based on the job. So starting from this
commit, we explicitly set the `APPVEYOR_BUILD_WORKER_IMAGE` variable per
job, which enables us to choose different images.

To be able to test a wider range of build configurations, this commit
also switches the jobs for VC2010 over to use the older, VS2013 based
images. As the next commit will introduce two new jobs for building with
VS2015, we have then covered both build environments.

Also, let us be a bit more explicit regarding the CMake generator.
Instead of only saying "Visual Studio 10", use the more descriptive
value "Visual Studio 10 2010" to at least avoid some confusion
surrounding the versioning scheme of Visual Studio.

(cherry picked from commit e1076db)
In order to cover a wider range of build environments, add two more jobs
which build and test libgit2 on Visual Studio 14 2015.

(cherry picked from commit 03a95bc)
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
(cherry picked from commit 723e1e9)
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.

(cherry picked from commit 3a72b0e)
When executing `libgit2_clar -smerge -invalid_option`, it will first execute
the merge test suite and afterwards output help because of the invalid option.

With this changa, it verifies all options before execute. If there are any
invalid options, it will output help and exit without actually executing
the test suites.

(cherry picked from commit 3275863)
(cherry picked from commit 2f4e7cb)
Similar to the way we parse the ctest output on POSIX systems, do the
same on Windows.  This allows us to append the `-r` flag to clar after
we've identified the command to run.

(cherry picked from commit 7c9769d)
Don't stop on test failures; run all the tests, even when a test fails.

(cherry picked from commit 429c7f1)
PowerShell can _read_ top-level variables in functions, but cannot _update_
top-level variables in functions unless they're explicitly prefixed with
`$global`.

(cherry picked from commit 0e26717)
@pks-t
pks-t force-pushed the pks/v0.26.8 branch 3 times, most recently from 6f0b000 to 87a046f Compare October 26, 2018 12:22
pks-t and others added 21 commits October 26, 2018 14:35
Free the url field when resetting the stream to avoid leaking it.

(cherry picked from commit ca2eb46)
Don't just free the push status structure, actually free the strings that were
strdup'd into the struct as well.

(cherry picked from commit dad9988)
Don't just free the spec vector, also free the specs themselves.

(cherry picked from commit d285de7)
Before resetting the url and username, ensure that we free them in case
they were set by environment variables.

(cherry picked from commit e84914f)
When quick-parsing a commit, we use `git__strtol64` to parse the
commit's time. The buffer that's passed to `commit_quick_parse` is the
raw data of an ODB object, though, whose data may not be properly
formatted and also does not have to be `NUL` terminated. This may lead
to out-of-bound reads.

Use `git__strntol64` to avoid this problem.

(cherry picked from commit 1a3fa1f)
We use `git__strtol64` to parse file modes of the index entries, which
does not limit the parsed buffer length. As the index can be essentially
treated as "untrusted" in that the data stems from the file system, it
may be misformatted and may not contain terminating `NUL` bytes. This
may lead to out-of-bounds reads when trying to parse index entries with
such malformatted modes.

Fix the issue by using `git__strntol64` instead.

(cherry picked from commit 600cead)
We use `git__strtol64` and `git__strtol32` to parse the trailing commit
or author date and timezone of signatures. As signatures are usually
part of a commit or tag object and thus essentially untrusted data, the
buffer may be misformatted and may not be `NUL` terminated. This may
lead to an out-of-bounds read.

Fix the issue by using `git__strntol64` and `git__strntol32` instead.

(cherry picked from commit 3db9aa6)
When parsing integers from configuration values, we use `git__strtol64`.
This is fine to do, as we always sanitize values and can thus be sure
that they'll have a terminating `NUL` byte. But as this is the last
call-site of `git__strtol64`, let's just pass in the length explicitly
by calling `strlen` on the value to be able to remove `git__strtol64`
altogether.

(cherry picked from commit 1a2efd1)
The function `git__strtol64` does not take a maximum buffer length as
parameter. This has led to some unsafe usages of this function, and as
such we may consider it as being unsafe to use. As we have now
eradicated all usages of this function, let's remove it completely to
avoid future misuse.

(cherry picked from commit 68deb2c)
We use the `git__strtol32` function to parse the child and entry count
of treecaches from the index, which do not accept a buffer length. As
the buffer that is being passed in is untrusted data and may thus be
malformed and may not contain a terminating `NUL` byte, we can overrun
the buffer and thus perform an out-of-bounds read.

Fix the issue by uzing `git__strntol32` instead.

(cherry picked from commit 21652ee)
Replace remaining uses of the `git__strtol32` function. While these uses
are all safe as the strings were either sanitized or from a trusted
source, we want to remove `git__strtol32` altogether to avoid future
misuse.

(cherry picked from commit 2613fbb)
The function `git__strtol32` can easily be misused when untrusted data
is passed to it that may not have been sanitized with trailing `NUL`
bytes. As all usages of this function have now been removed, we can
remove this function altogether to avoid future misuse of it.

(cherry picked from commit 8d7fa88)
Some edge cases were currently completely untested, e.g. parsing numbers
greater than INT64_{MIN,MAX}, truncating buffers by length and invalid
characters. Add tests to verify that the system under test performs as
expected.

(cherry picked from commit 39087ab)
While `git__strntol64` tries to detect integer overflows when doing the
necessary arithmetics to come up with the final result, it does the
detection only after the fact. This check thus relies on undefined
behavior of signed integer overflows. Fix this by instead checking
up-front whether the multiplications or additions will overflow.

Note that a detected overflow will not cause us to abort parsing the
current sequence of digits. In the case of an overflow, previous
behavior was to still set up the end pointer correctly to point to the
first character immediately after the currently parsed number. We do not
want to change this now as code may rely on the end pointer being set up
correctly even if the parsed number is too big to be represented as
64 bit integer.

(cherry picked from commit b09c1c7)
When an integer that is parsed with `git__strntol32` is too big to fit
into an int32, we will generate an error message that includes the
actual string that failed to parse. This does not acknowledge the fact
that the string may either not be NUL terminated or alternative include
additional characters after the number that is to be parsed. We may thus
end up printing characters into the buffer that aren't the number or,
worse, read out of bounds.

Fix the issue by utilizing the `endptr` that was set by
`git__strntol64`. This pointer is guaranteed to be set to the first
character following the number, and we can thus use it to compute the
width of the number that shall be printed. Create a test to verify that
we correctly truncate the number.

(cherry picked from commit ea19efc)
Unfortunately, neither the `memmem` nor the `strnstr` functions are part
of any C standard but are merely extensions of C that are implemented by
e.g. glibc. Thus, there is no standardized way to search for a string in
a block of memory with a limited size, and using `strstr` is to be
considered unsafe in case where the buffer has not been sanitized. In
fact, there are some uses of `strstr` in exactly that unsafe way in our
codebase.

Provide a new function `git__memmem` that implements the `memmem`
semantics. That is in a given haystack of `n` bytes, search for the
occurrence of a byte sequence of `m` bytes and return a pointer to the
first occurrence. The implementation chosen is the "Not So Naive"
algorithm from [1]. It was chosen as the implementation is comparably
simple while still being reasonably efficient in most cases.
Preprocessing happens in constant time and space, searching has a time
complexity of O(n*m) with a slightly sub-linear average case.

[1]: http://www-igm.univ-mlv.fr/~lecroq/string/

(cherry picked from commit 83e8a6b)
When parsing tags, we skip all unknown fields that appear before the tag
message. This skipping is done by using a plain `strstr(buffer, "\n\n")`
to search for the two newlines that separate tag fields from tag
message. As it is not possible to supply a buffer length to `strstr`,
this call may skip over the buffer's end and thus result in an out of
bounds read. As `strstr` may return a pointer that is out of bounds, the
following computation of `buffer_end - buffer` will overflow and result
in an allocation of an invalid length.

Fix the issue by using `git__memmem` instead. Add a test that verifies
parsing the tag fails not due to the allocation failure but due to the
tag having no message.

(cherry picked from commit ee11d47)
The commit message encoding is currently being parsed by the
`git__prefixcmp` function. As this function does not accept a buffer
length, it will happily skip over a buffer's end if it is not `NUL`
terminated.

Fix the issue by using `git__prefixncmp` instead. Add a test that
verifies that we are unable to parse the encoding field if it's cut off
by the supplied buffer length.

(cherry picked from commit 7655b2d)
@pks-t
pks-t merged commit 32dc763 into libgit2:maint/v0.26 Oct 26, 2018
@pks-t
pks-t deleted the pks/v0.26.8 branch October 26, 2018 12:54
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.

5 participants