From 417fb91b5d193c58a9a31ad6c7901ccfc08ff760 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 24 May 2022 00:23:03 +0000 Subject: [PATCH 01/28] compat/win32/syslog: fix use-after-realloc Git for Windows' SDK recently upgraded to GCC v12.x which points out that the `pos` variable might be used even after the corresponding memory was `realloc()`ed and therefore potentially no longer valid. Since a subset of this SDK is used in Git's CI/PR builds, we need to fix this to continue to be able to benefit from the CI/PR runs. Note: This bug has been with us since 2a6b149c64f6 (mingw: avoid using strbuf in syslog, 2011-10-06), and while it looks tempting to replace the hand-rolled string manipulation with a `strbuf`-based one, that commit's message explains why we cannot do that: The `syslog()` function is called as part of the function in `daemon.c` which is set as the `die()` routine, and since `strbuf_grow()` can call that function if it runs out of memory, this would cause a nasty infinite loop that we do not want to re-introduce. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- compat/win32/syslog.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c index 161978d720..1f8d8934cc 100644 --- a/compat/win32/syslog.c +++ b/compat/win32/syslog.c @@ -43,6 +43,7 @@ void syslog(int priority, const char *fmt, ...) va_end(ap); while ((pos = strstr(str, "%1")) != NULL) { + size_t offset = pos - str; char *oldstr = str; str = realloc(str, st_add(++str_len, 1)); if (!str) { @@ -50,6 +51,7 @@ void syslog(int priority, const char *fmt, ...) warning_errno("realloc failed"); return; } + pos = str + offset; memmove(pos + 2, pos + 1, strlen(pos)); pos[1] = ' '; } From c03ffcff4e84f4d1e05a86540cd541c759242fc8 Mon Sep 17 00:00:00 2001 From: Jiang Xin Date: Fri, 25 Nov 2022 17:59:51 +0800 Subject: [PATCH 02/28] github-actions: run gcc-8 on ubuntu-20.04 image MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub starts to upgrade its runner image "ubuntu-latest" from version "ubuntu-20.04" to version "ubuntu-22.04". It will fail to find and install "gcc-8" package on the new runner image. Change the runner image of the `linux-gcc` job from "ubuntu-latest" to "ubuntu-20.04" in order to install "gcc-8" as a dependency. Reviewed-by: Johannes Schindelin Helped-by: Ævar Arnfjörð Bjarmason Signed-off-by: Jiang Xin Signed-off-by: Junio C Hamano --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index a3fbbe6398..836d2c65c1 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -282,7 +282,7 @@ jobs: pool: ubuntu-latest - jobname: linux-gcc cc: gcc - pool: ubuntu-latest + pool: ubuntu-20.04 - jobname: osx-clang cc: clang pool: macos-latest From 20854bc47ae071d54f8aa71de948da5084e3adc7 Mon Sep 17 00:00:00 2001 From: Jiang Xin Date: Fri, 25 Nov 2022 17:59:52 +0800 Subject: [PATCH 03/28] ci: remove the pipe after "p4 -V" to catch errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When installing p4 as a dependency, we used to pipe output of "p4 -V" and "p4d -V" to validate the installation and output a condensed version information. But this would hide potential errors of p4 and would stop with an empty output. E.g.: p4d version 16.2 running on ubuntu 22.04 causes sigfaults, even before it produces any output. By removing the pipe after "p4 -V" and "p4d -V", we may get a verbose output, and stop immediately on errors because we have "set -e" in "ci/lib.sh". Since we won't look at these trace logs unless something fails, just including the raw output seems most sensible. Reviewed-by: Johannes Schindelin Helped-by: Ævar Arnfjörð Bjarmason Signed-off-by: Jiang Xin Signed-off-by: Junio C Hamano --- ci/install-dependencies.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh index 0b1184e04a..309b8b164e 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -81,9 +81,9 @@ esac if type p4d >/dev/null && type p4 >/dev/null then echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)" - p4d -V | grep Rev. + p4d -V echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)" - p4 -V | grep Rev. + p4 -V fi if type git-lfs >/dev/null then From 79e0626b39a4aa3754ec0719b3f67296ad39e724 Mon Sep 17 00:00:00 2001 From: Jiang Xin Date: Fri, 25 Nov 2022 17:59:53 +0800 Subject: [PATCH 04/28] ci: use the same version of p4 on both Linux and macOS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There would be a segmentation fault when running p4 v16.2 on ubuntu 22.04 which is the latest version of ubuntu runner image for github actions. By checking each version from [1], p4d version 21.1 and above can work properly on ubuntu 22.04. But version 22.x will break some p4 test cases. So p4 version 21.x is exactly the version we can use. With this update, the versions of p4 for Linux and macOS happen to be the same. So we can add the version number directly into the "P4WHENCE" variable, and reuse it in p4 installation for macOS. By removing the "LINUX_P4_VERSION" variable from "ci/lib.sh", the comment left above has nothing to do with p4, but still applies to git-lfs. Since we have a fixed version of git-lfs installed on Linux, we may have a different version on macOS. [1]: https://cdist2.perforce.com/perforce/ Reviewed-by: Johannes Schindelin Helped-by: Ævar Arnfjörð Bjarmason Signed-off-by: Jiang Xin Signed-off-by: Junio C Hamano --- ci/install-dependencies.sh | 18 ++++++++++-------- ci/lib.sh | 1 - 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh index 309b8b164e..19c5e8735d 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -5,7 +5,7 @@ . ${0%/*}/lib.sh -P4WHENCE=http://filehost.perforce.com/perforce/r$LINUX_P4_VERSION +P4WHENCE=https://cdist2.perforce.com/perforce/r21.2 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl @@ -44,13 +44,15 @@ osx-clang|osx-gcc) test -z "$BREW_INSTALL_PACKAGES" || brew install $BREW_INSTALL_PACKAGES brew link --force gettext - brew install --cask --no-quarantine perforce || { - # Update the definitions and try again - cask_repo="$(brew --repository)"/Library/Taps/homebrew/homebrew-cask && - git -C "$cask_repo" pull --no-stat --ff-only && - brew install --cask --no-quarantine perforce - } || - brew install homebrew/cask/perforce + mkdir -p $HOME/bin + ( + cd $HOME/bin + wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" && + tar -xf helix-core-server.tgz && + sudo xattr -d com.apple.quarantine p4 p4d 2>/dev/null || true + ) + PATH="$PATH:${HOME}/bin" + export PATH case "$jobname" in osx-gcc) brew install gcc@9 diff --git a/ci/lib.sh b/ci/lib.sh index 38c0eac351..6c14766431 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -199,7 +199,6 @@ linux-clang|linux-gcc) # were recorded in the Homebrew database upon creating the OS X # image. # Keep that in mind when you encounter a broken OS X build! - export LINUX_P4_VERSION="16.2" export LINUX_GIT_LFS_VERSION="1.5.2" P4_PATH="$HOME/custom/p4" From 86f6f4fa91a9a2310c8f73528a86bf2c52d47fe4 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 24 May 2022 00:23:04 +0000 Subject: [PATCH 05/28] nedmalloc: avoid new compile error GCC v12.x complains thusly: compat/nedmalloc/nedmalloc.c: In function 'DestroyCaches': compat/nedmalloc/nedmalloc.c:326:12: error: the comparison will always evaluate as 'true' for the address of 'caches' will never be NULL [-Werror=address] 326 | if(p->caches) | ^ compat/nedmalloc/nedmalloc.c:196:22: note: 'caches' declared here 196 | threadcache *caches[THREADCACHEMAXCACHES]; | ^~~~~~ ... and it is correct, of course. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- compat/nedmalloc/nedmalloc.c | 1 - 1 file changed, 1 deletion(-) diff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c index 1cc31c3502..141c30f07f 100644 --- a/compat/nedmalloc/nedmalloc.c +++ b/compat/nedmalloc/nedmalloc.c @@ -323,7 +323,6 @@ static NOINLINE void RemoveCacheEntries(nedpool *p, threadcache *tc, unsigned in } static void DestroyCaches(nedpool *p) THROWSPEC { - if(p->caches) { threadcache *tc; int n; From fda237cb648b3812f694bdbe899449247610415b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 16 Jan 2023 22:04:38 -0500 Subject: [PATCH 06/28] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT The two options do exactly the same thing, but the latter has been deprecated and in recent versions of curl may produce a compiler warning. Since the UPLOAD form is available everywhere (it was introduced in the year 2000 by curl 7.1), we can just switch to it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- http-push.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http-push.c b/http-push.c index 6a4a43e07f..585fbd57e3 100644 --- a/http-push.c +++ b/http-push.c @@ -198,7 +198,7 @@ static void curl_setup_http(CURL *curl, const char *url, const char *custom_req, struct buffer *buffer, curl_write_callback write_fn) { - curl_easy_setopt(curl, CURLOPT_PUT, 1); + curl_easy_setopt(curl, CURLOPT_UPLOAD, 1); curl_easy_setopt(curl, CURLOPT_URL, url); curl_easy_setopt(curl, CURLOPT_INFILE, buffer); curl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len); From b0e3e2d06ba3b71079d7b9b6402a0f410180c09b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 16 Jan 2023 22:04:44 -0500 Subject: [PATCH 07/28] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION The IOCTLFUNCTION option has been deprecated, and generates a compiler warning in recent versions of curl. We can switch to using SEEKFUNCTION instead. It was added in 2008 via curl 7.18.0; our INSTALL file already indicates we require at least curl 7.19.4. But there's one catch: curl says we should use CURL_SEEKFUNC_{OK,FAIL}, and those didn't arrive until 7.19.5. One workaround would be to use a bare 0/1 here (or define our own macros). But let's just bump the minimum required version to 7.19.5. That version is only a minor version bump from our existing requirement, and is only a 2 month time bump for versions that are almost 13 years old. So it's not likely that anybody cares about the distinction. Switching means we have to rewrite the ioctl functions into seek functions. In some ways they are simpler (seeking is the only operation), but in some ways more complex (the ioctl allowed only a full rewind, but now we can seek to arbitrary offsets). Curl will only ever use SEEK_SET (per their documentation), so I didn't bother implementing anything else, since it would naturally be completely untested. This seems unlikely to change, but I added an assertion just in case. Likewise, I doubt curl will ever try to seek outside of the buffer sizes we've told it, but I erred on the defensive side here, rather than do an out-of-bounds read. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- INSTALL | 4 ++++ http-push.c | 6 +++--- http.c | 22 ++++++++++------------ http.h | 8 ++++---- remote-curl.c | 32 +++++++++++++++----------------- 5 files changed, 36 insertions(+), 36 deletions(-) diff --git a/INSTALL b/INSTALL index 8474ad01bf..88c8b3cd46 100644 --- a/INSTALL +++ b/INSTALL @@ -145,6 +145,10 @@ Issues of note: patches into an IMAP mailbox, you do not have to have them (use NO_CURL). + Git requires version "7.19.5" or later of "libcurl" to build + without NO_CURL. This version requirement may be bumped in + the future. + - "expat" library; git-http-push uses it for remote lock management over DAV. Similar to "curl" above, this is optional (with NO_EXPAT). diff --git a/http-push.c b/http-push.c index 585fbd57e3..756779bb19 100644 --- a/http-push.c +++ b/http-push.c @@ -203,9 +203,9 @@ static void curl_setup_http(CURL *curl, const char *url, curl_easy_setopt(curl, CURLOPT_INFILE, buffer); curl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len); curl_easy_setopt(curl, CURLOPT_READFUNCTION, fread_buffer); -#ifndef NO_CURL_IOCTL - curl_easy_setopt(curl, CURLOPT_IOCTLFUNCTION, ioctl_buffer); - curl_easy_setopt(curl, CURLOPT_IOCTLDATA, buffer); +#ifndef NO_CURL_SEEK + curl_easy_setopt(curl, CURLOPT_SEEKFUNCTION, seek_buffer); + curl_easy_setopt(curl, CURLOPT_SEEKDATA, buffer); #endif curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_fn); curl_easy_setopt(curl, CURLOPT_NOBODY, 0); diff --git a/http.c b/http.c index 8b23a546af..b228137388 100644 --- a/http.c +++ b/http.c @@ -186,22 +186,20 @@ size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) return size / eltsize; } -#ifndef NO_CURL_IOCTL -curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp) +#ifndef NO_CURL_SEEK +int seek_buffer(void *clientp, curl_off_t offset, int origin) { struct buffer *buffer = clientp; - switch (cmd) { - case CURLIOCMD_NOP: - return CURLIOE_OK; - - case CURLIOCMD_RESTARTREAD: - buffer->posn = 0; - return CURLIOE_OK; - - default: - return CURLIOE_UNKNOWNCMD; + if (origin != SEEK_SET) + BUG("seek_buffer only handles SEEK_SET"); + if (offset < 0 || offset >= buffer->buf.len) { + error("curl seek would be outside of buffer"); + return CURL_SEEKFUNC_FAIL; } + + buffer->posn = offset; + return CURL_SEEKFUNC_OK; } #endif diff --git a/http.h b/http.h index 5de792ef3f..90679e1410 100644 --- a/http.h +++ b/http.h @@ -41,8 +41,8 @@ #define CURLE_HTTP_RETURNED_ERROR CURLE_HTTP_NOT_FOUND #endif -#if LIBCURL_VERSION_NUM < 0x070c03 -#define NO_CURL_IOCTL +#if LIBCURL_VERSION_NUM < 0x071200 +#define NO_CURL_SEEK #endif /* @@ -82,8 +82,8 @@ struct buffer { size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf); size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf); size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf); -#ifndef NO_CURL_IOCTL -curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp); +#ifndef NO_CURL_SEEK +int seek_buffer(void *clientp, curl_off_t offset, int origin); #endif /* Slot lifecycle functions */ diff --git a/remote-curl.c b/remote-curl.c index 0290b04891..f50531624a 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -707,26 +707,24 @@ static size_t rpc_out(void *ptr, size_t eltsize, return avail; } -#ifndef NO_CURL_IOCTL -static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp) +#ifndef NO_CURL_SEEK +static int rpc_seek(void *clientp, curl_off_t offset, int origin) { struct rpc_state *rpc = clientp; - switch (cmd) { - case CURLIOCMD_NOP: - return CURLIOE_OK; + if (origin != SEEK_SET) + BUG("rpc_seek only handles SEEK_SET, not %d", origin); - case CURLIOCMD_RESTARTREAD: - if (rpc->initial_buffer) { - rpc->pos = 0; - return CURLIOE_OK; + if (rpc->initial_buffer) { + if (offset < 0 || offset > rpc->len) { + error("curl seek would be outside of rpc buffer"); + return CURL_SEEKFUNC_FAIL; } - error(_("unable to rewind rpc post data - try increasing http.postBuffer")); - return CURLIOE_FAILRESTART; - - default: - return CURLIOE_UNKNOWNCMD; + rpc->pos = offset; + return CURL_SEEKFUNC_OK; } + error(_("unable to rewind rpc post data - try increasing http.postBuffer")); + return CURL_SEEKFUNC_FAIL; } #endif @@ -947,9 +945,9 @@ retry: rpc->initial_buffer = 1; curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out); curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc); -#ifndef NO_CURL_IOCTL - curl_easy_setopt(slot->curl, CURLOPT_IOCTLFUNCTION, rpc_ioctl); - curl_easy_setopt(slot->curl, CURLOPT_IOCTLDATA, rpc); +#ifndef NO_CURL_SEEK + curl_easy_setopt(slot->curl, CURLOPT_SEEKFUNCTION, rpc_seek); + curl_easy_setopt(slot->curl, CURLOPT_SEEKDATA, rpc); #endif if (options.verbosity > 1) { fprintf(stderr, "POST %s (chunked)\n", rpc->service_name); From 18bc8eb7b51f9249fd2d57aa09f2aa959dae14a2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 9 Aug 2021 18:47:42 -0400 Subject: [PATCH 08/28] range-diff: drop useless "offset" variable from read_patches() The "offset" variable was was introduced in 44b67cb62b (range-diff: split lines manually, 2019-07-11), but it has never done anything useful. We use it to count up the number of bytes we've consumed, but we never look at the result. It was probably copied accidentally from an almost-identical loop in apply.c:find_header() (and the point of that commit was to make use of the parse_git_diff_header() function which underlies both). Because the variable was set but not used, most compilers didn't seem to notice, but the upcoming clang-14 does complain about it, via its -Wunused-but-set-variable warning. Signed-off-by: Jeff King Acked-by: Derrick Stolee Signed-off-by: Junio C Hamano --- range-diff.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/range-diff.c b/range-diff.c index b9950f10c8..559a9291f9 100644 --- a/range-diff.c +++ b/range-diff.c @@ -48,7 +48,7 @@ static int read_patches(const char *range, struct string_list *list, struct patch_util *util = NULL; int in_header = 1; char *line, *current_filename = NULL; - int offset, len; + int len; size_t size; strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges", @@ -83,7 +83,7 @@ static int read_patches(const char *range, struct string_list *list, line = contents.buf; size = contents.len; - for (offset = 0; size > 0; offset += len, size -= len, line += len) { + for (; size > 0; size -= len, line += len) { const char *p; len = find_end_of_line(line, size); From a69043d51083151c538c3f2da0fc312f8af35edd Mon Sep 17 00:00:00 2001 From: Jiang Xin Date: Fri, 25 Nov 2022 17:59:54 +0800 Subject: [PATCH 09/28] ci: install python on ubuntu MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Python is missing from the default ubuntu-22.04 runner image, which prevents git-p4 from working. To install python on ubuntu, we need to provide the correct package names: * On Ubuntu 18.04 (bionic), "/usr/bin/python2" is provided by the "python" package, and "/usr/bin/python3" is provided by the "python3" package. * On Ubuntu 20.04 (focal) and above, "/usr/bin/python2" is provided by the "python2" package which has a different name from bionic, and "/usr/bin/python3" is provided by "python3". Since the "ubuntu-latest" runner image has a higher version, its safe to use "python2" or "python3" package name. Helped-by: Ævar Arnfjörð Bjarmason Signed-off-by: Jiang Xin Signed-off-by: Junio C Hamano --- ci/install-dependencies.sh | 2 +- ci/lib.sh | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh index 19c5e8735d..2d54427175 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -16,7 +16,7 @@ linux-clang|linux-gcc) sudo apt-add-repository -y "ppa:ubuntu-toolchain-r/test" sudo apt-get -q update sudo apt-get -q -y install language-pack-is libsvn-perl apache2 \ - $UBUNTU_COMMON_PKGS + $UBUNTU_COMMON_PKGS $PYTHON_PACKAGE case "$jobname" in linux-gcc) sudo apt-get -q -y install gcc-8 diff --git a/ci/lib.sh b/ci/lib.sh index 6c14766431..702ea96a38 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -184,13 +184,13 @@ export SKIP_DASHED_BUILT_INS=YesPlease case "$jobname" in linux-clang|linux-gcc) + PYTHON_PACKAGE=python2 if [ "$jobname" = linux-gcc ] then export CC=gcc-8 - MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python3" - else - MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python2" + PYTHON_PACKAGE=python3 fi + MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/$PYTHON_PACKAGE" export GIT_TEST_HTTPD=true From 07f91e5e79810a8f17de745d2d84c384add75f0a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 16 Jan 2023 22:04:48 -0500 Subject: [PATCH 10/28] http: support CURLOPT_PROTOCOLS_STR The CURLOPT_PROTOCOLS (and matching CURLOPT_REDIR_PROTOCOLS) flag was deprecated in curl 7.85.0, and using it generate compiler warnings as of curl 7.87.0. The path forward is to use CURLOPT_PROTOCOLS_STR, but we can't just do so unilaterally, as it was only introduced less than a year ago in 7.85.0. Until that version becomes ubiquitous, we have to either disable the deprecation warning or conditionally use the "STR" variant on newer versions of libcurl. This patch switches to the new variant, which is nice for two reasons: - we don't have to worry that silencing curl's deprecation warnings might cause us to miss other more useful ones - we'd eventually want to move to the new variant anyway, so this gets us set up (albeit with some extra ugly boilerplate for the conditional) There are a lot of ways to split up the two cases. One way would be to abstract the storage type (strbuf versus a long), how to append (strbuf_addstr vs bitwise OR), how to initialize, which CURLOPT to use, and so on. But the resulting code looks pretty magical: GIT_CURL_PROTOCOL_TYPE allowed = GIT_CURL_PROTOCOL_TYPE_INIT; if (...http is allowed...) GIT_CURL_PROTOCOL_APPEND(&allowed, "http", CURLOPT_HTTP); and you end up with more "#define GIT_CURL_PROTOCOL_TYPE" macros than actual code. On the other end of the spectrum, we could just implement two separate functions, one that handles a string list and one that handles bits. But then we end up repeating our list of protocols (http, https, ftp, ftp). This patch takes the middle ground. The run-time code is always there to handle both types, and we just choose which one to feed to curl. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- http.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 13 deletions(-) diff --git a/http.c b/http.c index b228137388..a42de7a751 100644 --- a/http.c +++ b/http.c @@ -808,20 +808,37 @@ void setup_curl_trace(CURL *handle) } #ifdef CURLPROTO_HTTP -static long get_curl_allowed_protocols(int from_user) +static void proto_list_append(struct strbuf *list, const char *proto) { - long allowed_protocols = 0; + if (!list) + return; + if (list->len) + strbuf_addch(list, ','); + strbuf_addstr(list, proto); +} + +static long get_curl_allowed_protocols(int from_user, struct strbuf *list) +{ + long bits = 0; - if (is_transport_allowed("http", from_user)) - allowed_protocols |= CURLPROTO_HTTP; - if (is_transport_allowed("https", from_user)) - allowed_protocols |= CURLPROTO_HTTPS; - if (is_transport_allowed("ftp", from_user)) - allowed_protocols |= CURLPROTO_FTP; - if (is_transport_allowed("ftps", from_user)) - allowed_protocols |= CURLPROTO_FTPS; + if (is_transport_allowed("http", from_user)) { + bits |= CURLPROTO_HTTP; + proto_list_append(list, "http"); + } + if (is_transport_allowed("https", from_user)) { + bits |= CURLPROTO_HTTPS; + proto_list_append(list, "https"); + } + if (is_transport_allowed("ftp", from_user)) { + bits |= CURLPROTO_FTP; + proto_list_append(list, "ftp"); + } + if (is_transport_allowed("ftps", from_user)) { + bits |= CURLPROTO_FTPS; + proto_list_append(list, "ftps"); + } - return allowed_protocols; + return bits; } #endif @@ -979,10 +996,24 @@ static CURL *get_curl_handle(void) curl_easy_setopt(result, CURLOPT_POST301, 1); #endif #ifdef CURLPROTO_HTTP +#if LIBCURL_VERSION_NUM >= 0x075500 + { + struct strbuf buf = STRBUF_INIT; + + get_curl_allowed_protocols(0, &buf); + curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS_STR, buf.buf); + strbuf_reset(&buf); + + get_curl_allowed_protocols(-1, &buf); + curl_easy_setopt(result, CURLOPT_PROTOCOLS_STR, buf.buf); + strbuf_release(&buf); + } +#else curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, - get_curl_allowed_protocols(0)); + get_curl_allowed_protocols(0, NULL)); curl_easy_setopt(result, CURLOPT_PROTOCOLS, - get_curl_allowed_protocols(-1)); + get_curl_allowed_protocols(-1, NULL)); +#endif #else warning(_("Protocol restrictions not supported with cURL < 7.19.4")); #endif From 8516dac1e1836843dd6a3dbc1d140a8b6d885ecb Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 9 Mar 2023 22:54:06 +0100 Subject: [PATCH 11/28] t0033: GETTEXT_POISON fix In e47363e5a8bd (t0033: add tests for safe.directory, 2022-04-13), we backported a patch onto v2.30.* that was originally based on a much newer version. The v2.30.* release train still has the GETTEXT_POISON CI job, though, and hence needs `test_i18n*` in its tests. Signed-off-by: Johannes Schindelin --- t/t0033-safe-directory.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh index 239d93f4d2..00c6c51100 100755 --- a/t/t0033-safe-directory.sh +++ b/t/t0033-safe-directory.sh @@ -9,7 +9,7 @@ export GIT_TEST_ASSUME_DIFFERENT_OWNER expect_rejected_dir () { test_must_fail git status 2>err && - grep "safe.directory" err + test_i18ngrep "safe.directory" err } test_expect_success 'safe.directory is not set' ' From e4298ccd7ff74cd2e11ff952b42c33ccc746e617 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 9 Mar 2023 22:51:43 +0100 Subject: [PATCH 12/28] t0003: GETTEXT_POISON fix, part 1 In dfa6b32b5e59 (attr: ignore attribute lines exceeding 2048 bytes, 2022-12-01), we backported a patch onto v2.30.* that was originally based on a much newer version. The v2.30.* release train still has the GETTEXT_POISON CI job, though, and hence needs `test_i18n*` in its tests. Signed-off-by: Johannes Schindelin --- t/t0003-attributes.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh index 9d9aa2855d..5bd9d9832d 100755 --- a/t/t0003-attributes.sh +++ b/t/t0003-attributes.sh @@ -344,7 +344,7 @@ test_expect_success 'large attributes line ignored in tree' ' printf "path %02043d" 1 >.gitattributes && git check-attr --all path >actual 2>err && echo "warning: ignoring overly long attributes line 1" >expect && - test_cmp expect err && + test_i18ncmp expect err && test_must_be_empty actual ' @@ -357,7 +357,7 @@ test_expect_success 'large attributes line ignores trailing content in tree' ' printf "a %02045dtrailing attribute\n" 1 >.gitattributes && git check-attr --all trailing >actual 2>err && echo "warning: ignoring overly long attributes line 1" >expect && - test_cmp expect err && + test_i18ncmp expect err && test_must_be_empty actual ' @@ -375,7 +375,7 @@ test_expect_success 'large attributes line ignored in index' ' git update-index --add --cacheinfo 100644,$blob,.gitattributes && git check-attr --cached --all path >actual 2>err && echo "warning: ignoring overly long attributes line 1" >expect && - test_cmp expect err && + test_i18ncmp expect err && test_must_be_empty actual ' @@ -385,7 +385,7 @@ test_expect_success 'large attributes line ignores trailing content in index' ' git update-index --add --cacheinfo 100644,$blob,.gitattributes && git check-attr --cached --all trailing >actual 2>err && echo "warning: ignoring overly long attributes line 1" >expect && - test_cmp expect err && + test_i18ncmp expect err && test_must_be_empty actual ' From a36df79a37c7c643177905ce725dca8e9bd092d3 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 9 Aug 2021 18:48:39 -0400 Subject: [PATCH 13/28] range-diff: handle unterminated lines in read_patches() When parsing our buffer of output from git-log, we have a find_end_of_line() helper that finds the next newline, and gives us the number of bytes to move past it, or the size of the whole remaining buffer if there is no newline. But trying to handle both those cases leads to some oddities: - we try to overwrite the newline with NUL in the caller, by writing over line[len-1]. This is at best redundant, since the helper will already have done so if it saw a newline. But if it didn't see a newline, it's actively wrong; we'll overwrite the byte at the end of the (unterminated) line. We could solve this just dropping the extra NUL assignment in the caller and just letting the helper do the right thing. But... - if we see a "diff --git" line, we'll restore the newline on top of the NUL byte, so we can pass the string to parse_git_diff_header(). But if there was no newline in the first place, we can't do this. There's no place to put it (the current code writes a newline over whatever byte we obliterated earlier). The best we can do is feed the complete remainder of the buffer to the function (which is, in fact, a string, by virtue of being a strbuf). To solve this, the caller needs to know whether we actually found a newline or not. We could modify find_end_of_line() to return that information, but we can further observe that it has only one caller. So let's just inline it in that caller. Nobody seems to have noticed this case, probably because git-log would never produce input that doesn't end with a newline. Arguably we could just return an error as soon as we see that the output does not end in a newline. But the code to do so actually ends up _longer_, mostly because of the cleanup we have to do in handling the error. Signed-off-by: Jeff King Acked-by: Derrick Stolee Signed-off-by: Junio C Hamano --- range-diff.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/range-diff.c b/range-diff.c index 559a9291f9..f64e84f56a 100644 --- a/range-diff.c +++ b/range-diff.c @@ -25,17 +25,6 @@ struct patch_util { struct object_id oid; }; -static size_t find_end_of_line(char *buffer, unsigned long size) -{ - char *eol = memchr(buffer, '\n', size); - - if (!eol) - return size; - - *eol = '\0'; - return eol + 1 - buffer; -} - /* * Reads the patches into a string list, with the `util` field being populated * as struct object_id (will need to be free()d). @@ -85,9 +74,16 @@ static int read_patches(const char *range, struct string_list *list, size = contents.len; for (; size > 0; size -= len, line += len) { const char *p; + char *eol; + + eol = memchr(line, '\n', size); + if (eol) { + *eol = '\0'; + len = eol + 1 - line; + } else { + len = size; + } - len = find_end_of_line(line, size); - line[len - 1] = '\0'; if (skip_prefix(line, "commit ", &p)) { if (util) { string_list_append(list, buf.buf)->util = util; @@ -129,7 +125,8 @@ static int read_patches(const char *range, struct string_list *list, strbuf_addch(&buf, '\n'); if (!util->diff_offset) util->diff_offset = buf.len; - line[len - 1] = '\n'; + if (eol) + *eol = '\n'; orig_len = len; len = parse_git_diff_header(&root, &linenr, 0, line, len, size, &patch); From d99728b2cafb3346679cb8733221c41149fdc760 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 9 Mar 2023 22:52:41 +0100 Subject: [PATCH 14/28] t0003: GETTEXT_POISON fix, conclusion In 3c50032ff528 (attr: ignore overly large gitattributes files, 2022-12-01), we backported a patch onto v2.30.* that was originally based on a much newer version. The v2.30.* release train still has the GETTEXT_POISON CI job, though, and hence needs `test_i18n*` in its tests. Signed-off-by: Johannes Schindelin --- t/t0003-attributes.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh index 5bd9d9832d..57ba303de8 100755 --- a/t/t0003-attributes.sh +++ b/t/t0003-attributes.sh @@ -366,7 +366,7 @@ test_expect_success EXPENSIVE 'large attributes file ignored in tree' ' dd if=/dev/zero of=.gitattributes bs=101M count=1 2>/dev/null && git check-attr --all path >/dev/null 2>err && echo "warning: ignoring overly large gitattributes file ${SQ}.gitattributes${SQ}" >expect && - test_cmp expect err + test_i18ncmp expect err ' test_expect_success 'large attributes line ignored in index' ' From c025b4b2f1ee0cd9eed2e900c03780683294bb18 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 9 Aug 2021 18:48:48 -0400 Subject: [PATCH 15/28] range-diff: use ssize_t for parsed "len" in read_patches() As we iterate through the buffer containing git-log output, parsing lines, we use an "int" to store the size of an individual line. This should be a size_t, as we have no guarantee that there is not a malicious 2GB+ commit-message line in the output. Overflowing this integer probably doesn't do anything _too_ terrible. We are not using the value to size a buffer, so the worst case is probably an out-of-bounds read from before the array. But it's easy enough to fix. Note that we have to use ssize_t here, since we also store the length result from parse_git_diff_header(), which may return a negative value for error. That function actually returns an int itself, which has a similar overflow problem, but I'll leave that for another day. Much of the apply.c code uses ints and should be converted as a whole; in the meantime, a negative return from parse_git_diff_header() will be interpreted as an error, and we'll bail (so we can't handle such a case, but given that it's likely to be malicious anyway, the important thing is we don't have any memory errors). Signed-off-by: Jeff King Acked-by: Derrick Stolee Signed-off-by: Junio C Hamano --- range-diff.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/range-diff.c b/range-diff.c index f64e84f56a..9e2b788cdf 100644 --- a/range-diff.c +++ b/range-diff.c @@ -37,7 +37,7 @@ static int read_patches(const char *range, struct string_list *list, struct patch_util *util = NULL; int in_header = 1; char *line, *current_filename = NULL; - int len; + ssize_t len; size_t size; strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges", From a2b2173cfeed82347711862d20aabbb1a5478379 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 9 Mar 2023 22:56:06 +0100 Subject: [PATCH 16/28] t5619: GETTEXT_POISON fix In cf8f6ce02a13 (clone: delay picking a transport until after get_repo_path(), 2023-01-24), we backported a patch onto v2.30.* that was originally based on a much newer version. The v2.30.* release train still has the GETTEXT_POISON CI job, though, and hence needs `test_i18n*` in its tests. Signed-off-by: Johannes Schindelin --- t/t5619-clone-local-ambiguous-transport.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t5619-clone-local-ambiguous-transport.sh b/t/t5619-clone-local-ambiguous-transport.sh index cce62bf78d..7eab09d02a 100755 --- a/t/t5619-clone-local-ambiguous-transport.sh +++ b/t/t5619-clone-local-ambiguous-transport.sh @@ -64,7 +64,7 @@ test_expect_success 'ambiguous transport does not lead to arbitrary file-inclusi # # This works for now, and if we ever fix the URL detection, it # is OK to change this to detect the transport error. - grep "protocol .* is not supported" err + test_i18ngrep "protocol .* is not supported" err ' test_done From 7c811ed5e55553769f0077ddf026ef17061bd8bd Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 9 Mar 2023 22:55:40 +0100 Subject: [PATCH 17/28] t5604: GETTEXT_POISON fix, part 1 In bffc762f87ae (dir-iterator: prevent top-level symlinks without FOLLOW_SYMLINKS, 2023-01-24), we backported a patch onto v2.30.* that was originally based on a much newer version. The v2.30.* release train still has the GETTEXT_POISON CI job, though, and hence needs `test_i18n*` in its tests. Signed-off-by: Johannes Schindelin --- t/t5604-clone-reference.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh index 4ff21d7ccf..9ba2af0ff7 100755 --- a/t/t5604-clone-reference.sh +++ b/t/t5604-clone-reference.sh @@ -354,7 +354,7 @@ test_expect_success SYMLINKS 'clone repo with symlinked objects directory' ' test_must_fail git clone --local malicious clone 2>err && test_path_is_missing clone && - grep "failed to start iterator over" err + test_i18ngrep "failed to start iterator over" err ' test_done From 0c8d22abaf155d97a0efd04bae9fcdad1de7956e Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 9 Mar 2023 22:54:32 +0100 Subject: [PATCH 18/28] t5604: GETTEXT_POISON fix, conclusion In fade728df122 (apply: fix writing behind newly created symbolic links, 2023-02-02), we backported a patch onto v2.30.* that was originally based on a much newer version. The v2.30.* release train still has the GETTEXT_POISON CI job, though, and hence needs `test_i18n*` in its tests. Signed-off-by: Johannes Schindelin --- t/t4115-apply-symlink.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t4115-apply-symlink.sh b/t/t4115-apply-symlink.sh index 1acb7b2582..14e0f4d705 100755 --- a/t/t4115-apply-symlink.sh +++ b/t/t4115-apply-symlink.sh @@ -72,7 +72,7 @@ test_expect_success SYMLINKS 'symlink escape when creating new files' ' cat >expected_stderr <<-EOF && error: affected file ${SQ}renamed-symlink/create-me${SQ} is beyond a symbolic link EOF - test_cmp expected_stderr stderr && + test_i18ncmp expected_stderr stderr && ! test_path_exists .git/create-me ' From 321854ac464bbc85ff2afe089c4a56505a1c9e25 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 24 May 2022 00:23:06 +0000 Subject: [PATCH 19/28] clone.c: avoid "exceeds maximum object size" error with GCC v12.x Technically, the pointer difference `end - start` _could_ be negative, and when cast to an (unsigned) `size_t` that would cause problems. In this instance, the symptom is: dir.c: In function 'git_url_basename': dir.c:3087:13: error: 'memchr' specified bound [9223372036854775808, 0] exceeds maximum object size 9223372036854775807 [-Werror=stringop-overread] CC ewah/bitmap.o 3087 | if (memchr(start, '/', end - start) == NULL | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ While it is a bit far-fetched to think that `end` (which is defined as `repo + strlen(repo)`) and `start` (which starts at `repo` and never steps beyond the NUL terminator) could result in such a negative difference, GCC has no way of knowing that. See also https://gcc.gnu.org/bugzilla//show_bug.cgi?id=85783. Let's just add a safety check, primarily for GCC's benefit. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/clone.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/builtin/clone.c b/builtin/clone.c index c042b2e256..9ab17cab19 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -250,6 +250,15 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare) end--; } + /* + * It should not be possible to overflow `ptrdiff_t` by passing in an + * insanely long URL, but GCC does not know that and will complain + * without this check. + */ + if (end - start < 0) + die(_("No directory name could be guessed.\n" + "Please specify a directory on the command line")); + /* * Strip trailing port number if we've got only a * hostname (that is, there is no dir separator but a From 5843080c85ae9d13f77442bec7bbec8e84a18100 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 2 Mar 2023 08:44:16 -0800 Subject: [PATCH 20/28] http.c: clear the 'finished' member once we are done with it In http.c, the run_active_slot() function allows the given "slot" to make progress by calling step_active_slots() in a loop repeatedly, and the loop is not left until the request held in the slot completes. Ages ago, we used to use the slot->in_use member to get out of the loop, which misbehaved when the request in "slot" completes (at which time, the result of the request is copied away from the slot, and the in_use member is cleared, making the slot ready to be reused), and the "slot" gets reused to service a different request (at which time, the "slot" becomes in_use again, even though it is for a different request). The loop terminating condition mistakenly thought that the original request has yet to be completed. Today's code, after baa7b67d (HTTP slot reuse fixes, 2006-03-10) fixed this issue, uses a separate "slot->finished" member that is set in run_active_slot() to point to an on-stack variable, and the code that completes the request in finish_active_slot() clears the on-stack variable via the pointer to signal that the particular request held by the slot has completed. It also clears the in_use member (as before that fix), so that the slot itself can safely be reused for an unrelated request. One thing that is not quite clean in this arrangement is that, unless the slot gets reused, at which point the finished member is reset to NULL, the member keeps the value of &finished, which becomes a dangling pointer into the stack when run_active_slot() returns. Clear the finished member before the control leaves the function, which has a side effect of unconfusing compilers like recent GCC 12 that is over-eager to warn against such an assignment. Signed-off-by: Junio C Hamano --- http.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/http.c b/http.c index 8b23a546af..f44209881e 100644 --- a/http.c +++ b/http.c @@ -1523,6 +1523,32 @@ void run_active_slot(struct active_request_slot *slot) finish_active_slot(slot); } #endif + + /* + * The value of slot->finished we set before the loop was used + * to set our "finished" variable when our request completed. + * + * 1. The slot may not have been reused for another requst + * yet, in which case it still has &finished. + * + * 2. The slot may already be in-use to serve another request, + * which can further be divided into two cases: + * + * (a) If call run_active_slot() hasn't been called for that + * other request, slot->finished would have been cleared + * by get_active_slot() and has NULL. + * + * (b) If the request did call run_active_slot(), then the + * call would have updated slot->finished at the beginning + * of this function, and with the clearing of the member + * below, we would find that slot->finished is now NULL. + * + * In all cases, slot->finished has no useful information to + * anybody at this point. Some compilers warn us for + * attempting to smuggle a pointer that is about to become + * invalid, i.e. &finished. We clear it here to assure them. + */ + slot->finished = NULL; } static void release_active_slot(struct active_request_slot *slot) From fef08dd32edb3ca751a51a0b88cf96b84fa79be8 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Tue, 23 Aug 2022 17:28:11 +0000 Subject: [PATCH 21/28] ci: update 'static-analysis' to Ubuntu 22.04 GitHub Actions scheduled a brownout of Ubuntu 18.04, which canceled all runs of the 'static-analysis' job in our CI runs. Update to 22.04 to avoid this as the brownout later turns into a complete deprecation. The use of 18.04 was set in d051ed77ee6 (.github/workflows/main.yml: run static-analysis on bionic, 2021-02-08) due to the lack of Coccinelle being available on 20.04 (which continues today). Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index a3fbbe6398..8c61544f67 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -340,7 +340,7 @@ jobs: if: needs.ci-config.outputs.enabled == 'yes' env: jobname: StaticAnalysis - runs-on: ubuntu-18.04 + runs-on: ubuntu-22.04 steps: - uses: actions/checkout@v1 - run: ci/install-dependencies.sh From 9db05711c98efc14f414d4c87135a34c13586e0b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 9 Mar 2023 16:02:54 +0100 Subject: [PATCH 22/28] apply --reject: overwrite existing `.rej` symlink if it exists The `git apply --reject` is expected to write out `.rej` files in case one or more hunks fail to apply cleanly. Historically, the command overwrites any existing `.rej` files. The idea being that apply/reject/edit cycles are relatively common, and the generated `.rej` files are not considered precious. But the command does not overwrite existing `.rej` symbolic links, and instead follows them. This is unsafe because the same patch could potentially create such a symbolic link and point at arbitrary paths outside the current worktree, and `git apply` would write the contents of the `.rej` file into that location. Therefore, let's make sure that any existing `.rej` file or symbolic link is removed before writing it. Reported-by: RyotaK Helped-by: Taylor Blau Helped-by: Junio C Hamano Helped-by: Linus Torvalds Signed-off-by: Johannes Schindelin --- apply.c | 14 ++++++++++++-- t/t4115-apply-symlink.sh | 15 +++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/apply.c b/apply.c index d80382c940..6634e9c510 100644 --- a/apply.c +++ b/apply.c @@ -4558,7 +4558,7 @@ static int write_out_one_reject(struct apply_state *state, struct patch *patch) FILE *rej; char namebuf[PATH_MAX]; struct fragment *frag; - int cnt = 0; + int fd, cnt = 0; struct strbuf sb = STRBUF_INIT; for (cnt = 0, frag = patch->fragments; frag; frag = frag->next) { @@ -4598,7 +4598,17 @@ static int write_out_one_reject(struct apply_state *state, struct patch *patch) memcpy(namebuf, patch->new_name, cnt); memcpy(namebuf + cnt, ".rej", 5); - rej = fopen(namebuf, "w"); + fd = open(namebuf, O_CREAT | O_EXCL | O_WRONLY, 0666); + if (fd < 0) { + if (errno != EEXIST) + return error_errno(_("cannot open %s"), namebuf); + if (unlink(namebuf)) + return error_errno(_("cannot unlink '%s'"), namebuf); + fd = open(namebuf, O_CREAT | O_EXCL | O_WRONLY, 0666); + if (fd < 0) + return error_errno(_("cannot open %s"), namebuf); + } + rej = fdopen(fd, "w"); if (!rej) return error_errno(_("cannot open %s"), namebuf); diff --git a/t/t4115-apply-symlink.sh b/t/t4115-apply-symlink.sh index 14e0f4d705..2d03c4e4d1 100755 --- a/t/t4115-apply-symlink.sh +++ b/t/t4115-apply-symlink.sh @@ -125,4 +125,19 @@ test_expect_success SYMLINKS 'symlink escape when deleting file' ' test_path_is_file .git/delete-me ' +test_expect_success SYMLINKS '--reject removes .rej symlink if it exists' ' + test_when_finished "git reset --hard && git clean -dfx" && + + test_commit file && + echo modified >file.t && + git diff -- file.t >patch && + echo modified-again >file.t && + + ln -s foo file.t.rej && + test_must_fail git apply patch --reject 2>err && + test_i18ngrep "Rejected hunk" err && + test_path_is_missing foo && + test_path_is_file file.t.rej +' + test_done From 29198213c9163c1d552ee2bdbf78d2b09ccc98b8 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Thu, 6 Apr 2023 11:42:03 -0400 Subject: [PATCH 23/28] t1300: demonstrate failure when renaming sections with long lines When renaming a configuration section which has an entry whose length exceeds the size of our buffer in config.c's implementation of `git_config_copy_or_rename_section_in_file()`, Git will incorrectly form a new configuration section with part of the data in the section being removed. In this instance, our first configuration file looks something like: [b] c = d [a] e = f [a] g = h Here, we have two configuration values, "b.c", and "a.g". The value "[a] e = f" belongs to the configuration value "b.c", and does not form its own section. However, when renaming the section 'a' to 'xyz', Git will write back "[xyz]\ne = f", but "[xyz]" is still attached to the value of "b.c", which is why "e = f" on its own line becomes a new entry called "b.e". A slightly different example embeds the section being renamed within another section. Demonstrate this failure in a test in t1300, which we will fix in the following commit. Co-authored-by: Johannes Schindelin Helped-by: Jeff King Signed-off-by: Johannes Schindelin Signed-off-by: Taylor Blau --- t/t1300-config.sh | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 1a4156c704..cd8f744160 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -613,6 +613,26 @@ test_expect_success 'renaming to bogus section is rejected' ' test_must_fail git config --rename-section branch.zwei "bogus name" ' +test_expect_failure 'renaming a section with a long line' ' + { + printf "[b]\\n" && + printf " c = d %1024s [a] e = f\\n" " " && + printf "[a] g = h\\n" + } >y && + git config -f y --rename-section a xyz && + test_must_fail git config -f y b.e +' + +test_expect_failure 'renaming an embedded section with a long line' ' + { + printf "[b]\\n" && + printf " c = d %1024s [a] [foo] e = f\\n" " " && + printf "[a] g = h\\n" + } >y && + git config -f y --rename-section a xyz && + test_must_fail git config -f y foo.e +' + cat >> .git/config << EOF [branch "zwei"] a = 1 [branch "vier"] EOF From c4137be0f5a6edf9a9044e6e43ecf4468c7a4046 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 22 Feb 2023 12:40:55 +0100 Subject: [PATCH 24/28] gettext: avoid using gettext if the locale dir is not present In cc5e1bf99247 (gettext: avoid initialization if the locale dir is not present, 2018-04-21) Git was taught to avoid a costly gettext start-up when there are not even any localized messages to work with. But we still called `gettext()` and `ngettext()` functions. Which caused a problem in Git for Windows when the libgettext that is consumed from the MSYS2 project stopped using a runtime prefix in https://github.com/msys2/MINGW-packages/pull/10461 Due to that change, we now use an unintialized gettext machinery that might get auto-initialized _using an unintended locale directory_: `C:\mingw64\share\locale`. Let's record the fact when the gettext initialization was skipped, and skip calling the gettext functions accordingly. This addresses CVE-2023-25815. Signed-off-by: Johannes Schindelin --- gettext.c | 4 ++++ gettext.h | 7 ++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/gettext.c b/gettext.c index 1b564216d0..610d402fe7 100644 --- a/gettext.c +++ b/gettext.c @@ -109,6 +109,8 @@ static void init_gettext_charset(const char *domain) setlocale(LC_CTYPE, "C"); } +int git_gettext_enabled = 0; + void git_setup_gettext(void) { const char *podir = getenv(GIT_TEXT_DOMAIN_DIR_ENVIRONMENT); @@ -130,6 +132,8 @@ void git_setup_gettext(void) init_gettext_charset("git"); textdomain("git"); + git_gettext_enabled = 1; + free(p); } diff --git a/gettext.h b/gettext.h index bee52eb113..b96ab9d340 100644 --- a/gettext.h +++ b/gettext.h @@ -31,9 +31,11 @@ int use_gettext_poison(void); #ifndef NO_GETTEXT +extern int git_gettext_enabled; void git_setup_gettext(void); int gettext_width(const char *s); #else +#define git_gettext_enabled (0) static inline void git_setup_gettext(void) { use_gettext_poison(); /* getenv() reentrancy paranoia */ @@ -48,7 +50,8 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid) { if (!*msgid) return ""; - return use_gettext_poison() ? "# GETTEXT POISON #" : gettext(msgid); + return use_gettext_poison() ? "# GETTEXT POISON #" : + !git_gettext_enabled ? msgid : gettext(msgid); } static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2) @@ -56,6 +59,8 @@ const char *Q_(const char *msgid, const char *plu, unsigned long n) { if (use_gettext_poison()) return "# GETTEXT POISON #"; + if (!git_gettext_enabled) + return n == 1 ? msgid : plu; return ngettext(msgid, plu, n); } From a5bb10fd5e74101e7c07da93e7c32bbe60f6173a Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Thu, 6 Apr 2023 14:07:58 -0400 Subject: [PATCH 25/28] config: avoid fixed-sized buffer when renaming/deleting a section MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When renaming (or deleting) a section of configuration, Git uses the function `git_config_copy_or_rename_section_in_file()` to rewrite the configuration file after applying the rename or deletion to the given section. To do this, Git repeatedly calls `fgets()` to read the existing configuration data into a fixed size buffer. When the configuration value under `old_name` exceeds the size of the buffer, we will call `fgets()` an additional time even if there is no newline in the configuration file, since our read length is capped at `sizeof(buf)`. If the first character of the buffer (after zero or more characters satisfying `isspace()`) is a '[', Git will incorrectly treat it as beginning a new section when the original section is being removed. In other words, a configuration value satisfying this criteria can incorrectly be considered as a new secftion instead of a variable in the original section. Avoid this issue by using a variable-width buffer in the form of a strbuf rather than a fixed-with region on the stack. A couple of small points worth noting: - Using a strbuf will cause us to allocate arbitrary sizes to match the length of each line. In practice, we don't expect any reasonable configuration files to have lines that long, and a bandaid will be introduced in a later patch to ensure that this is the case. - We are using strbuf_getwholeline() here instead of strbuf_getline() in order to match `fgets()`'s behavior of leaving the trailing LF character on the buffer (as well as a trailing NUL). This could be changed later, but using strbuf_getwholeline() changes the least about this function's implementation, so it is picked as the safest path. - It is temping to want to replace the loop to skip over characters matching isspace() at the beginning of the buffer with a convenience function like `strbuf_ltrim()`. But this is the wrong approach for a couple of reasons: First, it involves a potentially large and expensive `memmove()` which we would like to avoid. Second, and more importantly, we also *do* want to preserve those spaces to avoid changing the output of other sections. In all, this patch is a minimal replacement of the fixed-width buffer in `git_config_copy_or_rename_section_in_file()` to instead use a `struct strbuf`. Reported-by: André Baptista Reported-by: Vítor Pinho Helped-by: Patrick Steinhardt Co-authored-by: Johannes Schindelin Signed-off-by: Johannes Schindelin Signed-off-by: Taylor Blau --- config.c | 13 +++++++------ t/t1300-config.sh | 4 ++-- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/config.c b/config.c index 1137bd73af..524347676d 100644 --- a/config.c +++ b/config.c @@ -3091,7 +3091,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename char *filename_buf = NULL; struct lock_file lock = LOCK_INIT; int out_fd; - char buf[1024]; + struct strbuf buf = STRBUF_INIT; FILE *config_file = NULL; struct stat st; struct strbuf copystr = STRBUF_INIT; @@ -3132,14 +3132,14 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename goto out; } - while (fgets(buf, sizeof(buf), config_file)) { + while (!strbuf_getwholeline(&buf, config_file, '\n')) { unsigned i; int length; int is_section = 0; - char *output = buf; - for (i = 0; buf[i] && isspace(buf[i]); i++) + char *output = buf.buf; + for (i = 0; buf.buf[i] && isspace(buf.buf[i]); i++) ; /* do nothing */ - if (buf[i] == '[') { + if (buf.buf[i] == '[') { /* it's a section */ int offset; is_section = 1; @@ -3158,7 +3158,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename strbuf_reset(©str); } - offset = section_name_match(&buf[i], old_name); + offset = section_name_match(&buf.buf[i], old_name); if (offset > 0) { ret++; if (new_name == NULL) { @@ -3233,6 +3233,7 @@ out: out_no_rollback: free(filename_buf); config_store_data_clear(&store); + strbuf_release(&buf); return ret; } diff --git a/t/t1300-config.sh b/t/t1300-config.sh index cd8f744160..24c13b91db 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -613,7 +613,7 @@ test_expect_success 'renaming to bogus section is rejected' ' test_must_fail git config --rename-section branch.zwei "bogus name" ' -test_expect_failure 'renaming a section with a long line' ' +test_expect_success 'renaming a section with a long line' ' { printf "[b]\\n" && printf " c = d %1024s [a] e = f\\n" " " && @@ -623,7 +623,7 @@ test_expect_failure 'renaming a section with a long line' ' test_must_fail git config -f y b.e ' -test_expect_failure 'renaming an embedded section with a long line' ' +test_expect_success 'renaming an embedded section with a long line' ' { printf "[b]\\n" && printf " c = d %1024s [a] [foo] e = f\\n" " " && From e91cfe6085c4a61372d1f800b473b73b8d225d0d Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Thu, 6 Apr 2023 14:28:53 -0400 Subject: [PATCH 26/28] config.c: avoid integer truncation in `copy_or_rename_section_in_file()` There are a couple of spots within `copy_or_rename_section_in_file()` that incorrectly use an `int` to track an offset within a string, which may truncate or wrap around to a negative value. Historically it was impossible to have a line longer than 1024 bytes anyway, since we used fgets() with a fixed-size buffer of exactly that length. But the recent change to use a strbuf permits us to read lines of arbitrary length, so it's possible for a malicious input to cause us to overflow past INT_MAX and do an out-of-bounds array read. Practically speaking, however, this should never happen, since it requires 2GB section names or values, which are unrealistic in non-malicious circumstances. Co-authored-by: Jeff King Signed-off-by: Jeff King Signed-off-by: Taylor Blau --- config.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/config.c b/config.c index 524347676d..e4189aa2d7 100644 --- a/config.c +++ b/config.c @@ -3027,9 +3027,10 @@ void git_config_set_multivar(const char *key, const char *value, flags); } -static int section_name_match (const char *buf, const char *name) +static size_t section_name_match (const char *buf, const char *name) { - int i = 0, j = 0, dot = 0; + size_t i = 0, j = 0; + int dot = 0; if (buf[i] != '[') return 0; for (i = 1; buf[i] && buf[i] != ']'; i++) { @@ -3133,15 +3134,14 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename } while (!strbuf_getwholeline(&buf, config_file, '\n')) { - unsigned i; - int length; + size_t i, length; int is_section = 0; char *output = buf.buf; for (i = 0; buf.buf[i] && isspace(buf.buf[i]); i++) ; /* do nothing */ if (buf.buf[i] == '[') { /* it's a section */ - int offset; + size_t offset; is_section = 1; /* From 3bb3d6bac5f2b496dfa2862dc1a84cbfa9b4449a Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Wed, 12 Apr 2023 19:18:28 -0400 Subject: [PATCH 27/28] config.c: disallow overly-long lines in `copy_or_rename_section_in_file()` As a defense-in-depth measure to guard against any potentially-unknown buffer overflows in `copy_or_rename_section_in_file()`, refuse to work with overly-long lines in a gitconfig. Signed-off-by: Taylor Blau Signed-off-by: Johannes Schindelin --- config.c | 13 +++++++++++++ t/t1300-config.sh | 10 ++++++++++ 2 files changed, 23 insertions(+) diff --git a/config.c b/config.c index e4189aa2d7..b8194dfd8a 100644 --- a/config.c +++ b/config.c @@ -3083,6 +3083,8 @@ static int section_name_is_ok(const char *name) return 1; } +#define GIT_CONFIG_MAX_LINE_LEN (512 * 1024) + /* if new_name == NULL, the section is removed instead */ static int git_config_copy_or_rename_section_in_file(const char *config_filename, const char *old_name, @@ -3097,6 +3099,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename struct stat st; struct strbuf copystr = STRBUF_INIT; struct config_store_data store; + uint32_t line_nr = 0; memset(&store, 0, sizeof(store)); @@ -3137,6 +3140,16 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename size_t i, length; int is_section = 0; char *output = buf.buf; + + line_nr++; + + if (buf.len >= GIT_CONFIG_MAX_LINE_LEN) { + ret = error(_("refusing to work with overly long line " + "in '%s' on line %"PRIuMAX), + config_filename, (uintmax_t)line_nr); + goto out; + } + for (i = 0; buf.buf[i] && isspace(buf.buf[i]); i++) ; /* do nothing */ if (buf.buf[i] == '[') { diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 24c13b91db..de564cb8e5 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -633,6 +633,16 @@ test_expect_success 'renaming an embedded section with a long line' ' test_must_fail git config -f y foo.e ' +test_expect_success 'renaming a section with an overly-long line' ' + { + printf "[b]\\n" && + printf " c = d %525000s e" " " && + printf "[a] g = h\\n" + } >y && + test_must_fail git config -f y --rename-section a xyz 2>err && + test_i18ngrep "refusing to work with overly long line in .y. on line 2" err +' + cat >> .git/config << EOF [branch "zwei"] a = 1 [branch "vier"] EOF From 668f2d53613ac8fd373926ebe219f2c29112d93e Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 14 Apr 2023 11:54:08 -0400 Subject: [PATCH 28/28] Git 2.30.9 Signed-off-by: Taylor Blau Signed-off-by: Johannes Schindelin --- Documentation/RelNotes/2.30.9.txt | 43 +++++++++++++++++++++++++++++++ GIT-VERSION-GEN | 2 +- RelNotes | 2 +- 3 files changed, 45 insertions(+), 2 deletions(-) create mode 100644 Documentation/RelNotes/2.30.9.txt diff --git a/Documentation/RelNotes/2.30.9.txt b/Documentation/RelNotes/2.30.9.txt new file mode 100644 index 0000000000..708d626ce6 --- /dev/null +++ b/Documentation/RelNotes/2.30.9.txt @@ -0,0 +1,43 @@ +Git v2.30.9 Release Notes +========================= + +This release addresses the security issues CVE-2023-25652, +CVE-2023-25815, and CVE-2023-29007. + + +Fixes since v2.30.8 +------------------- + + * CVE-2023-25652: + + By feeding specially crafted input to `git apply --reject`, a + path outside the working tree can be overwritten with partially + controlled contents (corresponding to the rejected hunk(s) from + the given patch). + + * CVE-2023-25815: + + When Git is compiled with runtime prefix support and runs without + translated messages, it still used the gettext machinery to + display messages, which subsequently potentially looked for + translated messages in unexpected places. This allowed for + malicious placement of crafted messages. + + * CVE-2023-29007: + + When renaming or deleting a section from a configuration file, + certain malicious configuration values may be misinterpreted as + the beginning of a new configuration section, leading to arbitrary + configuration injection. + +Credit for finding CVE-2023-25652 goes to Ry0taK, and the fix was +developed by Taylor Blau, Junio C Hamano and Johannes Schindelin, +with the help of Linus Torvalds. + +Credit for finding CVE-2023-25815 goes to Maxime Escourbiac and +Yassine BENGANA of Michelin, and the fix was developed by Johannes +Schindelin. + +Credit for finding CVE-2023-29007 goes to André Baptista and Vítor Pinho +of Ethiack, and the fix was developed by Taylor Blau, and Johannes +Schindelin, with help from Jeff King, and Patrick Steinhardt. diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index 2a52946afc..6681d3083c 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.30.8 +DEF_VER=v2.30.9 LF=' ' diff --git a/RelNotes b/RelNotes index 9f25ba7139..2f23806490 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.30.8.txt \ No newline at end of file +Documentation/RelNotes/2.30.9.txt \ No newline at end of file