* Re: [PATCH 8/9] for-each-ref: add option to fully dereference tags
From: Junio C Hamano @ 2023-11-09 1:32 UTC (permalink / raw)
To: Victoria Dye; +Cc: Patrick Steinhardt, Victoria Dye via GitGitGadget, git
In-Reply-To: <xmqqfs1f3eji.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> You have to feed it to "git cat-file tag" and parse the contents of
> the tag obbject yourself to manually peel further levels of onion.
Alternatively, you can drive "git show -s" with "--format" and you
probably can produce a machine parseable output.
But it does not change the argument fundamentally. The point is
that "for-each-ref --format=%(*field)" that peels only the first
layer would not have helped all that much, if somebody really cares
about each levels of nested tags. They would have been relying on
a solution to deal with the second and further layers anyway, and
that solution would have been working with the first layer, too.
^ permalink raw reply
* Re: [PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks
From: Patrick Steinhardt @ 2023-11-09 6:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqqwmus3zvd.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1909 bytes --]
On Thu, Nov 09, 2023 at 02:43:02AM +0900, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > ... But the more robust way to do it is:
> >
> > # export ORIG_PATH, which presumably is not cleared the same way PATH
> > # is, so that the hook can access it
> > ORIG_PATH=$PATH &&
> > export ORIG_PATH &&
> >
> > cat >"$hook" <<EOF
> > # pull the original path from the caller
> > PATH=$ORIG_PATH
> > export PATH
> >
> > ...do other stuff...
> > EOF
> >
> > That's assuming that environment variables make it intact to the hook at
> > all (it is not clear to me why the original $PATH doesn't).
>
> Yeah, the parenthetical comment points at the crux of the issue. I
> can tell from the patch what issue the platform throws at us we are
> trying to work around, but it is frustrating not to know why the
> platform does such unpleasant things in the first place.
>
> Thanks.
Indeed, I should've described this better in the commit message. Quoting
[1]:
> By default, Subversion executes hook scripts with an empty
> environment—that is, no environment variables are set at all, not even
> $PATH (or %PATH%, under Windows). Because of this, many administrators
> are baffled when their hook program runs fine by hand, but doesn't
> work when invoked by Subversion. Administrators have historically
> worked around this problem by manually setting all the environment
> variables their hook scripts need in the scripts themselves.
So it's not an issue of the environment, but rather an implementation
detail of how Subversion hooks work. It's surprising that this does not
fail on other platforms -- maybe the shell has a default PATH there that
allow us to locate basename(1)? I dunno.
Will add to the commit message.
Patrick
[1]: https://svnbook.red-bean.com/en/1.8/svn.reposadmin.create.html#svn.reposadmin.create.hooks
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Patrick Steinhardt @ 2023-11-09 6:30 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20231108165426.GB1028115@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2646 bytes --]
On Wed, Nov 08, 2023 at 11:54:26AM -0500, Jeff King wrote:
> On Wed, Nov 08, 2023 at 03:57:19PM +0100, Patrick Steinhardt wrote:
>
> > While it is possible to specify these paths via `LIB_HTTPD_PATH` and
> > `LIB_HTTPD_MODULE_PATH`, it is not a nice experience for the developer
> > to figure out how to set those up. And in fact we can do better by
> > dynamically detecting both httpd and its module path at runtime:
> >
> > - The httpd binary can be located via PATH.
> >
> > - The module directory can (in many cases) be derived via the
> > `HTTPD_ROOT` compile-time variable.
> >
> > Amend the code to do so. The new runtime-detected paths will only be
> > used in case none of the hardcoded paths are usable.
>
> If these improve detection on your platform, I think that is a good
> thing and they are worth doing. But as a generic mechanism, I have two
> comments:
>
> > -for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
> > +for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2' "$(command -v httpd)"
> > do
> > if test -x "$DEFAULT_HTTPD_PATH"
> > then
>
> The binary goes under the name "httpd", but also "apache2". But the PATH
> search only looks for "httpd". Should we check "command -v apache2" as
> well?
Doesn't hurt to do it.
> This also means we may run "test -x" on an empty string, but that is
> probably OK in practice (we could guard it with "test -n", though).
Fair, will do.
> > +if test -x "$DEFAULT_HTTPD_PATH"
> > +then
> > + DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
> > +fi
>
> I was really pleased to see this and hoped it could replace the
> kitchen-sink list of paths in the hunk below. But sadly I think it
> depends on having a configured apache setup. On my Debian system, for
> example, I have the "apache2-bin" package installed but not "apache"
> (because only the latter actually sets up a system apache daemon, which
> I don't want). And thus there is no config:
>
> $ /usr/sbin/apache2 -D HTTPD_ROOT
> apache2: Could not open configuration file /etc/apache2/apache2.conf: No such file or directory
>
> So without a system config file to act as a template for our custom
> config, I think we are stuck with guessing where the installer might
> have put them.
Indeed. Also, paths like `/usr/libexec/apache2` wouldn't be possible to
be discovered via `${HTTPD_ROOT}/modules` anyway. It's kind of a shame
that there is no way (or at least no way I know of) to portably discover
the default modules directory used by httpd.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks
From: Patrick Steinhardt @ 2023-11-09 7:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <ZUx8adNt3Ky-U09W@tanuki>
[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]
On Thu, Nov 09, 2023 at 07:30:01AM +0100, Patrick Steinhardt wrote:
> On Thu, Nov 09, 2023 at 02:43:02AM +0900, Junio C Hamano wrote:
> > Jeff King <peff@peff.net> writes:
[snip]
> > By default, Subversion executes hook scripts with an empty
> > environment—that is, no environment variables are set at all, not even
> > $PATH (or %PATH%, under Windows). Because of this, many administrators
> > are baffled when their hook program runs fine by hand, but doesn't
> > work when invoked by Subversion. Administrators have historically
> > worked around this problem by manually setting all the environment
> > variables their hook scripts need in the scripts themselves.
>
> So it's not an issue of the environment, but rather an implementation
> detail of how Subversion hooks work. It's surprising that this does not
> fail on other platforms -- maybe the shell has a default PATH there that
> allow us to locate basename(1)? I dunno.
Yup, that's in fact it. Bash falls back to a default PATH that can be
queried via getconf(1p), and this works alright on most platforms. Not
on NixOS though.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 0/9] some more chunk-file bounds-checks fixes
From: Jeff King @ 2023-11-09 7:03 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
This is a follow-up to the series from:
https://lore.kernel.org/git/20231009205544.GA3281950@coredump.intra.peff.net/
which was merged to master as jk/chunk-bounds. There were a few issues
left open by that series and its review:
1. the midx code didn't check fanout ordering
2. whether we needed to sprinkle some more st_mult() on it
3. improving some of the error messages (translations, some
consistency, maybe more details)
4. possible refactoring with a pair_chunk_expect() API (Taylor posted
a series in that direction, which is currently in limbo)
The patches here fix the remaining correctness issues (points 1 and 2,
along with a few other small issues I found). There's some improvement
on 3, but I stopped short of adding lots more details. Partially because
the series was already getting big, and partially because going further
may depend on what we do with 4.
With regards to the pair_chunk_expect() thing, this series is
incompatible (textually, but not conceptually) with what Taylor posted
earlier, just because I'm moving some checks into the chunk-reader
callbacks. Because it's fixing user-visible bugs, I think we'd want to
do this first, and then (possibly) rebase Taylor's series on top. But I
also think some of the things I noticed around overflow (especially
patches 1 and 6) may inform how we'd want the pair_chunk_expect() API to
look.
This is a continuation of the jk/chunk-bounds topic, which is new in the
v2.43 cycle. But it should be OK to leave this until after the release.
Nothing here is fixing a regression in the 2.43 release candidates; it's
just a few bits that were incomplete. That said, I did try to float the
correctness bits to the first two patches just in case. ;)
[1/9]: commit-graph: handle overflow in chunk_size checks
[2/9]: midx: check consistency of fanout table
[3/9]: commit-graph: drop redundant call to "lite" verification
[4/9]: commit-graph: clarify missing-chunk error messages
[5/9]: commit-graph: abort as soon as we see a bogus chunk
[6/9]: commit-graph: use fanout value for graph size
[7/9]: commit-graph: check order while reading fanout chunk
[8/9]: commit-graph: drop verify_commit_graph_lite()
[9/9]: commit-graph: mark chunk error messages for translation
commit-graph.c | 94 +++++++++++++------------------------
midx.c | 20 ++++----
t/t5318-commit-graph.sh | 16 ++++---
t/t5319-multi-pack-index.sh | 14 ++++++
4 files changed, 67 insertions(+), 77 deletions(-)
-Peff
^ permalink raw reply
* [PATCH 1/9] commit-graph: handle overflow in chunk_size checks
From: Jeff King @ 2023-11-09 7:09 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
We check the size of chunks with fixed records by multiplying the width
of each record by the number of commits in the file. Like:
if (chunk_size != g->num_commits * GRAPH_DATA_WIDTH)
If this multiplication overflows, we may not notice a chunk is too small
(which could later lead to out-of-bound reads).
In the current code this is only possible for the CDAT chunk, but the
reasons are quite subtle. We compute g->num_commits by dividing the size
of the OIDL chunk by the hash length (since it consists of a bunch of
hashes). So we know that any size_t multiplication that uses a value
smaller than the hash length cannot overflow. And the CDAT records are
the only ones that are larger (the others are just 4-byte records). So
it's worth fixing all of these, to make it clear that they're not
subject to overflow (without having to reason about seemingly unrelated
code).
The obvious thing to do is add an st_mult(), like:
if (chunk_size != st_mult(g->num_commits, GRAPH_DATA_WIDTH))
And that certainly works, but it has one downside: if we detect an
overflow, we'll immediately die(). But the commit graph is an optional
file; if we run into other problems loading it, we'll generally return
an error and fall back to accessing the full objects. Using st_mult()
means a malformed file will abort the whole process.
So instead, we can do a division like this:
if (chunk_size / GRAPH_DATA_WIDTH != g->num_commits)
where there's no possibility of overflow. We do lose a little bit of
precision; due to integer division truncation we'd allow up to an extra
GRAPH_DATA_WIDTH-1 bytes of data in the chunk. That's OK. Our main goal
here is making sure we don't have too _few_ bytes, which would cause an
out-of-bounds read (we could actually replace our "!=" with "<", but I
think it's worth being a little pedantic, as a large mismatch could be a
sign of other problems).
I didn't add a test here. We'd need to generate a very large graph file
in order to get g->num_commits large enough to cause an overflow. And a
later patch in this series will use this same division technique in a
way that is much easier to trigger in the tests.
Signed-off-by: Jeff King <peff@peff.net>
---
There are still several st_mult() calls within commit-graph.c, unrelated
to my jk/chunk-bounds series. I suspect they're all redundant, as the
chunk-size checks give a stricter bound. But checking and removing them
is a separate topic.
I think the midx code has similar st_mult() problems, but it's quite
happy to die() on any error already. So I've left auditing that for
another day.
Another alternative to the division is introducing an st_mult() variant
like:
if (!st_mult(&out, &a, &b))
return error(...);
We may want to go there in the long run as part of lib-ification, but I
didn't want to get into it for this series.
commit-graph.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index ee66098e07..5d7d7a89e5 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -344,7 +344,7 @@ static int graph_read_commit_data(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
struct commit_graph *g = data;
- if (chunk_size != g->num_commits * GRAPH_DATA_WIDTH)
+ if (chunk_size / GRAPH_DATA_WIDTH != g->num_commits)
return error("commit-graph commit data chunk is wrong size");
g->chunk_commit_data = chunk_start;
return 0;
@@ -354,7 +354,7 @@ static int graph_read_generation_data(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
struct commit_graph *g = data;
- if (chunk_size != g->num_commits * sizeof(uint32_t))
+ if (chunk_size / sizeof(uint32_t) != g->num_commits)
return error("commit-graph generations chunk is wrong size");
g->chunk_generation_data = chunk_start;
return 0;
@@ -364,7 +364,7 @@ static int graph_read_bloom_index(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
struct commit_graph *g = data;
- if (chunk_size != g->num_commits * 4) {
+ if (chunk_size / 4 != g->num_commits) {
warning("commit-graph changed-path index chunk is too small");
return -1;
}
--
2.43.0.rc1.572.g273fc7bed6
^ permalink raw reply related
* [PATCH v3 0/3] t: improve compatibility with NixOS
From: Patrick Steinhardt @ 2023-11-09 7:09 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699428122.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5864 bytes --]
Hi,
this is the third version of my patch series to improve compatibility of
our tests with NixOS.
Changes compared to v2:
- Patch 1: We now check for both httpd and apache2 binaries via
PATH.
- Patch 1: We're a bit more defensive and will check whether
variables are empty before passing them to either `test -d` or
`test -x`.
- Patch 3: Clarified why PATH is unset.
- Patch 3: Instead of writing PATH into the hook we now write it
into the `hook-env` configuration file read by Subversion.
Patrick
Patrick Steinhardt (3):
t/lib-httpd: dynamically detect httpd and modules path
t/lib-httpd: stop using legacy crypt(3) for authentication
t9164: fix inability to find basename(1) in Subversion hooks
t/lib-httpd.sh | 17 +++++++++++++----
t/lib-httpd/passwd | 2 +-
t/lib-httpd/proxy-passwd | 2 +-
t/t9164-git-svn-dcommit-concurrent.sh | 9 ++++++++-
4 files changed, 23 insertions(+), 7 deletions(-)
Range-diff against v2:
1: cee8fbebf84 ! 1: e4c75c492dd t/lib-httpd: dynamically detect httpd and modules path
@@ t/lib-httpd.sh: fi
HTTPD_PARA=""
-for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
-+for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2' "$(command -v httpd)"
++for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' \
++ '/usr/sbin/apache2' \
++ "$(command -v httpd)" \
++ "$(command -v apache2)"
do
- if test -x "$DEFAULT_HTTPD_PATH"
+- if test -x "$DEFAULT_HTTPD_PATH"
++ if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH"
then
-@@ t/lib-httpd.sh: do
+ break
fi
done
@@ t/lib-httpd.sh: do
+ '/usr/libexec/httpd' \
+ "${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}"
do
- if test -d "$DEFAULT_HTTPD_MODULE_PATH"
+- if test -d "$DEFAULT_HTTPD_MODULE_PATH"
++ if test -n "$DEFAULT_HTTPD_MODULE_PATH" -a -d "$DEFAULT_HTTPD_MODULE_PATH"
then
+ break
+ fi
2: f4c6c754f1e = 2: 3dce76da477 t/lib-httpd: stop using legacy crypt(3) for authentication
3: 361f1bd9c88 ! 3: 6891e254155 t9164: fix inability to find basename(1) in hooks
@@ Metadata
Author: Patrick Steinhardt <ps@pks.im>
## Commit message ##
- t9164: fix inability to find basename(1) in hooks
+ t9164: fix inability to find basename(1) in Subversion hooks
- The post-commit hook in t9164 is executed with a default environment.
- To work around this issue we already write the current `PATH` value into
- the hook script. But this is done at a point where we already tried to
- execute non-built-in commands, namely basename(1). While this seems to
- work alright on most platforms, it fails on NixOS.
+ Hooks executed by Subversion are spawned with an empty environment. By
+ default, not even variables like PATH will be propagated to them. In
+ order to ensure that we're still able to find required executables, we
+ thus write the current PATH variable into the hook script itself and
+ then re-export it in t9164.
- Set the `PATH` variable earlier to fix this issue. Note that we do this
- via two separate heredocs. This is done because in the first one we do
- want to expand variables, whereas in the second one we don't.
+ This happens too late in the script though, as we already tried to
+ execute the basename(1) utility before exporting the PATH variable. This
+ tends to work on most platforms as the fallback value of PATH for Bash
+ (see `getconf PATH`) is likely to contain this binary. But on more
+ exotic platforms like NixOS this is not the case, and thus the test
+ fails.
+
+ While we could work around this issue by simply setting PATH earlier, it
+ feels fragile to inject a user-controlled value into the script and have
+ the shell interpret it. Instead, we can refactor the hook setup to write
+ a `hooks-env` file that configures PATH for us. Like this, Subversion
+ will know to set up the environment as expected for all hooks.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## t/t9164-git-svn-dcommit-concurrent.sh ##
@@ t/t9164-git-svn-dcommit-concurrent.sh: setup_hook()
+ "passed to setup_hook" >&2 ; return 1; }
echo "cnt=$skip_revs" > "$hook_type-counter"
rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
- hook="$rawsvnrepo/hooks/$hook_type"
-- cat > "$hook" <<- 'EOF1'
-+ cat >"$hook" <<-EOF
- #!/bin/sh
- set -e
-+
-+ PATH=\"$PATH\"
-+ export PATH
-+ EOF
+
-+ cat >>"$hook" <<-'EOF'
- cd "$1/.." # "$1" is repository location
- exec >> svn-hook.log 2>&1
- hook="$(basename "$0")"
-@@ t/t9164-git-svn-dcommit-concurrent.sh: setup_hook()
- cnt="$(($cnt - 1))"
- echo "cnt=$cnt" > ./$hook-counter
- [ "$cnt" = "0" ] || exit 0
--EOF1
++ # Subversion hooks run with an empty environment by default. We thus
++ # need to propagate PATH so that we can find executables.
++ cat >"$rawsvnrepo/conf/hooks-env" <<-EOF
++ [default]
++ PATH = ${PATH}
+ EOF
+
+ hook="$rawsvnrepo/hooks/$hook_type"
+ cat > "$hook" <<- 'EOF1'
+ #!/bin/sh
+@@ t/t9164-git-svn-dcommit-concurrent.sh: EOF1
if [ "$hook_type" = "pre-commit" ]; then
echo "echo 'commit disallowed' >&2; exit 1" >>"$hook"
else
base-commit: dadef801b365989099a9929e995589e455c51fed
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Patrick Steinhardt @ 2023-11-09 7:09 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699513524.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2159 bytes --]
In order to set up the Apache httpd server, we need to locate both the
httpd binary and its default module path. This is done with a hardcoded
list of locations that we scan. While this works okayish with distros
that more-or-less follow the Filesystem Hierarchy Standard, it falls
apart on others like NixOS that don't.
While it is possible to specify these paths via `LIB_HTTPD_PATH` and
`LIB_HTTPD_MODULE_PATH`, it is not a nice experience for the developer
to figure out how to set those up. And in fact we can do better by
dynamically detecting both httpd and its module path at runtime:
- The httpd binary can be located via PATH.
- The module directory can (in many cases) be derived via the
`HTTPD_ROOT` compile-time variable.
Amend the code to do so. The new runtime-detected paths will only be
used in case none of the hardcoded paths are usable.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/lib-httpd.sh | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 5fe3c8ab69d..6ab8f273a3a 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -55,21 +55,30 @@ fi
HTTPD_PARA=""
-for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
+for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' \
+ '/usr/sbin/apache2' \
+ "$(command -v httpd)" \
+ "$(command -v apache2)"
do
- if test -x "$DEFAULT_HTTPD_PATH"
+ if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH"
then
break
fi
done
+if test -x "$DEFAULT_HTTPD_PATH"
+then
+ DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
+fi
+
for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
'/usr/lib/apache2/modules' \
'/usr/lib64/httpd/modules' \
'/usr/lib/httpd/modules' \
- '/usr/libexec/httpd'
+ '/usr/libexec/httpd' \
+ "${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}"
do
- if test -d "$DEFAULT_HTTPD_MODULE_PATH"
+ if test -n "$DEFAULT_HTTPD_MODULE_PATH" -a -d "$DEFAULT_HTTPD_MODULE_PATH"
then
break
fi
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication
From: Patrick Steinhardt @ 2023-11-09 7:09 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699513524.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]
When setting up httpd for our tests, we also install a passwd and
proxy-passwd file that contain the test user's credentials. These
credentials currently use crypt(3) as the password encryption schema.
This schema can be considered deprecated nowadays as it is not safe
anymore. Quoting Apache httpd's documentation [1]:
> Unix only. Uses the traditional Unix crypt(3) function with a
> randomly-generated 32-bit salt (only 12 bits used) and the first 8
> characters of the password. Insecure.
This is starting to cause issues in modern Linux distributions. glibc
has deprecated its libcrypt library that used to provide crypt(3) in
favor of the libxcrypt library. This newer replacement provides a
compile time switch to disable insecure password encryption schemata,
which causes crypt(3) to always return `EINVAL`. The end result is that
httpd tests that exercise authentication will fail on distros that use
libxcrypt without these insecure encryption schematas.
Regenerate the passwd files to instead use the default password
encryption schema, which is md5. While it feels kind of funny that an
MD5-based encryption schema should be more secure than anything else, it
is the current default and supported by all platforms. Furthermore, it
really doesn't matter all that much given that these files are only used
for testing purposes anyway.
[1]: https://httpd.apache.org/docs/2.4/misc/password_encryptions.html
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/lib-httpd/passwd | 2 +-
t/lib-httpd/proxy-passwd | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
index 99a34d64874..d9c122f3482 100644
--- a/t/lib-httpd/passwd
+++ b/t/lib-httpd/passwd
@@ -1 +1 @@
-user@host:xb4E8pqD81KQs
+user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
diff --git a/t/lib-httpd/proxy-passwd b/t/lib-httpd/proxy-passwd
index 77c25138e07..2ad7705d9a3 100644
--- a/t/lib-httpd/proxy-passwd
+++ b/t/lib-httpd/proxy-passwd
@@ -1 +1 @@
-proxuser:2x7tAukjAED5M
+proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 3/3] t9164: fix inability to find basename(1) in Subversion hooks
From: Patrick Steinhardt @ 2023-11-09 7:10 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699513524.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2201 bytes --]
Hooks executed by Subversion are spawned with an empty environment. By
default, not even variables like PATH will be propagated to them. In
order to ensure that we're still able to find required executables, we
thus write the current PATH variable into the hook script itself and
then re-export it in t9164.
This happens too late in the script though, as we already tried to
execute the basename(1) utility before exporting the PATH variable. This
tends to work on most platforms as the fallback value of PATH for Bash
(see `getconf PATH`) is likely to contain this binary. But on more
exotic platforms like NixOS this is not the case, and thus the test
fails.
While we could work around this issue by simply setting PATH earlier, it
feels fragile to inject a user-controlled value into the script and have
the shell interpret it. Instead, we can refactor the hook setup to write
a `hooks-env` file that configures PATH for us. Like this, Subversion
will know to set up the environment as expected for all hooks.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t9164-git-svn-dcommit-concurrent.sh | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index c8e6c0733f4..d1dec89c3b7 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -46,6 +46,14 @@ setup_hook()
"passed to setup_hook" >&2 ; return 1; }
echo "cnt=$skip_revs" > "$hook_type-counter"
rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
+
+ # Subversion hooks run with an empty environment by default. We thus
+ # need to propagate PATH so that we can find executables.
+ cat >"$rawsvnrepo/conf/hooks-env" <<-EOF
+ [default]
+ PATH = ${PATH}
+ EOF
+
hook="$rawsvnrepo/hooks/$hook_type"
cat > "$hook" <<- 'EOF1'
#!/bin/sh
@@ -63,7 +71,6 @@ EOF1
if [ "$hook_type" = "pre-commit" ]; then
echo "echo 'commit disallowed' >&2; exit 1" >>"$hook"
else
- echo "PATH=\"$PATH\"; export PATH" >>"$hook"
echo "svnconf=\"$svnconf\"" >>"$hook"
cat >>"$hook" <<- 'EOF2'
cd work-auto-commits.svn
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/9] midx: check consistency of fanout table
From: Jeff King @ 2023-11-09 7:12 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
The commit-graph, midx, and pack idx on-disk formats all have oid fanout
tables which are fed to bsearch_hash(). If these tables do not increase
monotonically, then the binary search may not only produce bogus values,
it may cause out of bounds reads.
We fixed this for commit graphs in 4169d89645 (commit-graph: check
consistency of fanout table, 2023-10-09). That commit argued that we did
not need to do the same for midx and pack idx files, because they
already did this check. However, that is wrong. We _do_ check the fanout
table for pack idx files when we load them, but we only do so for midx
files when running "git multi-pack-index verify". So it is possible to
get an out-of-bounds read by running a normal command with a specially
crafted midx file.
Let's fix this using the same solution (and roughly the same test) we
did for the commit-graph in 4169d89645. This replaces the same check
from "multi-pack-index verify", because verify uses the same read
routines, we'd bail on reading the midx much sooner now. So let's make
sure to copy its verbose error message.
Signed-off-by: Jeff King <peff@peff.net>
---
midx.c | 20 +++++++++++---------
t/t5319-multi-pack-index.sh | 14 ++++++++++++++
2 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/midx.c b/midx.c
index 2f3863c936..1d14661dad 100644
--- a/midx.c
+++ b/midx.c
@@ -64,13 +64,24 @@ void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m)
static int midx_read_oid_fanout(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
+ int i;
struct multi_pack_index *m = data;
m->chunk_oid_fanout = (uint32_t *)chunk_start;
if (chunk_size != 4 * 256) {
error(_("multi-pack-index OID fanout is of the wrong size"));
return 1;
}
+ for (i = 0; i < 255; i++) {
+ uint32_t oid_fanout1 = ntohl(m->chunk_oid_fanout[i]);
+ uint32_t oid_fanout2 = ntohl(m->chunk_oid_fanout[i+1]);
+
+ if (oid_fanout1 > oid_fanout2) {
+ error(_("oid fanout out of order: fanout[%d] = %"PRIx32" > %"PRIx32" = fanout[%d]"),
+ i, oid_fanout1, oid_fanout2, i + 1);
+ return 1;
+ }
+ }
m->num_objects = ntohl(m->chunk_oid_fanout[255]);
return 0;
}
@@ -1782,15 +1793,6 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
}
stop_progress(&progress);
- for (i = 0; i < 255; i++) {
- uint32_t oid_fanout1 = ntohl(m->chunk_oid_fanout[i]);
- uint32_t oid_fanout2 = ntohl(m->chunk_oid_fanout[i + 1]);
-
- if (oid_fanout1 > oid_fanout2)
- midx_report(_("oid fanout out of order: fanout[%d] = %"PRIx32" > %"PRIx32" = fanout[%d]"),
- i, oid_fanout1, oid_fanout2, i + 1);
- }
-
if (m->num_objects == 0) {
midx_report(_("the midx contains no oid"));
/*
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index c4c6060cee..c20aafe99a 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -1157,4 +1157,18 @@ test_expect_success 'reader notices too-small revindex chunk' '
test_cmp expect.err err
'
+test_expect_success 'reader notices out-of-bounds fanout' '
+ # This is similar to the out-of-bounds fanout test in t5318. The values
+ # in adjacent entries should be large but not identical (they
+ # are used as hi/lo starts for a binary search, which would then abort
+ # immediately).
+ corrupt_chunk OIDF 0 $(printf "%02x000000" $(test_seq 0 254)) &&
+ test_must_fail git log 2>err &&
+ cat >expect <<-\EOF &&
+ error: oid fanout out of order: fanout[254] = fe000000 > 5c = fanout[255]
+ fatal: multi-pack-index required OID fanout chunk missing or corrupted
+ EOF
+ test_cmp expect err
+'
+
test_done
--
2.43.0.rc1.572.g273fc7bed6
^ permalink raw reply related
* [PATCH 3/9] commit-graph: drop redundant call to "lite" verification
From: Jeff King @ 2023-11-09 7:13 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
The idea of verify_commit_graph_lite() is to have cheap verification
checks both for everyday use of the graph files (to avoid out of bounds
reads, etc) as well as for doing a full check via "commit-graph verify"
(which will also check the hash, etc).
But the expensive verification checks operate on a commit_graph struct,
which we get by using the normal everyday-reader code! So any problem
we'd find by calling it would have been found before we even got to the
verify_one_commit_graph() function.
Removing it simplifies the code a bit, but also frees us up to move the
"lite" verification steps around within that everyday-reader code.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 5d7d7a89e5..d9fc08de86 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2690,10 +2690,6 @@ static int verify_one_commit_graph(struct repository *r,
struct commit *seen_gen_zero = NULL;
struct commit *seen_gen_non_zero = NULL;
- verify_commit_graph_error = verify_commit_graph_lite(g);
- if (verify_commit_graph_error)
- return verify_commit_graph_error;
-
if (!commit_graph_checksum_valid(g)) {
graph_report(_("the commit-graph file has incorrect checksum and is likely corrupt"));
verify_commit_graph_error = VERIFY_COMMIT_GRAPH_ERROR_HASH;
--
2.43.0.rc1.572.g273fc7bed6
^ permalink raw reply related
* [PATCH 4/9] commit-graph: clarify missing-chunk error messages
From: Jeff King @ 2023-11-09 7:14 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
When a required commit-graph chunk cannot be loaded, we leave its entry
in the struct NULL, and then later complain that it is missing. But
that's just one reason we might not have loaded it, as we also do some
data quality checks.
Let's switch these messages to say "missing or corrupted", which is
exactly what the midx code says for the same cases. Likewise, we'll use
the same phrasing and capitalization as those for consistency. And while
we're here, we can mark them for translation (just like the midx ones).
Signed-off-by: Jeff King <peff@peff.net>
---
I went with "corrupted" here for consistency with the others (versus
"corrupt"). If we think there's a reason to prefer one over the other,
I'm happy to take a patch on top fixing all of them.
commit-graph.c | 6 +++---
t/t5318-commit-graph.sh | 10 +++++-----
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index d9fc08de86..989ebbe816 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -292,15 +292,15 @@ static int verify_commit_graph_lite(struct commit_graph *g)
* itself.
*/
if (!g->chunk_oid_fanout) {
- error("commit-graph is missing the OID Fanout chunk");
+ error(_("commit-graph required OID fanout chunk missing or corrupted"));
return 1;
}
if (!g->chunk_oid_lookup) {
- error("commit-graph is missing the OID Lookup chunk");
+ error(_("commit-graph required OID lookup chunk missing or corrupted"));
return 1;
}
if (!g->chunk_commit_data) {
- error("commit-graph is missing the Commit Data chunk");
+ error(_("commit-graph required commit data chunk missing or corrupted"));
return 1;
}
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index d4fc65e078..affb959d64 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -540,17 +540,17 @@ test_expect_success 'detect low chunk count' '
test_expect_success 'detect missing OID fanout chunk' '
corrupt_graph_and_verify $GRAPH_BYTE_OID_FANOUT_ID "\0" \
- "missing the OID Fanout chunk"
+ "commit-graph required OID fanout chunk missing or corrupted"
'
test_expect_success 'detect missing OID lookup chunk' '
corrupt_graph_and_verify $GRAPH_BYTE_OID_LOOKUP_ID "\0" \
- "missing the OID Lookup chunk"
+ "commit-graph required OID lookup chunk missing or corrupted"
'
test_expect_success 'detect missing commit data chunk' '
corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_DATA_ID "\0" \
- "missing the Commit Data chunk"
+ "commit-graph required commit data chunk missing or corrupted"
'
test_expect_success 'detect incorrect fanout' '
@@ -842,7 +842,7 @@ test_expect_success 'reader notices too-small oid fanout chunk' '
check_corrupt_chunk OIDF clear $(printf "000000%02x" $(test_seq 250)) &&
cat >expect.err <<-\EOF &&
error: commit-graph oid fanout chunk is wrong size
- error: commit-graph is missing the OID Fanout chunk
+ error: commit-graph required OID fanout chunk missing or corrupted
EOF
test_cmp expect.err err
'
@@ -874,7 +874,7 @@ test_expect_success 'reader notices too-small commit data chunk' '
check_corrupt_chunk CDAT clear 00000000 &&
cat >expect.err <<-\EOF &&
error: commit-graph commit data chunk is wrong size
- error: commit-graph is missing the Commit Data chunk
+ error: commit-graph required commit data chunk missing or corrupted
EOF
test_cmp expect.err err
'
--
2.43.0.rc1.572.g273fc7bed6
^ permalink raw reply related
* [PATCH 5/9] commit-graph: abort as soon as we see a bogus chunk
From: Jeff King @ 2023-11-09 7:17 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
The code to read commit-graph files tries to read all of the required
chunks, but doesn't abort if we can't find one (or if it's corrupted).
It's only at the end of reading the file that we then do some sanity
checks for NULL entries. But it's preferable to detect the errors and
bail immediately, for a few reasons:
1. It's less error-prone. It's easy in the reader functions to flag an
error but still end up setting some struct fields (an error I in
fact made while working on this patch series).
2. It's safer. Since verifying some chunks depends on the values of
other chunks, we may be depending on not-yet-verified data. I don't
know offhand of any case where this can cause problems, but it's
one less subtle thing to worry about in the reader code.
3. It prevents the user from seeing nonsense errors. If we're missing
an OIDL chunk, then g->num_commits will be zero. And so we may
complain that the size of our CDAT chunk (which should have a
fixed-size record for each commit) is wrong unless it's also zero.
But that's misleading; the problem is the missing OIDL chunk; the
CDAT one might be fine!
So let's just check the return value from read_chunk(). This is exactly
how the midx chunk-reading code does it.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 29 +++++++++++++----------------
1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 989ebbe816..374575b484 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -291,19 +291,6 @@ static int verify_commit_graph_lite(struct commit_graph *g)
* over g->num_commits, or runs a checksum on the commit-graph
* itself.
*/
- if (!g->chunk_oid_fanout) {
- error(_("commit-graph required OID fanout chunk missing or corrupted"));
- return 1;
- }
- if (!g->chunk_oid_lookup) {
- error(_("commit-graph required OID lookup chunk missing or corrupted"));
- return 1;
- }
- if (!g->chunk_commit_data) {
- error(_("commit-graph required commit data chunk missing or corrupted"));
- return 1;
- }
-
for (i = 0; i < 255; i++) {
uint32_t oid_fanout1 = ntohl(g->chunk_oid_fanout[i]);
uint32_t oid_fanout2 = ntohl(g->chunk_oid_fanout[i + 1]);
@@ -462,9 +449,19 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
GRAPH_HEADER_SIZE, graph->num_chunks, 1))
goto free_and_return;
- read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph);
- read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);
- read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph);
+ if (read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph)) {
+ error(_("commit-graph required OID fanout chunk missing or corrupted"));
+ goto free_and_return;
+ }
+ if (read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph)) {
+ error(_("commit-graph required OID lookup chunk missing or corrupted"));
+ goto free_and_return;
+ }
+ if (read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph)) {
+ error(_("commit-graph required commit data chunk missing or corrupted"));
+ goto free_and_return;
+ }
+
pair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges,
&graph->chunk_extra_edges_size);
pair_chunk(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs,
--
2.43.0.rc1.572.g273fc7bed6
^ permalink raw reply related
* [PATCH 6/9] commit-graph: use fanout value for graph size
From: Jeff King @ 2023-11-09 7:24 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
Commit-graph, midx, and pack idx files all have both a lookup table of
oids and an oid fanout table. In midx and pack idx files, we take the
final entry of the fanout table as the source of truth for the number of
entries, and then verify that the size of the lookup table matches that.
But for commit-graph files, we do the opposite: we use the size of the
lookup table as the source of truth, and then check the final fanout
entry against it.
As noted in 4169d89645 (commit-graph: check consistency of fanout
table, 2023-10-09), either is correct. But there are a few reasons to
prefer the fanout table as the source of truth:
1. The fanout entries are 32-bits on disk, and that defines the
maximum number of entries we can store. But since the size of the
lookup table is only bounded by the filesystem, it can be much
larger. And hence computing it as the commit-graph does means that
we may truncate the result when storing it in a uint32_t.
2. We read the fanout first, then the lookup table. If we're verifying
the chunks as we read them, then we'd want to take the fanout as
truth (we have nothing yet to check it against) and then we can
check that the lookup table matches what we already know.
3. It is pointlessly inconsistent with the midx and pack idx code.
Since the three have to do similar size and bounds checks, it is
easier to reason about all three if they use the same approach.
So this patch moves the assignment of g->num_commits to the fanout
parser, and then we can check the size of the lookup chunk as soon as we
try to load it.
There's already a test covering this situation, which munges the final
fanout entry to 2^32-1. In the current code we complain that it does not
agree with the table size. But now that we treat the munged value as the
source of truth, we'll complain that the lookup table is the wrong size
(again, either is correct). So we'll have to update the message we
expect (and likewise for an earlier test which does similar munging).
There's a similar test for this situation on the midx side, but rather
than making a very-large fanout value, it just truncates the lookup
table. We could do that here, too, but the very-large fanout value
actually shows an interesting corner case. On a 32-bit system,
multiplying to find the expected table size would cause an integer
overflow. Using st_mult() would detect that, but cause us to die()
rather than falling back to the non-graph code path. Checking the size
using division (as we do with existing chunk-size checks) avoids the
overflow entirely, and the test demonstrates this when run on a 32-bit
system.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 8 +++-----
t/t5318-commit-graph.sh | 5 +++--
2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 374575b484..094814c2ba 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -300,10 +300,6 @@ static int verify_commit_graph_lite(struct commit_graph *g)
return 1;
}
}
- if (ntohl(g->chunk_oid_fanout[255]) != g->num_commits) {
- error("commit-graph oid table and fanout disagree on size");
- return 1;
- }
return 0;
}
@@ -315,6 +311,7 @@ static int graph_read_oid_fanout(const unsigned char *chunk_start,
if (chunk_size != 256 * sizeof(uint32_t))
return error("commit-graph oid fanout chunk is wrong size");
g->chunk_oid_fanout = (const uint32_t *)chunk_start;
+ g->num_commits = ntohl(g->chunk_oid_fanout[255]);
return 0;
}
@@ -323,7 +320,8 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,
{
struct commit_graph *g = data;
g->chunk_oid_lookup = chunk_start;
- g->num_commits = chunk_size / g->hash_len;
+ if (chunk_size / g->hash_len != g->num_commits)
+ return error(_("commit-graph OID lookup chunk is the wrong size"));
return 0;
}
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index affb959d64..8bd7fcefb5 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -560,7 +560,7 @@ test_expect_success 'detect incorrect fanout' '
test_expect_success 'detect incorrect fanout final value' '
corrupt_graph_and_verify $GRAPH_BYTE_FANOUT2 "\01" \
- "oid table and fanout disagree on size"
+ "OID lookup chunk is the wrong size"
'
test_expect_success 'detect incorrect OID order' '
@@ -850,7 +850,8 @@ test_expect_success 'reader notices too-small oid fanout chunk' '
test_expect_success 'reader notices fanout/lookup table mismatch' '
check_corrupt_chunk OIDF 1020 "FFFFFFFF" &&
cat >expect.err <<-\EOF &&
- error: commit-graph oid table and fanout disagree on size
+ error: commit-graph OID lookup chunk is the wrong size
+ error: commit-graph required OID lookup chunk missing or corrupted
EOF
test_cmp expect.err err
'
--
2.43.0.rc1.572.g273fc7bed6
^ permalink raw reply related
* [PATCH 7/9] commit-graph: check order while reading fanout chunk
From: Jeff King @ 2023-11-09 7:25 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
We read the fanout chunk, storing a pointer to it, but only confirm that
the entries are monotonic in a final "lite" verification step. Let's
move that into the actual OIDF chunk callback, so that we can report
problems immediately (for all the reasons given in the previous
"commit-graph: abort as soon as we see a bogus chunk" commit).
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 25 +++++++++++++------------
t/t5318-commit-graph.sh | 1 +
2 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 094814c2ba..4ba523cd15 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -277,8 +277,6 @@ struct commit_graph *load_commit_graph_one_fd_st(struct repository *r,
static int verify_commit_graph_lite(struct commit_graph *g)
{
- int i;
-
/*
* Basic validation shared between parse_commit_graph()
* which'll be called every time the graph is used, and the
@@ -291,27 +289,30 @@ static int verify_commit_graph_lite(struct commit_graph *g)
* over g->num_commits, or runs a checksum on the commit-graph
* itself.
*/
- for (i = 0; i < 255; i++) {
- uint32_t oid_fanout1 = ntohl(g->chunk_oid_fanout[i]);
- uint32_t oid_fanout2 = ntohl(g->chunk_oid_fanout[i + 1]);
-
- if (oid_fanout1 > oid_fanout2) {
- error("commit-graph fanout values out of order");
- return 1;
- }
- }
-
return 0;
}
static int graph_read_oid_fanout(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
struct commit_graph *g = data;
+ int i;
+
if (chunk_size != 256 * sizeof(uint32_t))
return error("commit-graph oid fanout chunk is wrong size");
g->chunk_oid_fanout = (const uint32_t *)chunk_start;
g->num_commits = ntohl(g->chunk_oid_fanout[255]);
+
+ for (i = 0; i < 255; i++) {
+ uint32_t oid_fanout1 = ntohl(g->chunk_oid_fanout[i]);
+ uint32_t oid_fanout2 = ntohl(g->chunk_oid_fanout[i + 1]);
+
+ if (oid_fanout1 > oid_fanout2) {
+ error("commit-graph fanout values out of order");
+ return 1;
+ }
+ }
+
return 0;
}
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 8bd7fcefb5..7fe7c72a87 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -867,6 +867,7 @@ test_expect_success 'reader notices out-of-bounds fanout' '
check_corrupt_chunk OIDF 0 $(printf "%02x000000" $(test_seq 0 254)) &&
cat >expect.err <<-\EOF &&
error: commit-graph fanout values out of order
+ error: commit-graph required OID fanout chunk missing or corrupted
EOF
test_cmp expect.err err
'
--
2.43.0.rc1.572.g273fc7bed6
^ permalink raw reply related
* [PATCH 8/9] commit-graph: drop verify_commit_graph_lite()
From: Jeff King @ 2023-11-09 7:25 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
As we've moved all of the checks from this function directly into the
chunk-reading code used by the caller (and there is only one caller), we
can just drop it entirely.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 20 --------------------
1 file changed, 20 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 4ba523cd15..6fbfe4c68e 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -275,23 +275,6 @@ struct commit_graph *load_commit_graph_one_fd_st(struct repository *r,
return ret;
}
-static int verify_commit_graph_lite(struct commit_graph *g)
-{
- /*
- * Basic validation shared between parse_commit_graph()
- * which'll be called every time the graph is used, and the
- * much more expensive verify_commit_graph() used by
- * "commit-graph verify".
- *
- * There should only be very basic checks here to ensure that
- * we don't e.g. segfault in fill_commit_in_graph(), but
- * because this is a very hot codepath nothing that e.g. loops
- * over g->num_commits, or runs a checksum on the commit-graph
- * itself.
- */
- return 0;
-}
-
static int graph_read_oid_fanout(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
@@ -495,9 +478,6 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
oidread(&graph->oid, graph->data + graph->data_len - graph->hash_len);
- if (verify_commit_graph_lite(graph))
- goto free_and_return;
-
free_chunkfile(cf);
return graph;
--
2.43.0.rc1.572.g273fc7bed6
^ permalink raw reply related
* [PATCH 9/9] commit-graph: mark chunk error messages for translation
From: Jeff King @ 2023-11-09 7:26 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
The patches from f32af12cee (Merge branch 'jk/chunk-bounds', 2023-10-23)
added many new untranslated error messages. While it's unlikely for most
users to see these messages at all, most of the other commit-graph error
messages are translated (and likewise for the matching midx messages).
Let's mark them all for consistency (and to help any poor unfortunate
user who does manage to find a broken graph file).
Signed-off-by: Jeff King <peff@peff.net>
---
The "wrong size" ones may be dropped eventually if we have a generic
pair_chunk_expect() API, but it seemed easier to just fix them all
mechanically in one go.
commit-graph.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 6fbfe4c68e..acac9bf6e1 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -282,7 +282,7 @@ static int graph_read_oid_fanout(const unsigned char *chunk_start,
int i;
if (chunk_size != 256 * sizeof(uint32_t))
- return error("commit-graph oid fanout chunk is wrong size");
+ return error(_("commit-graph oid fanout chunk is wrong size"));
g->chunk_oid_fanout = (const uint32_t *)chunk_start;
g->num_commits = ntohl(g->chunk_oid_fanout[255]);
@@ -291,7 +291,7 @@ static int graph_read_oid_fanout(const unsigned char *chunk_start,
uint32_t oid_fanout2 = ntohl(g->chunk_oid_fanout[i + 1]);
if (oid_fanout1 > oid_fanout2) {
- error("commit-graph fanout values out of order");
+ error(_("commit-graph fanout values out of order"));
return 1;
}
}
@@ -314,7 +314,7 @@ static int graph_read_commit_data(const unsigned char *chunk_start,
{
struct commit_graph *g = data;
if (chunk_size / GRAPH_DATA_WIDTH != g->num_commits)
- return error("commit-graph commit data chunk is wrong size");
+ return error(_("commit-graph commit data chunk is wrong size"));
g->chunk_commit_data = chunk_start;
return 0;
}
@@ -324,7 +324,7 @@ static int graph_read_generation_data(const unsigned char *chunk_start,
{
struct commit_graph *g = data;
if (chunk_size / sizeof(uint32_t) != g->num_commits)
- return error("commit-graph generations chunk is wrong size");
+ return error(_("commit-graph generations chunk is wrong size"));
g->chunk_generation_data = chunk_start;
return 0;
}
@@ -334,7 +334,7 @@ static int graph_read_bloom_index(const unsigned char *chunk_start,
{
struct commit_graph *g = data;
if (chunk_size / 4 != g->num_commits) {
- warning("commit-graph changed-path index chunk is too small");
+ warning(_("commit-graph changed-path index chunk is too small"));
return -1;
}
g->chunk_bloom_indexes = chunk_start;
@@ -348,8 +348,8 @@ static int graph_read_bloom_data(const unsigned char *chunk_start,
uint32_t hash_version;
if (chunk_size < BLOOMDATA_CHUNK_HEADER_SIZE) {
- warning("ignoring too-small changed-path chunk"
- " (%"PRIuMAX" < %"PRIuMAX") in commit-graph file",
+ warning(_("ignoring too-small changed-path chunk"
+ " (%"PRIuMAX" < %"PRIuMAX") in commit-graph file"),
(uintmax_t)chunk_size,
(uintmax_t)BLOOMDATA_CHUNK_HEADER_SIZE);
return -1;
@@ -605,7 +605,7 @@ int open_commit_graph_chain(const char *chain_file,
/* treat empty files the same as missing */
errno = ENOENT;
} else {
- warning("commit-graph chain file too small");
+ warning(_("commit-graph chain file too small"));
errno = EINVAL;
}
return 0;
@@ -946,7 +946,7 @@ static int fill_commit_in_graph(struct repository *r,
parent_data_pos = edge_value & GRAPH_EDGE_LAST_MASK;
do {
if (g->chunk_extra_edges_size / sizeof(uint32_t) <= parent_data_pos) {
- error("commit-graph extra-edges pointer out of bounds");
+ error(_("commit-graph extra-edges pointer out of bounds"));
free_commit_list(item->parents);
item->parents = NULL;
item->object.parsed = 0;
--
2.43.0.rc1.572.g273fc7bed6
^ permalink raw reply related
* Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Jeff King @ 2023-11-09 7:32 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <e4c75c492dd89fd7464db2b3028b2bb9e6addbf8.1699513524.git.ps@pks.im>
On Thu, Nov 09, 2023 at 08:09:52AM +0100, Patrick Steinhardt wrote:
> -for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
> +for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' \
> + '/usr/sbin/apache2' \
> + "$(command -v httpd)" \
> + "$(command -v apache2)"
> do
> - if test -x "$DEFAULT_HTTPD_PATH"
> + if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH"
Sorry to be a pedant, but I'm not sure if we might have portability
problems with "-a". It's an XSI extension, and POSIX labels it as
obsolescent because it can create parsing ambiguities.
We do have a few instances, but only in corners of the test suite that
probably don't get as much exposure (t/perf and valgrind/valgrind.sh).
So maybe not worth worrying about, but it's easy to write it as:
if test -n "$DEFAULT_HTTPD_PATH" && test -x "$DEFAULT_HTTPD_PATH"
-Peff
^ permalink raw reply
* Re: [PATCH v3 3/3] t9164: fix inability to find basename(1) in Subversion hooks
From: Jeff King @ 2023-11-09 7:35 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <6891e2541552073c6827d81d2b761252f253bea2.1699513524.git.ps@pks.im>
On Thu, Nov 09, 2023 at 08:10:01AM +0100, Patrick Steinhardt wrote:
> + # Subversion hooks run with an empty environment by default. We thus
> + # need to propagate PATH so that we can find executables.
> + cat >"$rawsvnrepo/conf/hooks-env" <<-EOF
> + [default]
> + PATH = ${PATH}
> + EOF
This is so much less ugly than the shell shenanigans discussed in the
earlier round. Thanks for finding it. :)
The ${PATH} here is interpolated by the here-doc. I guess it's possible
somebody's exotic PATH could break the svn conf syntax, but it's
probably not worth worrying about.
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/3] t: improve compatibility with NixOS
From: Jeff King @ 2023-11-09 7:36 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <cover.1699513524.git.ps@pks.im>
On Thu, Nov 09, 2023 at 08:09:47AM +0100, Patrick Steinhardt wrote:
> this is the third version of my patch series to improve compatibility of
> our tests with NixOS.
>
> Changes compared to v2:
>
> - Patch 1: We now check for both httpd and apache2 binaries via
> PATH.
>
> - Patch 1: We're a bit more defensive and will check whether
> variables are empty before passing them to either `test -d` or
> `test -x`.
>
> - Patch 3: Clarified why PATH is unset.
>
> - Patch 3: Instead of writing PATH into the hook we now write it
> into the `hook-env` configuration file read by Subversion.
Thanks. This looks good to me, modulo a minor shell nit I mentioned in
the first patch.
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Patrick Steinhardt @ 2023-11-09 7:36 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20231109073250.GA2698227@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]
On Thu, Nov 09, 2023 at 02:32:50AM -0500, Jeff King wrote:
> On Thu, Nov 09, 2023 at 08:09:52AM +0100, Patrick Steinhardt wrote:
>
> > -for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
> > +for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' \
> > + '/usr/sbin/apache2' \
> > + "$(command -v httpd)" \
> > + "$(command -v apache2)"
> > do
> > - if test -x "$DEFAULT_HTTPD_PATH"
> > + if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH"
>
> Sorry to be a pedant, but I'm not sure if we might have portability
> problems with "-a". It's an XSI extension, and POSIX labels it as
> obsolescent because it can create parsing ambiguities.
>
> We do have a few instances, but only in corners of the test suite that
> probably don't get as much exposure (t/perf and valgrind/valgrind.sh).
> So maybe not worth worrying about, but it's easy to write it as:
Yeah, I was grepping for it in our codebase and saw other occurrences,
so I assumed it was fair game. If we're going to convert it to the
below, how about I send another patch on top that also converts the
preexisting instances so that the next one grepping for it isn't going
to repeat the same mistake?
Patrick
> if test -n "$DEFAULT_HTTPD_PATH" && test -x "$DEFAULT_HTTPD_PATH"
>
> -Peff
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Junio C Hamano @ 2023-11-09 7:46 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Jeff King, git
In-Reply-To: <ZUyMFZ7c9_rlu5lk@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> Yeah, I was grepping for it in our codebase and saw other occurrences,
> so I assumed it was fair game. If we're going to convert it to the
> below, how about I send another patch on top that also converts the
> preexisting instances so that the next one grepping for it isn't going
> to repeat the same mistake?
Yup, an independent clean-up would be fine. Now we need to find a
way to give better visibility to CodingGuidelines, which already
says this:
- We do not write our "test" command with "-a" and "-o" and use "&&"
or "||" to concatenate multiple "test" commands instead, because
the use of "-a/-o" is often error-prone. E.g.
test -n "$x" -a "$a" = "$b"
is buggy and breaks when $x is "=", but
test -n "$x" && test "$a" = "$b"
does not have such a problem.
^ permalink raw reply
* Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Jeff King @ 2023-11-09 7:48 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <ZUyMFZ7c9_rlu5lk@tanuki>
On Thu, Nov 09, 2023 at 08:36:53AM +0100, Patrick Steinhardt wrote:
> > Sorry to be a pedant, but I'm not sure if we might have portability
> > problems with "-a". It's an XSI extension, and POSIX labels it as
> > obsolescent because it can create parsing ambiguities.
> >
> > We do have a few instances, but only in corners of the test suite that
> > probably don't get as much exposure (t/perf and valgrind/valgrind.sh).
> > So maybe not worth worrying about, but it's easy to write it as:
>
> Yeah, I was grepping for it in our codebase and saw other occurrences,
> so I assumed it was fair game. If we're going to convert it to the
> below, how about I send another patch on top that also converts the
> preexisting instances so that the next one grepping for it isn't going
> to repeat the same mistake?
I would be very happy with that. :)
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2023, #04; Thu, 9)
From: Jeff King @ 2023-11-09 7:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Taylor Blau
In-Reply-To: <xmqq34xg5ek3.fsf@gitster.g>
On Thu, Nov 09, 2023 at 02:40:28AM +0900, Junio C Hamano wrote:
> * tb/pair-chunk-expect-size (2023-10-14) 8 commits
> - midx: read `OOFF` chunk with `pair_chunk_expect()`
> - midx: read `OIDL` chunk with `pair_chunk_expect()`
> - midx: read `OIDF` chunk with `pair_chunk_expect()`
> - commit-graph: read `BIDX` chunk with `pair_chunk_expect()`
> - commit-graph: read `GDAT` chunk with `pair_chunk_expect()`
> - commit-graph: read `CDAT` chunk with `pair_chunk_expect()`
> - commit-graph: read `OIDF` chunk with `pair_chunk_expect()`
> - chunk-format: introduce `pair_chunk_expect()` helper
>
> Code clean-up for jk/chunk-bounds topic.
>
> Comments?
> source: <45cac29403e63483951f7766c6da3c022c68d9f0.1697225110.git.me@ttaylorr.com>
> source: <cover.1697225110.git.me@ttaylorr.com>
Sorry it took me a while to circle back to this topic. I posted a
competing series just now in:
https://lore.kernel.org/git/20231109070310.GA2697602@coredump.intra.peff.net/
that I think should take precedence (and would require some reworking of
Taylor's patches, so you'd just eject them in the meantime).
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox