* [PATCH v3 2/8] sequencer.c: split up sequencer_remove_state()
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v3-0.8-00000000000-20230118T160600Z-avarab@gmail.com>
Split off the free()-ing in sequencer_remove_state() into a utility
function, which will be adjusted and called independent of the other
code in sequencer_remove_state() in a subsequent commit.
The only functional change here is changing the "int" to a "size_t",
which is the correct type, as "xopts_nr" is a "size_t".
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
sequencer.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index bcb662e23be..d385bea2bed 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -351,10 +351,22 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
return buf.buf;
}
+static void replay_opts_release(struct replay_opts *opts)
+{
+ free(opts->gpg_sign);
+ free(opts->reflog_action);
+ free(opts->default_strategy);
+ free(opts->strategy);
+ for (size_t i = 0; i < opts->xopts_nr; i++)
+ free(opts->xopts[i]);
+ free(opts->xopts);
+ strbuf_release(&opts->current_fixups);
+}
+
int sequencer_remove_state(struct replay_opts *opts)
{
struct strbuf buf = STRBUF_INIT;
- int i, ret = 0;
+ int ret = 0;
if (is_rebase_i(opts) &&
strbuf_read_file(&buf, rebase_path_refs_to_delete(), 0) > 0) {
@@ -373,14 +385,7 @@ int sequencer_remove_state(struct replay_opts *opts)
}
}
- free(opts->gpg_sign);
- free(opts->reflog_action);
- free(opts->default_strategy);
- free(opts->strategy);
- for (i = 0; i < opts->xopts_nr; i++)
- free(opts->xopts[i]);
- free(opts->xopts);
- strbuf_release(&opts->current_fixups);
+ replay_opts_release(opts);
strbuf_reset(&buf);
strbuf_addstr(&buf, get_dir(opts));
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v3 1/8] rebase: use "cleanup" pattern in do_interactive_rebase()
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v3-0.8-00000000000-20230118T160600Z-avarab@gmail.com>
Use a "goto cleanup" pattern in do_interactive_rebase(). This
eliminates some duplicated free() code added in 53bbcfbde7c (rebase
-i: implement the main part of interactive rebase as a builtin,
2018-09-27), and sets us up for a subsequent commit which'll make
further use of the "cleanup" label.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/rebase.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 1481c5b6a5b..7141fd5e0c1 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -256,7 +256,7 @@ static void split_exec_commands(const char *cmd, struct string_list *commands)
static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
{
- int ret;
+ int ret = -1;
char *revisions = NULL, *shortrevisions = NULL;
struct strvec make_script_args = STRVEC_INIT;
struct todo_list todo_list = TODO_LIST_INIT;
@@ -265,16 +265,12 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head->object.oid,
&revisions, &shortrevisions))
- return -1;
+ goto cleanup;
if (init_basic_state(&replay,
opts->head_name ? opts->head_name : "detached HEAD",
- opts->onto, &opts->orig_head->object.oid)) {
- free(revisions);
- free(shortrevisions);
-
- return -1;
- }
+ opts->onto, &opts->orig_head->object.oid))
+ goto cleanup;
if (!opts->upstream && opts->squash_onto)
write_file(path_squash_onto(), "%s\n",
@@ -304,6 +300,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
opts->autosquash, opts->update_refs, &todo_list);
}
+cleanup:
string_list_clear(&commands, 0);
free(revisions);
free(shortrevisions);
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v3 0/8] sequencer API & users: fix widespread leaks
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v2-0.9-00000000000-20230112T124201Z-avarab@gmail.com>
This series fixes various widespread leaks in the sequencer and its
users (rebase, revert, cherry-pick). As a result 18 tests become
leak-free in their entirety.
See the v1 for a longer general summary:
https://lore.kernel.org/git/cover-00.10-00000000000-20221230T071741Z-avarab@gmail.com/
Changes since v2:
* Reword/amend commit messages for some of the functional changes in
v1..v2.
* Remove leftover "opts->xopts_nr = 0" now that we don't
FREE_AND_NULL() anymore.
* Drop the "squash_onto_name" refactoring to a "to_free"
* Instead add a new "keep_base_onto_name" in the next commit. I'd
missed that if both options were provided we'd die(), so that
free() wasn't needed before re-assignment, as Phillip pointed
out...
CI & branch for this available at:
https://github.com/avar/git/tree/avar/leak-fixes-sequencer-rebase-3
Ævar Arnfjörð Bjarmason (8):
rebase: use "cleanup" pattern in do_interactive_rebase()
sequencer.c: split up sequencer_remove_state()
rebase & sequencer API: fix get_replay_opts() leak in "rebase"
builtin/revert.c: move free-ing of "revs" to replay_opts_release()
builtin/rebase.c: fix "options.onto_name" leak
sequencer.c: always free() the "msgbuf" in do_pick_commit()
builtin/rebase.c: free() "options.strategy_opts"
commit.c: free() revs.commit in get_fork_point()
builtin/rebase.c | 22 ++++++++------
builtin/revert.c | 8 ++---
commit.c | 1 +
sequencer.c | 42 ++++++++++++++++----------
sequencer.h | 1 +
t/t3405-rebase-malformed.sh | 1 +
t/t3412-rebase-root.sh | 1 +
t/t3416-rebase-onto-threedots.sh | 1 +
t/t3419-rebase-patch-id.sh | 1 +
t/t3423-rebase-reword.sh | 1 +
t/t3425-rebase-topology-merges.sh | 2 ++
t/t3431-rebase-fork-point.sh | 1 +
t/t3432-rebase-fast-forward.sh | 1 +
t/t3437-rebase-fixup-options.sh | 1 +
t/t3438-rebase-broken-files.sh | 2 ++
t/t3501-revert-cherry-pick.sh | 1 +
t/t3502-cherry-pick-merge.sh | 1 +
t/t3503-cherry-pick-root.sh | 1 +
t/t3506-cherry-pick-ff.sh | 1 +
t/t3511-cherry-pick-x.sh | 1 +
t/t7402-submodule-rebase.sh | 1 +
t/t9106-git-svn-commit-diff-clobber.sh | 1 -
t/t9164-git-svn-dcommit-concurrent.sh | 1 -
23 files changed, 61 insertions(+), 33 deletions(-)
Range-diff against v2:
1: d0a0524f3d4 = 1: b223429df33 rebase: use "cleanup" pattern in do_interactive_rebase()
2: c4eaa8dfef4 = 2: 00c7f04363f sequencer.c: split up sequencer_remove_state()
3: f06f565ceaf ! 3: e4a96898a68 rebase & sequencer API: fix get_replay_opts() leak in "rebase"
@@ Commit message
get_replay_opts() function in "builtin/rebase.c". See [1] for the
initial addition of get_replay_opts().
- To safely call our new replay_opts_release() we'll need to change all
- the free() to a FREE_AND_NULL(), and set "xopts_nr" to "0" after we
- loop over it and free() it (the free() in the loop doesn't need to be
- a FREE_AND_NULL()).
+ To safely call our new replay_opts_release() we'll need to stop
+ calling it in sequencer_remove_state(), and instead call it where we
+ allocate the "struct replay_opts" itself.
This is because in e.g. do_interactive_rebase() we construct a "struct
replay_opts" with "get_replay_opts()", and then call
@@ Commit message
But if we encounter errors anywhere along the way we'd punt out early,
and not free() the memory we allocated. Remembering whether we
- previously called sequencer_remove_state() would be a hassle, so let's
- make it safe to re-invoke replay_opts_release() instead.
+ previously called sequencer_remove_state() would be a hassle.
- I experimented with a change to be more paranoid instead, i.e. to
- exhaustively check our state via an enum. We could make sure that we:
-
- - Only allow calling "replay_opts_release()" after
- "sequencer_remove_state()", but not the other way around.
-
- - Forbid invoking either function twice in a row.
-
- But such paranoia isn't warranted here, let's instead take the easy
- way out and FREE_AND_NULL() this.
+ Using a FREE_AND_NULL() pattern would also work, as it would be safe
+ replay_opts_release() repeatedly, but let's fix this properly instead,
+ by having the owner of the data free() it.
See [2] for the initial implementation of "sequencer_remove_state()",
which assumed that it should be removing the full (including on-disk)
@@ sequencer.c: static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
{
free(opts->gpg_sign);
free(opts->reflog_action);
-@@ sequencer.c: static void replay_opts_release(struct replay_opts *opts)
- free(opts->strategy);
- for (size_t i = 0; i < opts->xopts_nr; i++)
- free(opts->xopts[i]);
-+ opts->xopts_nr = 0;
- free(opts->xopts);
- strbuf_release(&opts->current_fixups);
- }
@@ sequencer.c: int sequencer_remove_state(struct replay_opts *opts)
}
}
4: e83bdfab046 ! 4: 9f72cc6e46b builtin/revert.c: move free-ing of "revs" to replay_opts_release()
@@ Commit message
rather than having these users reach into the struct to free its
individual members.
- As explained in earlier change we should be using FREE_AND_NULL() in
- replay_opts_release() rather than free().
-
1. d1ec656d68f (cherry-pick: free "struct replay_opts" members,
2022-11-08)
2. fd74ac95ac3 (revert: free "struct replay_opts" members, 2022-07-01)
@@ builtin/revert.c: int cmd_cherry_pick(int argc, const char **argv, const char *p
## sequencer.c ##
@@ sequencer.c: void replay_opts_release(struct replay_opts *opts)
- opts->xopts_nr = 0;
+ free(opts->xopts[i]);
free(opts->xopts);
strbuf_release(&opts->current_fixups);
+ if (opts->revs)
5: 4fea2b77c6d < -: ----------- builtin/rebase.c: rename "squash_onto_name" to "to_free"
6: 898bb7698fc ! 5: 3d5c3152f69 builtin/rebase.c: fix "options.onto_name" leak
@@ Metadata
## Commit message ##
builtin/rebase.c: fix "options.onto_name" leak
- In [1] we started saving away the earlier xstrdup()'d
- "options.onto_name" assignment to free() it, but when [2] added this
- "keep_base" branch it didn't free() the already assigned value before
- re-assigning to "options.onto_name". Let's do that, and fix the memory
- leak.
+ Similar to the existing "squash_onto_name" added in [1] we need to
+ free() the xstrdup()'d "options.onto.name" added for "--keep-base" in
+ [2]..
1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
2. 414d924beb4 (rebase: teach rebase --keep-base, 2019-08-27)
@@ Commit message
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## builtin/rebase.c ##
+@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
+ struct string_list strategy_options = STRING_LIST_INIT_NODUP;
+ struct object_id squash_onto;
+ char *squash_onto_name = NULL;
++ char *keep_base_onto_name = NULL;
+ int reschedule_failed_exec = -1;
+ int allow_preemptive_ff = 1;
+ int preserve_merges_selected = 0;
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
strbuf_addstr(&buf, options.upstream_name);
strbuf_addstr(&buf, "...");
strbuf_addstr(&buf, branch_name);
- options.onto_name = xstrdup(buf.buf);
-+ free(to_free);
-+ options.onto_name = to_free = xstrdup(buf.buf);
++ options.onto_name = keep_base_onto_name = xstrdup(buf.buf);
} else if (!options.onto_name)
options.onto_name = options.upstream_name;
if (strstr(options.onto_name, "...")) {
+@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
+ free(options.strategy);
+ strbuf_release(&options.git_format_patch_opt);
+ free(squash_onto_name);
++ free(keep_base_onto_name);
+ string_list_clear(&exec, 0);
+ string_list_clear(&strategy_options, 0);
+ return !!ret;
## t/t3416-rebase-onto-threedots.sh ##
@@ t/t3416-rebase-onto-threedots.sh: test_description='git rebase --onto A...B'
7: fb38dc873f9 = 6: c07dc006c6d sequencer.c: always free() the "msgbuf" in do_pick_commit()
8: d4b0e2a5c83 ! 7: ee8262ab22a builtin/rebase.c: free() "options.strategy_opts"
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
free(options.strategy);
+ free(options.strategy_opts);
strbuf_release(&options.git_format_patch_opt);
- free(to_free);
- string_list_clear(&exec, 0);
+ free(squash_onto_name);
+ free(keep_base_onto_name);
9: fd9c7a5547c = 8: 84343ea6bf6 commit.c: free() revs.commit in get_fork_point()
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply
* Re: [PATCH] git: replace strbuf_addstr with strbuf_addch for all strings of length 2
From: Junio C Hamano @ 2023-01-18 16:04 UTC (permalink / raw)
To: Rose via GitGitGadget; +Cc: git, Seija Kijin
In-Reply-To: <pull.1436.git.git.1673992498572.gitgitgadget@gmail.com>
"Rose via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Seija Kijin <doremylover123@gmail.com>
>
> This helps reduce overhead of calculating the length
Have you measured how much overhead this change is saving, or is
this a 300 line e-mail message that churns code without giving us
any measurable improvement?
There also seem to be some totally unrelated changes of dubious
merit hidden in these patches (see below).
> Subject: Re: [PATCH] git: replace strbuf_addstr with strbuf_addch for all strings of length 2
I think you are touching strings of length 1, not 2. Running
strlen() on such a string returns will return 1 without counting the
terminating NUL.
> diff --git a/builtin/am.c b/builtin/am.c
> index 7e88d2426d7..c96886e0433 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -329,13 +329,11 @@ static void write_author_script(const struct am_state *state)
>
> strbuf_addstr(&sb, "GIT_AUTHOR_NAME=");
> sq_quote_buf(&sb, state->author_name);
> - strbuf_addch(&sb, '\n');
>
> - strbuf_addstr(&sb, "GIT_AUTHOR_EMAIL=");
> + strbuf_addstr(&sb, "\nGIT_AUTHOR_EMAIL=");
> sq_quote_buf(&sb, state->author_email);
> - strbuf_addch(&sb, '\n');
>
> - strbuf_addstr(&sb, "GIT_AUTHOR_DATE=");
> + strbuf_addstr(&sb, "\nGIT_AUTHOR_DATE=");
> sq_quote_buf(&sb, state->author_date);
> strbuf_addch(&sb, '\n');
This may reduce the number of lines, but markedly worsens the
readability of the resulting code. Each of the three-line blocks in
the original used to be logically complete and independent unit, but
now each of them depend on what the last block wants.
In any case, this has nothing to do with "addstr() of constant string
can be replaced with add() with a constant length or addch() of a
sing character to make it unnecessary to compute the length".
By the way, the current implementation of strbuf_addstr() looks like
this:
static inline void strbuf_addstr(struct strbuf *sb, const char *s)
{
strbuf_add(sb, s, strlen(s));
}
Decent optimizing compilers should be able to see through the code
like this you write:
strbuf_addstr(&sb, "constant");
which becomes
strbuf_add(&sb, "constant", strlen("constant"))
when inlined, and realize strlen("constant") can be computed at
compile time, turning it into
strbuf_add(&sb, "constant", 8);
That way, people can make their strbuf_addstr() calls in the way
they find the most natural, i.e. without having to choose between
_addstr() and _add(). If the compilers can do their unnecessary
thinking for us humans, we should make them do so to help us.
If somebody can prove that between these two
strbuf_add(&sb, "c", 1);
strbuf_addch(&sb, 'c');
there is a meaningful difference in overhead to encourage us to
rewrite the former to the latter, perhaps a similar trick can be
employed in the implementation of strbuf_add(), perhaps like:
static inline void strbuf_add(struct strbuf *sb, const void *data, size_t len)
{
if (len == 1) {
strbuf_addch(sb, *((char *)sb));
} else {
strbuf_grow(sb, len);
memcpy(sb->buf + sb->len, data, len);
strbuf_setlen(sb, sb->len + len);
}
}
That way, people can make their strbuf_add() and strbuf_addstr()
calls in the way they find the most natural, i.e. without having to
choose between _add() and _addch() depending on the length of the
string. This makes the code easier to maintain, as we do not have
to change the code all that much when the length of the string we
need to append to a strbuf changes from 1 to more (or the other way
around).
But I somehow doubt it is worth it.
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 71f925e456c..3ab4cc0a56b 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -143,11 +143,9 @@ static void get_ac_line(const char *inbuf, const char *what,
>
> if (split_ident_line(&ident, tmp, len)) {
> error_out:
> - /* Ugh */
> - tmp = "(unknown)";
> - strbuf_addstr(name, tmp);
> - strbuf_addstr(mail, tmp);
> - strbuf_addstr(tz, tmp);
> + strbuf_addstr(name, "(unknown)");
> + strbuf_addstr(mail, "(unknown)");
> + strbuf_addstr(tz, "(unknown)");
This is another unrelated change that has not much to do with the
theme of the change, to use addch() when the string is of length 1.
> if (opt->priv->call_depth) {
> - strbuf_addchars(dest, ' ', 2);
> - strbuf_addstr(dest, "From inner merge:");
> + strbuf_addstr(dest, " From inner merge:");
> strbuf_addchars(dest, ' ', opt->priv->call_depth * 2);
Ditto, even though this is not as horrible as the change to builtin/am.c
we saw earlier.
I'll stop here.
^ permalink raw reply
* Re: [PATCH] grep: fall back to interpreter mode if JIT fails
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 15:44 UTC (permalink / raw)
To: Mathias Krause; +Cc: Carlo Arenas, Junio C Hamano, git
In-Reply-To: <f680b274-fa85-6624-096a-7753a2671c15@grsecurity.net>
On Wed, Jan 18 2023, Mathias Krause wrote:
> On 20.12.22 22:11, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Tue, Dec 20 2022, Mathias Krause wrote:
>>
>> [De-CC-ing pcre-dev@, since this part is all git-specific]
>>
>>> Am 19.12.22 um 10:00 schrieb Ævar Arnfjörð Bjarmason:
>>>>
>>>> On Fri, Dec 16 2022, Carlo Arenas wrote:
>>>>
>>>> [CC-ing pcre-dev@ for this "future error API" discussion]
>>>>
>>>>> On Fri, Dec 16, 2022 at 3:09 PM Junio C Hamano <gitster@pobox.com> wrote:
>>>>>>
>>>>>> Mathias Krause <minipli@grsecurity.net> writes:
>>>>>>
>>>>>>> ... However, from a user's point of view a fall back to
>>>>>>> interpreter mode might still be desired in this case, as a failing
>>>>>>> 'git grep' is simply not acceptable, IMHO.
>>>>>>
>>>>>> "git grep" that silently produces a wrong result (by falling back
>>>>>> after a problem is detected) would not be acceptable, either.
>>>>>
>>>>> except that an error at this point only invalidates the use of JIT,
>>>>> so calling pcre2_jit_match() is invalid but calling pcre2_match() is not.
>>>>>
>>>>> the later is setup to be used later by the code that is added,
>>>>
>>>> I think we could stumble ahead, but if this were to happen our
>>>> assumptions about how the API works have been invalidated.
>>>
>>> Well, pcre2_jit_compile() might fail for internal reasons, e.g.
>>> pcre2jit(3) states: "[...] If a pattern is too big, a call to
>>> pcre2_jit_compile() returns PCRE2_ERROR_NOMEMORY."
>>>
>>> For example, the following fails for me:
>>> $ git grep -P "$(perl -e 'print "(.)" x 4000')" -- grep.c
>>> fatal: Couldn't JIT the PCRE2 pattern '(.)(.)(.)(.)…
>>>
>>> But explicitly disabling JIT makes it "work":
>>> $ git grep -P "(*NO_JIT)$(perl -e 'print "(.)" x 4000')" -- grep.c
>>> $
>>>
>>> It's a made up example and might even be intended behavior by git, but
>>> it also proves a point Carlo already mentioned, a failing call to
>>> pcre2_jit_compile() only invalidates the use of the JIT engine. We can
>>> still use and fall back to the interpreter.
>>
>> We should arguably do this, I hadn't bothered because I haven't been
>> able to find anything except pathological patterns where it matters, and
>> silently falling back in those cases will suck a lot more IMO.
>>
>> If you are using such a pathological pattern it's almost always a better
>> idea to adjust your crazy pattern.
>>
>> So I think in the *general* case we really should just keep this, and
>> *maybe* suggest the user try with (*NO_JIT) in the pattern.
>
> Except for the case I'm trying to address, where we simply cannot detect
> *why* the JIT is failing from the error code alone. It'll error out with
> PCRE2_ERROR_NOMEMORY for the case where the JIT fails to allocate W|X
> memory as well as for the case where the pattern is crazy.
*nod*, though I see you saw that I addressed that below.
>> But silently falling back kind of sucks, but unfortunately pcre2 doesn't
>> provide a way to say "failed because of SELinux" v.s. "failed because
>> the pattern is crazy", except that we could try to compile a known-good
>> pattern with the JIT, to disambiguate the two.
>
> Exactly, so what about something like this:
>
> If JIT is generally available, try to JIT the user supplied pattern:
> 1/ If it fails with PCRE2_ERROR_NOMEMORY, try to compile a know good
> pattern, e.g. ".":
> 1a/ If this fails with PCRE2_ERROR_NOMEMORY as well, fall back to
> interpreter, as JIT is non-functional because of SELinux / PaX.
> 1b/ If not, it's a "crazy" pattern, suggest '(*NO_JIT)'.
> 2/ If it succeeds or fails with a different error, continue as of now,
> i.e. use JIT on success or die() on error, optionally suggesting
> '(*NO_JIT)'.
>
> That should handle the case you're concerned about and only fall back to
> interpreter mode if JIT won't be functional anyway. Admitted, this would
> also allow crazy patterns, but there's simply no way to detect these
> under such constraints.
That sounds good, i.e. we could narrow the JIT falling back case to
these SELinux cases and the like, distinct from generic internal errors.
Maybe it's too much paranoia, but it should work & get rid of the
ambiguity.
>> Anyway, if this is your goal you should really lead with that, not with
>> fixing a relatively obscure SELinux edge case...
>
> It's just that I'm suffering from that "obscure SELinux edge case", just
> not under SELinux but PaX MPROTECT. I only mentioned SELinux to state,
> it's not only an issue for PaX but regular systems as well. So it's not
> a hypothetical case I like to get handled, but my work environment to
> get usable again.
Right, I didn't mean to imply it was, just that for the rest of us the
more general effect of this change outside of SELinux is of more general
interest.
>>> It would be used anyway if PCRE2 was compiled without JIT support, so I
>>> don't see any issues with falling back to interpreter mode if the JIT
>>> compilation fails -- for whatever reason.
>>
>> It's the "for whatever reason" that I take issue with. We'd be in an
>> unknown state with the API behaving differently than we expect, and
>> returning unknown codes. That's different than the *known* error codes
>> (e.g. "no memory", oven though it's meaning is apparently overloaded to
>> the point of near-uselessness).
>>
>>>> The pcre2_jit_compile() doesn't promise to return a finite set of error
>>>> codes, but:
>>>>
>>>> [...]0 for success, or a negative error code otherwise[...]
>>>>
>>>> But if new codes were added it's anyone's guess what state we'd be in,
>>>> so I think the safe thing is to BUG() out if we get as far as
>>>> pcre2_jit_compile() and don't get either PCRE2_ERROR_JIT_BADOPTION or
>>>> PCRE2_ERROR_NOMEMORY.
>>>
>>> But why BUG()? JIT is an optimization that might fail for PCRE2 internal
>>> reasons. Why should we make 'git grep' fail too in this case when we can
>>> handle it just fine by attempting to use the interpreter?
>>>
>>> If the pattern is really bogus, the interpreter will complain as well
>>> and we'll error out. But failing just because the JIT engine can't
>>> handle the pattern? Doesn't sound right to me.
>>
>> See above, we're failing because our assumptions about how to use the
>> API have broken down at that point. We usually bug out in those cases.
>>
>>>>>> Receiving BADOPTION could be a sign that there is something wrong in
>>>>>> the input, not from the end-user but from the code, in which case
>>>>>> stopping with BUG() may be a more appropriate?
>>>>>
>>>>> The way PCRE handles this kind of errors internally is to instruct pcre2_match()
>>>>> to use the interpreter.
>>>>>
>>>>> While a BUG() might be a way to ensure the code is using the right set
>>>>> of options
>>>>> I would expect that the failure will be reported by pcre2_compile
>>>>> instead, with the
>>>>> only cases left, only being interna to PCRE (ex: JIT can't yet support
>>>>> a feature the
>>>>> interpreter has)
>>>>
>>>> I agree that it's possible in general that an external library might
>>>> start returning a "benign" error code that we could recover from, so
>>>> BUG(...) would be overdoing it.
>>>
>>> And I think that's the case here: JIT is an optimization that might not
>>> be available under all circumstances, as, for example, under SELinux's
>>> 'deny_execmem' setting. So we need to have a backup plan for such
>>> systems anyway. Why not always try to use the interpreter if JIT
>>> compilation fails?
>>
>> See above, but maybe it's the least sucky thing (and definitely
>> simpler). I'm mainly checking that we're doing that we want here, and
>> that we're going into it with eyes open.
>>
>> That we're now discussing a topic entirely different from SELinux on a
>> thread where we're (according to the commit message) fixing pcre2 where
>> the JIT is "unusable on such systems" is my main concern here.
>
> Yeah, I overlooked that angle initially, but it's a valid concern.
> However, limiting the functional change of falling back to interpreter
> mode on "JIT's broken anyway" systems only should address these and get
> me a functional 'git grep' again.
*nod*
>>>> So not only would a BUG() biting us here require them to create a new
>>>> code for the state of "we have the JIT, but can't use it here" (for some
>>>> reason I can't imagine, as "PCRE2_ERROR_NOMEMORY" is already
>>>> "overloaded" to mean that).
>>>>
>>>> It would also require them to invent a new "soft" failure mode for the
>>>> JIT, i.e. not the facility added in a25b9085043, where we can use the
>>>> JIT, but it's not on after all due to a "(*NO_JIT)" in the pattern
>>>> itself.
>>>
>>> We should really treat PCRE2 JIT as an *optional* optimization that
>>> might not be available for certain cases. For these we should, IMHO,
>>> simply use the interpreter mode, instead of bugging users with a BUG() /
>>> die().
>>
>> To summarize some of the above, I think performance also matters, we
>> have cases where:
>>
>> A. We could use the non-JIT
>> B. We could use the JIT, and it's a *lot* faster
>> C. We can't use the JIT at all
>> D. We can't use the JIT because we run into its limits
>>
>> I think it's fair to die on "D" as in practice you only (I think!) run
>> into it on pathological patterns, but yes, another option would be to
>> fall back to "A".
>>
>> But thinking you're doing "B" and not wanting to implicitly fall back to
>> "A" is also a valid use-case.
>
> Agreed. My above sketched handling should do that, as in not falling
> back to interpreter mode when the JIT would be functional per se, but
> just failed on this particular pattern.
>
>> So I'm inclined to suggest that we should be less helpful with automatic
>> fallbacks, and just suggest a "try it with '(*NO_JIT)'" advice() or
>> something.
>
> Well, that's a real pain from a user's (my) point of view. Trust me, I'm
> suffering from this, otherwise I wouldn't have brought up the issue ;)
That's fair enough, falling back in the "D" case sounds good.
>> But as noted above needing to always disable an apparently "available"
>> JIT on some systems (SELinux) does throw a monkey wrench into that
>> particular suggestion :(
>
> Yep.
>
>> So I'm not sure, I'm mainly trying to encourage you to think through the
>> edge cases, and to summarize the full impact of the change in a re-roll.
>
> Yeah, I agree. The implied fallback to the interpreter, even for the
> more obscure cases should have been mentioned in the changelog, but I
> overlooked / ignored that case initially.
>
> My take from the discussion is to do a re-roll with something like this:
>
> if (p->pcre2_jit_on) {
> jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);
> if (jitret == PCRE2_ERROR_NOMEMORY && !pcre2_jit_functional()) {
> /* JIT's non-functional because of SELinux / PaX */
> p->pcre2_jit_on = 0;
> return;
> } else if (jitret) {
> die("Couldn't JIT the PCRE2 pattern '%s', got '%d'\n"
> "Try prefixing the pattern with '(*NO_JIT)'\n",
> p->pattern, jitret);
> }
> ...
> }
>
> ...with pcre2_jit_functional() being something like this:
>
> static int pcre2_jit_functional(void)
> {
> pcre2_code *code;
> size_t off;
> int err;
>
> code = pcre2_compile((PCRE2_SPTR)".", 1, 0, &err, &off, NULL);
> if (!code)
> return 0;
>
> err = pcre2_jit_compile(code, PCRE2_JIT_COMPLETE);
> pcre2_code_free(code);
>
> return err == 0;
> }
This seems sensible as pseudocode, although please don't add another
pcre2_compile() entry point (as e.g. passing NULL here will bypass the
context we carefully set up, and if we have a custom allocator...).
Instead you could just re-enter the API itself via
compile_grep_patterns(), or perhaps lower via
compile_pcre2_pattern(). Then add some flag to "struct grep_opt" to
indicate that we shouldn't die() or BUG() there, but just run a "jit
test".
This code also treats all failures of pcre2_jit_compile() the same, but
here we only want PCRE2_ERROR_NOMEMORY.
^ permalink raw reply
* Re: [BUG?] 'git rebase --update-refs --whitespace=fix' incompatible, but not enforced
From: Derrick Stolee @ 2023-01-18 15:51 UTC (permalink / raw)
To: Philippe Blain, Git mailing list, Derrick Stolee, Elijah Newren
In-Reply-To: <b322c536-5a75-bb0c-8eac-1a99d3ba3230@gmail.com>
On 1/17/2023 5:02 PM, Philippe Blain wrote:
> Hi Elijah and Stolee,
>
> First, Stolee, thanks a lot for adding '--update-refs', it is very useful (I've
> added it to my config so I don't forget to use it).
I'm glad you're using it, and thanks for the report!
> I recently learned that 'git rebase --whitespace=fix' exists, which is also
> great but since it uses the apply backend, it can't be used with --update-refs.
> I understand this, and the fact that adding '--whitespace=fix' to the merge
> backend would be a big task; this is not what this email is about.
I also use --whitespace=fix, and am disappointed that it doesn't work with
--update-refs. I guess I just haven't used it in my workflows that depend on
--update-refs.
> I think there is a bug in that the code does not enforce the fact that
> both options can't be used together. Whenever '--whitespace' or '-C' are used,
> the code defaults to the apply backend:
...
> but 'is_merge' only checks if 'opts->type == REBASE_MERGE', so the check only
> works if --merge was given explicitely, but not when none of '--merge' or '--apply'
> were given (and so the default "merge" backend is used).
>
> I would have expected the code to die telling me --update-refs and --whitespace
> are incompatible. But instead it defaulted to --apply, and (of course) did not
> update the refs in my history (which I found out later).
Yes, I agree that there should be an error message (and a die(), not just a
warning). I quickly whipped up the patch below, which should resolve your
concern.
Note that I was a bit worried about users with the config option and not
just those that specify the option over the command-line. I put in an extra
warning for those users, but I could see the community wanting different
behavior there.
Let me know what you think. If we need a new version, I'll create a new
thread for that review.
Thanks,
-Stolee
--- >8 ---
From fe310b37796b0b15554481eb1cfa3942a9200b4b Mon Sep 17 00:00:00 2001
From: Derrick Stolee <derrickstolee@github.com>
Date: Wed, 18 Jan 2023 10:38:18 -0500
Subject: [PATCH] rebase: die for both --apply and --update-refs
The --apply backend is not compatible with the --update-refs option,
which requires the interactive backend. Without any warning, users
running 'git rebase --whitespace=fix' with --update-refs (or
rebase.updateRefs=true in their config) will realize that their non-HEAD
branches did not come along for the ride.
Fix this with a die() message in the presence of both options. Since we
cannot distinguish between the --update-refs option and the config
option at this point, do an extra check to see if --update-refs was
implied by the config and present a helpful warning to use
--no-update-refs.
It is possible that users might want to be able to use options such as
--whitespace=fix with rebase.updateRefs=true in their config without
explicitly adding --no-update-refs. However, it is probably safest to
require them to explicitly opt-in to that behavior. Users with the
config option likely expect that their refs will be automatically
updated and this will help warn them that the action they are doing is
likely destructive to their branch organization.
Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
builtin/rebase.c | 21 +++++++++++++++++++++
t/t3404-rebase-interactive.sh | 12 ++++++++++++
2 files changed, 33 insertions(+)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 1481c5b6a5b..5330258c865 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1247,6 +1247,27 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
die(_("The --edit-todo action can only be used during "
"interactive rebase."));
+ /* Check for arguments that imply --apply before checking --apply itself. */
+ if (options.update_refs) {
+ const char *incompatible = NULL;
+ if (options.git_am_opts.nr)
+ incompatible = options.git_am_opts.v[0];
+ else if (options.type == REBASE_APPLY)
+ incompatible = "--apply";
+
+ if (incompatible) {
+ int from_config = 0;
+ if (!git_config_get_bool("rebase.updaterefs", &from_config) &&
+ from_config) {
+ warning(_("you have 'rebase.updateRefs' enabled in your config, "
+ "but it is incompatible with one or more options;"));
+ warning(_("run again with '--no-update-refs' to resolve the issue"));
+ }
+ die(_("options '%s' and '%s' cannot be used together"),
+ "--upate-refs", incompatible);
+ }
+ }
+
if (trace2_is_enabled()) {
if (is_merge(&options))
trace2_cmd_mode("interactive");
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 462cefd25df..8681c8a07f8 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -2123,6 +2123,18 @@ test_expect_success '--update-refs: check failed ref update' '
test_cmp expect err.trimmed
'
+test_expect_success '--apply options are incompatible with --update-refs' '
+ for opt in "--whitespace=fix" "-C1" "--apply"
+ do
+ test_must_fail git rebase "$opt" --update-refs HEAD~1 2>err &&
+ grep "options '\''--upate-refs'\'' and '\''$opt'\'' cannot be used together" err &&
+
+ test_must_fail git -c rebase.updateRefs=true rebase "$opt" HEAD~1 2>err &&
+ grep "options '\''--upate-refs'\'' and '\''$opt'\'' cannot be used together" err &&
+ grep "you have '\''rebase.updateRefs'\'' enabled" err || return 1
+ done
+'
+
# This must be the last test in this file
test_expect_success '$EDITOR and friends are unchanged' '
test_editor_unchanged
--
2.39.1.vfs.0.0
^ permalink raw reply related
* Re: [PATCH] ssh signing: better error message when key not in agent
From: Adam Szkoda @ 2023-01-18 15:28 UTC (permalink / raw)
To: phillip.wood; +Cc: Adam Szkoda via GitGitGadget, git
In-Reply-To: <8025d5c7-ab55-c533-1997-05b4c7339d61@dunelm.org.uk>
Hi Phillip,
Good point! My first thought is to try doing a stat() syscall on the
path from 'user.signingKey' to see if it exists and if not, treat it
as a public key (and pass the -U option). If that sounds reasonable,
I can update the patch.
Best
— Adam
On Wed, Jan 18, 2023 at 3:34 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 18/01/2023 11:10, Phillip Wood wrote:
> >> the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All
> >> that
> >> needs to be done is to pass an additional backward-compatible option
> >> -U to
> >> 'ssh-keygen -Y sign' call. With '-U', ssh-keygen always interprets
> >> the file
> >> as public key and expects to find the private key in the agent.
> >
> > The documentation for user.signingKey says
> >
> > If gpg.format is set to ssh this can contain the path to either your
> > private ssh key or the public key when ssh-agent is used.
> >
> > If I've understood correctly passing -U will prevent users from setting
> > this to a private key.
>
> If there is an easy way to tell if the user has given us a public key
> then we could pass "-U" in that case.
>
> Best Wishes
>
> Phillip
^ permalink raw reply
* Re: [PATCH] ssh signing: better error message when key not in agent
From: Phillip Wood @ 2023-01-18 14:34 UTC (permalink / raw)
To: Adam Szkoda via GitGitGadget, git; +Cc: Adam Szkoda
In-Reply-To: <abec912c-065d-2098-962e-41f9646dd046@dunelm.org.uk>
On 18/01/2023 11:10, Phillip Wood wrote:
>> the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All
>> that
>> needs to be done is to pass an additional backward-compatible option
>> -U to
>> 'ssh-keygen -Y sign' call. With '-U', ssh-keygen always interprets
>> the file
>> as public key and expects to find the private key in the agent.
>
> The documentation for user.signingKey says
>
> If gpg.format is set to ssh this can contain the path to either your
> private ssh key or the public key when ssh-agent is used.
>
> If I've understood correctly passing -U will prevent users from setting
> this to a private key.
If there is an easy way to tell if the user has given us a public key
then we could pass "-U" in that case.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH] grep: fall back to interpreter mode if JIT fails
From: Mathias Krause @ 2023-01-18 14:22 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Carlo Arenas, Junio C Hamano, git
In-Reply-To: <221220.86bknxwy9t.gmgdl@evledraar.gmail.com>
On 20.12.22 22:11, Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, Dec 20 2022, Mathias Krause wrote:
>
> [De-CC-ing pcre-dev@, since this part is all git-specific]
>
>> Am 19.12.22 um 10:00 schrieb Ævar Arnfjörð Bjarmason:
>>>
>>> On Fri, Dec 16 2022, Carlo Arenas wrote:
>>>
>>> [CC-ing pcre-dev@ for this "future error API" discussion]
>>>
>>>> On Fri, Dec 16, 2022 at 3:09 PM Junio C Hamano <gitster@pobox.com> wrote:
>>>>>
>>>>> Mathias Krause <minipli@grsecurity.net> writes:
>>>>>
>>>>>> ... However, from a user's point of view a fall back to
>>>>>> interpreter mode might still be desired in this case, as a failing
>>>>>> 'git grep' is simply not acceptable, IMHO.
>>>>>
>>>>> "git grep" that silently produces a wrong result (by falling back
>>>>> after a problem is detected) would not be acceptable, either.
>>>>
>>>> except that an error at this point only invalidates the use of JIT,
>>>> so calling pcre2_jit_match() is invalid but calling pcre2_match() is not.
>>>>
>>>> the later is setup to be used later by the code that is added,
>>>
>>> I think we could stumble ahead, but if this were to happen our
>>> assumptions about how the API works have been invalidated.
>>
>> Well, pcre2_jit_compile() might fail for internal reasons, e.g.
>> pcre2jit(3) states: "[...] If a pattern is too big, a call to
>> pcre2_jit_compile() returns PCRE2_ERROR_NOMEMORY."
>>
>> For example, the following fails for me:
>> $ git grep -P "$(perl -e 'print "(.)" x 4000')" -- grep.c
>> fatal: Couldn't JIT the PCRE2 pattern '(.)(.)(.)(.)…
>>
>> But explicitly disabling JIT makes it "work":
>> $ git grep -P "(*NO_JIT)$(perl -e 'print "(.)" x 4000')" -- grep.c
>> $
>>
>> It's a made up example and might even be intended behavior by git, but
>> it also proves a point Carlo already mentioned, a failing call to
>> pcre2_jit_compile() only invalidates the use of the JIT engine. We can
>> still use and fall back to the interpreter.
>
> We should arguably do this, I hadn't bothered because I haven't been
> able to find anything except pathological patterns where it matters, and
> silently falling back in those cases will suck a lot more IMO.
>
> If you are using such a pathological pattern it's almost always a better
> idea to adjust your crazy pattern.
>
> So I think in the *general* case we really should just keep this, and
> *maybe* suggest the user try with (*NO_JIT) in the pattern.
Except for the case I'm trying to address, where we simply cannot detect
*why* the JIT is failing from the error code alone. It'll error out with
PCRE2_ERROR_NOMEMORY for the case where the JIT fails to allocate W|X
memory as well as for the case where the pattern is crazy.
> But silently falling back kind of sucks, but unfortunately pcre2 doesn't
> provide a way to say "failed because of SELinux" v.s. "failed because
> the pattern is crazy", except that we could try to compile a known-good
> pattern with the JIT, to disambiguate the two.
Exactly, so what about something like this:
If JIT is generally available, try to JIT the user supplied pattern:
1/ If it fails with PCRE2_ERROR_NOMEMORY, try to compile a know good
pattern, e.g. ".":
1a/ If this fails with PCRE2_ERROR_NOMEMORY as well, fall back to
interpreter, as JIT is non-functional because of SELinux / PaX.
1b/ If not, it's a "crazy" pattern, suggest '(*NO_JIT)'.
2/ If it succeeds or fails with a different error, continue as of now,
i.e. use JIT on success or die() on error, optionally suggesting
'(*NO_JIT)'.
That should handle the case you're concerned about and only fall back to
interpreter mode if JIT won't be functional anyway. Admitted, this would
also allow crazy patterns, but there's simply no way to detect these
under such constraints.
> Anyway, if this is your goal you should really lead with that, not with
> fixing a relatively obscure SELinux edge case...
It's just that I'm suffering from that "obscure SELinux edge case", just
not under SELinux but PaX MPROTECT. I only mentioned SELinux to state,
it's not only an issue for PaX but regular systems as well. So it's not
a hypothetical case I like to get handled, but my work environment to
get usable again.
>> It would be used anyway if PCRE2 was compiled without JIT support, so I
>> don't see any issues with falling back to interpreter mode if the JIT
>> compilation fails -- for whatever reason.
>
> It's the "for whatever reason" that I take issue with. We'd be in an
> unknown state with the API behaving differently than we expect, and
> returning unknown codes. That's different than the *known* error codes
> (e.g. "no memory", oven though it's meaning is apparently overloaded to
> the point of near-uselessness).
>
>>> The pcre2_jit_compile() doesn't promise to return a finite set of error
>>> codes, but:
>>>
>>> [...]0 for success, or a negative error code otherwise[...]
>>>
>>> But if new codes were added it's anyone's guess what state we'd be in,
>>> so I think the safe thing is to BUG() out if we get as far as
>>> pcre2_jit_compile() and don't get either PCRE2_ERROR_JIT_BADOPTION or
>>> PCRE2_ERROR_NOMEMORY.
>>
>> But why BUG()? JIT is an optimization that might fail for PCRE2 internal
>> reasons. Why should we make 'git grep' fail too in this case when we can
>> handle it just fine by attempting to use the interpreter?
>>
>> If the pattern is really bogus, the interpreter will complain as well
>> and we'll error out. But failing just because the JIT engine can't
>> handle the pattern? Doesn't sound right to me.
>
> See above, we're failing because our assumptions about how to use the
> API have broken down at that point. We usually bug out in those cases.
>
>>>>> Receiving BADOPTION could be a sign that there is something wrong in
>>>>> the input, not from the end-user but from the code, in which case
>>>>> stopping with BUG() may be a more appropriate?
>>>>
>>>> The way PCRE handles this kind of errors internally is to instruct pcre2_match()
>>>> to use the interpreter.
>>>>
>>>> While a BUG() might be a way to ensure the code is using the right set
>>>> of options
>>>> I would expect that the failure will be reported by pcre2_compile
>>>> instead, with the
>>>> only cases left, only being interna to PCRE (ex: JIT can't yet support
>>>> a feature the
>>>> interpreter has)
>>>
>>> I agree that it's possible in general that an external library might
>>> start returning a "benign" error code that we could recover from, so
>>> BUG(...) would be overdoing it.
>>
>> And I think that's the case here: JIT is an optimization that might not
>> be available under all circumstances, as, for example, under SELinux's
>> 'deny_execmem' setting. So we need to have a backup plan for such
>> systems anyway. Why not always try to use the interpreter if JIT
>> compilation fails?
>
> See above, but maybe it's the least sucky thing (and definitely
> simpler). I'm mainly checking that we're doing that we want here, and
> that we're going into it with eyes open.
>
> That we're now discussing a topic entirely different from SELinux on a
> thread where we're (according to the commit message) fixing pcre2 where
> the JIT is "unusable on such systems" is my main concern here.
Yeah, I overlooked that angle initially, but it's a valid concern.
However, limiting the functional change of falling back to interpreter
mode on "JIT's broken anyway" systems only should address these and get
me a functional 'git grep' again.
>>> So not only would a BUG() biting us here require them to create a new
>>> code for the state of "we have the JIT, but can't use it here" (for some
>>> reason I can't imagine, as "PCRE2_ERROR_NOMEMORY" is already
>>> "overloaded" to mean that).
>>>
>>> It would also require them to invent a new "soft" failure mode for the
>>> JIT, i.e. not the facility added in a25b9085043, where we can use the
>>> JIT, but it's not on after all due to a "(*NO_JIT)" in the pattern
>>> itself.
>>
>> We should really treat PCRE2 JIT as an *optional* optimization that
>> might not be available for certain cases. For these we should, IMHO,
>> simply use the interpreter mode, instead of bugging users with a BUG() /
>> die().
>
> To summarize some of the above, I think performance also matters, we
> have cases where:
>
> A. We could use the non-JIT
> B. We could use the JIT, and it's a *lot* faster
> C. We can't use the JIT at all
> D. We can't use the JIT because we run into its limits
>
> I think it's fair to die on "D" as in practice you only (I think!) run
> into it on pathological patterns, but yes, another option would be to
> fall back to "A".
>
> But thinking you're doing "B" and not wanting to implicitly fall back to
> "A" is also a valid use-case.
Agreed. My above sketched handling should do that, as in not falling
back to interpreter mode when the JIT would be functional per se, but
just failed on this particular pattern.
> So I'm inclined to suggest that we should be less helpful with automatic
> fallbacks, and just suggest a "try it with '(*NO_JIT)'" advice() or
> something.
Well, that's a real pain from a user's (my) point of view. Trust me, I'm
suffering from this, otherwise I wouldn't have brought up the issue ;)
> But as noted above needing to always disable an apparently "available"
> JIT on some systems (SELinux) does throw a monkey wrench into that
> particular suggestion :(
Yep.
> So I'm not sure, I'm mainly trying to encourage you to think through the
> edge cases, and to summarize the full impact of the change in a re-roll.
Yeah, I agree. The implied fallback to the interpreter, even for the
more obscure cases should have been mentioned in the changelog, but I
overlooked / ignored that case initially.
My take from the discussion is to do a re-roll with something like this:
if (p->pcre2_jit_on) {
jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);
if (jitret == PCRE2_ERROR_NOMEMORY && !pcre2_jit_functional()) {
/* JIT's non-functional because of SELinux / PaX */
p->pcre2_jit_on = 0;
return;
} else if (jitret) {
die("Couldn't JIT the PCRE2 pattern '%s', got '%d'\n"
"Try prefixing the pattern with '(*NO_JIT)'\n",
p->pattern, jitret);
}
...
}
...with pcre2_jit_functional() being something like this:
static int pcre2_jit_functional(void)
{
pcre2_code *code;
size_t off;
int err;
code = pcre2_compile((PCRE2_SPTR)".", 1, 0, &err, &off, NULL);
if (!code)
return 0;
err = pcre2_jit_compile(code, PCRE2_JIT_COMPLETE);
pcre2_code_free(code);
return err == 0;
}
Thanks,
Mathias
^ permalink raw reply
* Re: test_pause giving '__git_ps1: not found' warning
From: Philip Oakley @ 2023-01-18 13:50 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Philippe Blain, Elijah Newren
In-Reply-To: <Y8WLmmjL2HH2szGs@coredump.intra.peff.net>
On 16/01/2023 17:38, Jeff King wrote:
> On Sat, Jan 14, 2023 at 05:49:15PM +0000, Philip Oakley wrote:
>
>> On 14/01/2023 14:54, Philip Oakley wrote:
>>> I was trying to refine a test_expect_failure test [1] and tried
>>> inserting a `test_pause &&` test line [2].
>>>
>>> I then found, when it paused, I was repeatedly given the warning line
>>> /bin/sh: 1: __git_ps1: not found
>>> in the terminal until I expected the test shell.
>>>
>>> my PS1 is working normally in the terminal, but not here. Is this
>>> expected, or do I need to set up anything else?
>>>
>>> Normally I'm on Git for Windows, but this was on my old laptop (Acer
>>> 7741 i5 4GB ram..) converted to Ubuntu 20.04, which I use when away.
> On Ubuntu, your /bin/sh is likely to be dash. And unless you've set
> TEST_SHELL_PATH, that's what test_pause will use.
Thanks. Some of these little tidbits passed me by.
I wasactually trying to understand why $(printf "\145\314\201") wasn't
working as expected (or the hex equivalent) depending on which machine
(Ubuntu / Windows) I was on (when running in a test as part of
t/t4205-log-pretty-formats.sh). I.e. sometimes the results looked like
the unicode was being recognised, and others as if part were just ascii
characters, ignoring the octal/hex escaping)
>
>> My local .bashrc has
>>
>> . /home/philip/git-completion.bash
>> . ~/git-prompt.sh
>> export GIT_PS1_SHOWDIRTYSTATE=1
>> export PS1='\w$(__git_ps1 " (%s)")\$ '
> It's unusual to export PS1. Only the shell needs it.
It's the default setting described in the git-scm book's Appendix A "Git
in Bash" [1].
Not sure if extra clarification is needed, or where that advice
originated from. It (export'ing) is not currently part of contrib's
git-prompt.sh comments [2].
> But by exporting
> it, the child "dash" will see it, but of course doesn't have the
> __git_ps1() function defined. And thus the complaint.
>
> If you stop exporting it, then you'd see dash's regular prompt. Which
> fixes the "whoops, __git_ps1() is not defined" problem, but probably is
> not quite what you want, if you really wanted a nice prompt.
>
>> Not sure why I have a relative and an absolute path but... , so I'll try
>> updating the git-prompt.sh to an absolute path, and if that works, maybe
>> think about adding an extra comment to the `test-lib-functions.sh` to note
>> the change of HOME and potential '__git_ps1' problem
> It looks like test_pause() has various options for retaining shell,
> term, home, etc, and includes documentation. I haven't played with them
> myself, though.
> Usually I just use "-i" to stop the test, and then
> investigate the result in one of my regular shells (and in the rare case
> of "oops, it succeeds when I expect it to fail,
> I insert "false &&" as
> appropriate).
That's a neat alternative.
>
> -Peff
--
Philip
[1]
https://git-scm.com/book/uz/v2/Appendix-A%3A-Git-in-Other-Environments-Git-in-Bash
[2] https://github.com/git/git/blob/master/contrib/completion/git-prompt.sh
^ permalink raw reply
* [PATCH v5 18/19] receive-pack: free() the "ref_name" in "struct command"
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-avarab@gmail.com>
Fix a memory leak that's been with us since this code was introduced
in 575f497456e (Add first cut at "git-receive-pack", 2005-06-29), see
eb1af2df0b1 (git-receive-pack: start parsing ref update commands,
2005-06-29) for the later change that refactored the code to add the
"ref_name" member.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/receive-pack.c | 10 ++++++++++
t/t5405-send-pack-rewind.sh | 1 +
t/t5406-remote-rejects.sh | 1 +
t/t5507-remote-environment.sh | 2 ++
t/t5522-pull-symlink.sh | 1 +
t/t5527-fetch-odd-refs.sh | 1 +
t/t5560-http-backend-noserver.sh | 1 +
t/t5561-http-backend.sh | 1 +
t/t5562-http-backend-content-length.sh | 2 ++
t/t5705-session-id-in-capabilities.sh | 1 +
10 files changed, 21 insertions(+)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index a90af303630..451bad776c6 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2032,6 +2032,15 @@ static struct command **queue_command(struct command **tail,
return &cmd->next;
}
+static void free_commands(struct command *commands)
+{
+ while (commands) {
+ struct command *next = commands->next;
+ free(commands);
+ commands = next;
+ }
+}
+
static void queue_commands_from_cert(struct command **tail,
struct strbuf *push_cert)
{
@@ -2569,6 +2578,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
run_receive_hook(commands, "post-receive", 1,
&push_options);
run_update_post_hook(commands);
+ free_commands(commands);
string_list_clear(&push_options, 0);
if (auto_gc) {
struct child_process proc = CHILD_PROCESS_INIT;
diff --git a/t/t5405-send-pack-rewind.sh b/t/t5405-send-pack-rewind.sh
index 11f03239a06..1686ac13aa6 100755
--- a/t/t5405-send-pack-rewind.sh
+++ b/t/t5405-send-pack-rewind.sh
@@ -5,6 +5,7 @@ test_description='forced push to replace commit we do not have'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t5406-remote-rejects.sh b/t/t5406-remote-rejects.sh
index dcbeb420827..d6a99466338 100755
--- a/t/t5406-remote-rejects.sh
+++ b/t/t5406-remote-rejects.sh
@@ -2,6 +2,7 @@
test_description='remote push rejects are reported by client'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t5507-remote-environment.sh b/t/t5507-remote-environment.sh
index e6149295b18..c6a6957c500 100755
--- a/t/t5507-remote-environment.sh
+++ b/t/t5507-remote-environment.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='check environment showed to remote side of transports'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'set up "remote" push situation' '
diff --git a/t/t5522-pull-symlink.sh b/t/t5522-pull-symlink.sh
index bcff460d0a2..394bc60cb8e 100755
--- a/t/t5522-pull-symlink.sh
+++ b/t/t5522-pull-symlink.sh
@@ -2,6 +2,7 @@
test_description='pulling from symlinked subdir'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# The scenario we are building:
diff --git a/t/t5527-fetch-odd-refs.sh b/t/t5527-fetch-odd-refs.sh
index e2770e4541f..98ece27c6a0 100755
--- a/t/t5527-fetch-odd-refs.sh
+++ b/t/t5527-fetch-odd-refs.sh
@@ -4,6 +4,7 @@ test_description='test fetching of oddly-named refs'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# afterwards we will have:
diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index d30cf4f5b83..f75068de648 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh
@@ -4,6 +4,7 @@ test_description='test git-http-backend-noserver'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
HTTPD_DOCUMENT_ROOT_PATH="$TRASH_DIRECTORY"
diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
index 9c57d843152..e1d3b8caed0 100755
--- a/t/t5561-http-backend.sh
+++ b/t/t5561-http-backend.sh
@@ -4,6 +4,7 @@ test_description='test git-http-backend'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-httpd.sh
diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
index b68ec22d3fd..7ee9858a78b 100755
--- a/t/t5562-http-backend-content-length.sh
+++ b/t/t5562-http-backend-content-length.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='test git-http-backend respects CONTENT_LENGTH'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_lazy_prereq GZIP 'gzip --version'
diff --git a/t/t5705-session-id-in-capabilities.sh b/t/t5705-session-id-in-capabilities.sh
index ed38c76c290..b8a722ec27e 100755
--- a/t/t5705-session-id-in-capabilities.sh
+++ b/t/t5705-session-id-in-capabilities.sh
@@ -2,6 +2,7 @@
test_description='session ID in capabilities'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
REPO="$(pwd)/repo"
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v5 14/19] builtin/merge.c: free "&buf" on "Your local changes..." error
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-avarab@gmail.com>
Plug a memory leak introduced in [1], since that change didn't follow
the "goto done" pattern introduced in [2] we'd leak the "&buf" memory.
1. e4cdfe84a0d (merge: abort if index does not match HEAD for trivial
merges, 2022-07-23)
2. d5a35c114ab (Copy resolve_ref() return value for longer use,
2011-11-13)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/merge.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 91dd5435c59..2b13124c497 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1618,7 +1618,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
error(_("Your local changes to the following files would be overwritten by merge:\n %s"),
sb.buf);
strbuf_release(&sb);
- return 2;
+ ret = 2;
+ goto done;
}
/* See if it is really trivial. */
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v5 16/19] grep.c: refactor free_grep_patterns()
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-avarab@gmail.com>
Refactor the free_grep_patterns() function to split out the freeing of
the "struct grep_pat" it contains, right now we're only freeing the
"pattern_list", but we should be freeing another member of the same
type, which we'll do in the subsequent commit.
Let's also replace the "return" if we don't have an
"opt->pattern_expression" with a conditional call of
free_pattern_expr().
Before db84376f981 (grep.c: remove "extended" in favor of
"pattern_expression", fix segfault, 2022-10-11) the pattern here was:
if (!x)
return;
free(y);
But after the cleanup in db84376f981 (which was a narrow segfault fix,
and thus avoided refactoring this) we ended up with:
if (!x)
return;
free(x);
Let's instead do:
if (x)
free(x);
This doesn't matter for the subsequent change, but as we're
refactoring this function let's make it easier to reason about, and to
extend in the future, i.e. if we start to free free() members that
come after "pattern_expression" in the "struct grep_opt".
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
grep.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/grep.c b/grep.c
index 06eed694936..a4450df4559 100644
--- a/grep.c
+++ b/grep.c
@@ -769,11 +769,11 @@ static void free_pattern_expr(struct grep_expr *x)
free(x);
}
-void free_grep_patterns(struct grep_opt *opt)
+static void free_grep_pat(struct grep_pat *pattern)
{
struct grep_pat *p, *n;
- for (p = opt->pattern_list; p; p = n) {
+ for (p = pattern; p; p = n) {
n = p->next;
switch (p->token) {
case GREP_PATTERN: /* atom */
@@ -790,10 +790,14 @@ void free_grep_patterns(struct grep_opt *opt)
}
free(p);
}
+}
- if (!opt->pattern_expression)
- return;
- free_pattern_expr(opt->pattern_expression);
+void free_grep_patterns(struct grep_opt *opt)
+{
+ free_grep_pat(opt->pattern_list);
+
+ if (opt->pattern_expression)
+ free_pattern_expr(opt->pattern_expression);
}
static const char *end_of_line(const char *cp, unsigned long *left)
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v5 13/19] builtin/merge.c: use fixed strings, not "strbuf", fix leak
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-avarab@gmail.com>
Follow-up 465028e0e25 (merge: add missing strbuf_release(),
2021-10-07) and address the "msg" memory leak in this block. We could
free "&msg" before the "goto done" here, but even better is to avoid
allocating it in the first place.
By repeating the "Fast-forward" string here we can avoid using a
"struct strbuf" altogether.
Suggested-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/merge.c | 11 ++++-------
t/t6439-merge-co-error-msgs.sh | 1 +
2 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 0f093f2a4f2..91dd5435c59 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1560,7 +1560,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
!common->next &&
oideq(&common->item->object.oid, &head_commit->object.oid)) {
/* Again the most common case of merging one remote. */
- struct strbuf msg = STRBUF_INIT;
+ const char *msg = have_message ?
+ "Fast-forward (no commit created; -m option ignored)" :
+ "Fast-forward";
struct commit *commit;
if (verbosity >= 0) {
@@ -1570,10 +1572,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
find_unique_abbrev(&remoteheads->item->object.oid,
DEFAULT_ABBREV));
}
- strbuf_addstr(&msg, "Fast-forward");
- if (have_message)
- strbuf_addstr(&msg,
- " (no commit created; -m option ignored)");
commit = remoteheads->item;
if (!commit) {
ret = 1;
@@ -1592,9 +1590,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
goto done;
}
- finish(head_commit, remoteheads, &commit->object.oid, msg.buf);
+ finish(head_commit, remoteheads, &commit->object.oid, msg);
remove_merge_branch_state(the_repository);
- strbuf_release(&msg);
goto done;
} else if (!remoteheads->next && common->next)
;
diff --git a/t/t6439-merge-co-error-msgs.sh b/t/t6439-merge-co-error-msgs.sh
index 52cf0c87690..0cbec57cdab 100755
--- a/t/t6439-merge-co-error-msgs.sh
+++ b/t/t6439-merge-co-error-msgs.sh
@@ -5,6 +5,7 @@ test_description='unpack-trees error messages'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v5 19/19] push: free_refs() the "local_refs" in set_refspecs()
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-avarab@gmail.com>
Fix a memory leak that's been with us since this code was added in
ca02465b413 (push: use remote.$name.push as a refmap, 2013-12-03).
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/push.c | 1 +
t/t1416-ref-transaction-hooks.sh | 1 +
t/t2402-worktree-list.sh | 1 +
t/t5504-fetch-receive-strict.sh | 1 +
t/t5523-push-upstream.sh | 1 +
t/t5529-push-errors.sh | 2 ++
t/t5546-receive-limits.sh | 2 ++
t/t5547-push-quarantine.sh | 2 ++
t/t5606-clone-options.sh | 1 +
t/t5810-proto-disable-local.sh | 2 ++
t/t5813-proto-disable-ssh.sh | 2 ++
t/t7409-submodule-detached-work-tree.sh | 1 +
t/t7416-submodule-dash-url.sh | 2 ++
t/t7450-bad-git-dotfiles.sh | 2 ++
14 files changed, 21 insertions(+)
diff --git a/builtin/push.c b/builtin/push.c
index 60ac8017e52..f48e4c6a856 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -129,6 +129,7 @@ static void set_refspecs(const char **refs, int nr, const char *repo)
} else
refspec_append(&rs, ref);
}
+ free_refs(local_refs);
}
static int push_url_of_remote(struct remote *remote, const char ***url_p)
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index 27731722a5b..b32ca798f9f 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -5,6 +5,7 @@ test_description='reference transaction hooks'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
index 79e0fce2d90..9ad9be0c208 100755
--- a/t/t2402-worktree-list.sh
+++ b/t/t2402-worktree-list.sh
@@ -5,6 +5,7 @@ test_description='test git worktree list'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index ac4099ca893..14e8af1f3b7 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -4,6 +4,7 @@ test_description='fetch/receive strict mode'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup and inject "corrupt or missing" object' '
diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
index fdb42920564..c9acc076353 100755
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -4,6 +4,7 @@ test_description='push with --set-upstream'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-terminal.sh
diff --git a/t/t5529-push-errors.sh b/t/t5529-push-errors.sh
index ce85fd30ad1..0247137cb36 100755
--- a/t/t5529-push-errors.sh
+++ b/t/t5529-push-errors.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='detect some push errors early (before contacting remote)'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup commits' '
diff --git a/t/t5546-receive-limits.sh b/t/t5546-receive-limits.sh
index 0b0e987fdb7..eed3c9d81ab 100755
--- a/t/t5546-receive-limits.sh
+++ b/t/t5546-receive-limits.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='check receive input limits'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# Let's run tests with different unpack limits: 1 and 10000
diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
index 1876fb34e51..9f899b8c7d7 100755
--- a/t/t5547-push-quarantine.sh
+++ b/t/t5547-push-quarantine.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='check quarantine of objects during push'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'create picky dest repo' '
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index cf221e92c4d..27f9f776389 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -4,6 +4,7 @@ test_description='basic clone options'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t5810-proto-disable-local.sh b/t/t5810-proto-disable-local.sh
index c1ef99b85c2..862610256fb 100755
--- a/t/t5810-proto-disable-local.sh
+++ b/t/t5810-proto-disable-local.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='test disabling of local paths in clone/fetch'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY/lib-proto-disable.sh"
diff --git a/t/t5813-proto-disable-ssh.sh b/t/t5813-proto-disable-ssh.sh
index 3f084ee3065..2e975dc70ec 100755
--- a/t/t5813-proto-disable-ssh.sh
+++ b/t/t5813-proto-disable-ssh.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='test disabling of git-over-ssh in clone/fetch'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY/lib-proto-disable.sh"
diff --git a/t/t7409-submodule-detached-work-tree.sh b/t/t7409-submodule-detached-work-tree.sh
index 374ed481e9c..574a6fc526e 100755
--- a/t/t7409-submodule-detached-work-tree.sh
+++ b/t/t7409-submodule-detached-work-tree.sh
@@ -13,6 +13,7 @@ TEST_NO_CREATE_REPO=1
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t7416-submodule-dash-url.sh b/t/t7416-submodule-dash-url.sh
index 3ebd9859814..7cf72b9a076 100755
--- a/t/t7416-submodule-dash-url.sh
+++ b/t/t7416-submodule-dash-url.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='check handling of disallowed .gitmodule urls'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh
index ba1f569bcbb..0d0c3f2c683 100755
--- a/t/t7450-bad-git-dotfiles.sh
+++ b/t/t7450-bad-git-dotfiles.sh
@@ -12,6 +12,8 @@ Such as:
- symlinked .gitmodules, etc
'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-pack.sh
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v5 15/19] object-file.c: release the "tag" in check_tag()
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-avarab@gmail.com>
Fix a memory leak that's been with us ever since c879daa2372 (Make
hash-object more robust against malformed objects, 2011-02-05). With
"HASH_FORMAT_CHECK" (used by "hash-object" and "replace") we'll parse
tags into a throwaway variable on the stack, but weren't freeing the
"item->tag" we might malloc() when doing so.
The clearing that release_tag_memory() does for us is redundant here,
but let's use it as-is anyway. It only has one other existing caller,
which does need the tag to be cleared.
Mark the tests that now pass in their entirety as passing under
"SANITIZE=leak", which means we'll test them as part of the
"linux-leaks" CI job.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
object-file.c | 1 +
t/t3800-mktag.sh | 1 +
t/t5302-pack-index.sh | 2 ++
3 files changed, 4 insertions(+)
diff --git a/object-file.c b/object-file.c
index 80a0cd3b351..b554266aff4 100644
--- a/object-file.c
+++ b/object-file.c
@@ -2324,6 +2324,7 @@ static void check_tag(const void *buf, size_t size)
memset(&t, 0, sizeof(t));
if (parse_tag_buffer(the_repository, &t, buf, size))
die(_("corrupt tag"));
+ release_tag_memory(&t);
}
static int index_mem(struct index_state *istate,
diff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh
index e3cf0ffbe59..d3e428ff46e 100755
--- a/t/t3800-mktag.sh
+++ b/t/t3800-mktag.sh
@@ -4,6 +4,7 @@
test_description='git mktag: tag object verify test'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
###########################################################
diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
index b0095ab41d3..54b11f81c63 100755
--- a/t/t5302-pack-index.sh
+++ b/t/t5302-pack-index.sh
@@ -4,6 +4,8 @@
#
test_description='pack index with 64-bit offsets and object CRC'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v5 17/19] grep API: plug memory leaks by freeing "header_list"
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-avarab@gmail.com>
When the "header_list" struct member was added in [1] it wasn't made
to free the list using loop added for the adjacent "pattern_list"
member, see [2] for when we started freeing it.
This makes e.g. this command leak-free when run on git.git:
./git -P log -1 --color=always --author=A origin/master
1. 80235ba79ef ("log --author=me --grep=it" should find intersection,
not union, 2010-01-17)
2. b48fb5b6a95 (grep: free expressions and patterns when done.,
2006-09-27)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
grep.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/grep.c b/grep.c
index a4450df4559..c908535e0d8 100644
--- a/grep.c
+++ b/grep.c
@@ -795,6 +795,7 @@ static void free_grep_pat(struct grep_pat *pattern)
void free_grep_patterns(struct grep_opt *opt)
{
free_grep_pat(opt->pattern_list);
+ free_grep_pat(opt->header_list);
if (opt->pattern_expression)
free_pattern_expr(opt->pattern_expression);
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v5 09/19] http-backend.c: fix "dir" and "cmd_arg" leaks in cmd_main()
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-avarab@gmail.com>
Free the "dir" variable after we're done with it. Before
917adc03608 (http-backend: add GIT_PROJECT_ROOT environment var,
2009-10-30) there was no leak here, as we'd get it via getenv(), but
since 917adc03608 we've xstrdup()'d it (or the equivalent), so we need
to free() it.
We also need to free the "cmd_arg" variable, which has been leaked
ever since it was added in 2f4038ab337 (Git-aware CGI to provide dumb
HTTP transport, 2009-10-30).
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
http-backend.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/http-backend.c b/http-backend.c
index 6eb3b2fe51c..67819d931ce 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -786,6 +786,7 @@ int cmd_main(int argc, const char **argv)
if (!getenv("GIT_HTTP_EXPORT_ALL") &&
access("git-daemon-export-ok", F_OK) )
not_found(&hdr, "Repository not exported: '%s'", dir);
+ free(dir);
http_config();
max_request_buffer = git_env_ulong("GIT_HTTP_MAX_REQUEST_BUFFER",
@@ -795,5 +796,6 @@ int cmd_main(int argc, const char **argv)
setenv(GIT_PROTOCOL_ENVIRONMENT, proto_header, 0);
cmd->imp(&hdr, cmd_arg);
+ free(cmd_arg);
return 0;
}
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v5 12/19] show-branch: free() allocated "head" before return
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-avarab@gmail.com>
Stop leaking the "head" variable, which we've been leaking since it
was originally added in [1], and in its current form since [2]
1. ed378ec7e85 (Make ref resolution saner, 2006-09-11)
2. d9e557a320b (show-branch: store resolved head in heap buffer,
2017-02-14).
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/show-branch.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index c013abaf942..358ac3e519a 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -956,5 +956,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
if (shown_merge_point && --extra < 0)
break;
}
+ free(head);
return 0;
}
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v5 11/19] commit-graph: fix a parse_options_concat() leak
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-avarab@gmail.com>
When the parse_options_concat() was added to this file in
84e4484f128 (commit-graph: use parse_options_concat(), 2021-08-23) we
wouldn't free() it if we returned early in these cases.
Since "result" is 0 by default we can "goto cleanup" in both cases,
and only need to set "result" if write_commit_graph_reachable() fails.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/commit-graph.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 0102ac8540e..93704f95a9d 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -269,8 +269,8 @@ static int graph_write(int argc, const char **argv, const char *prefix)
if (opts.reachable) {
if (write_commit_graph_reachable(odb, flags, &write_opts))
- return 1;
- return 0;
+ result = 1;
+ goto cleanup;
}
if (opts.stdin_packs) {
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v5 08/19] worktree: fix a trivial leak in prune_worktrees()
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-avarab@gmail.com>
We were leaking both the "struct strbuf" in prune_worktrees(), as well
as the "path" we got from should_prune_worktree(). Since these were
the only two uses of the "struct string_list" let's change it to a
"DUP" and push these to it with "string_list_append_nodup()".
For the string_list_append_nodup() we could also string_list_append()
the main_path.buf, and then strbuf_release(&main_path) right away. But
doing it this way avoids an allocation, as we already have the "struct
strbuf" prepared for appending to "kept".
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/worktree.c | 6 +++---
t/t2401-worktree-prune.sh | 1 +
t/t2406-worktree-repair.sh | 1 +
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 591d659faea..865ce9be22b 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -173,7 +173,7 @@ static void prune_worktrees(void)
{
struct strbuf reason = STRBUF_INIT;
struct strbuf main_path = STRBUF_INIT;
- struct string_list kept = STRING_LIST_INIT_NODUP;
+ struct string_list kept = STRING_LIST_INIT_DUP;
DIR *dir = opendir(git_path("worktrees"));
struct dirent *d;
if (!dir)
@@ -184,14 +184,14 @@ static void prune_worktrees(void)
if (should_prune_worktree(d->d_name, &reason, &path, expire))
prune_worktree(d->d_name, reason.buf);
else if (path)
- string_list_append(&kept, path)->util = xstrdup(d->d_name);
+ string_list_append_nodup(&kept, path)->util = xstrdup(d->d_name);
}
closedir(dir);
strbuf_add_absolute_path(&main_path, get_git_common_dir());
/* massage main worktree absolute path to match 'gitdir' content */
strbuf_strip_suffix(&main_path, "/.");
- string_list_append(&kept, strbuf_detach(&main_path, NULL));
+ string_list_append_nodup(&kept, strbuf_detach(&main_path, NULL));
prune_dups(&kept);
string_list_clear(&kept, 1);
diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh
index 3d28c7f06b2..568a47ec426 100755
--- a/t/t2401-worktree-prune.sh
+++ b/t/t2401-worktree-prune.sh
@@ -5,6 +5,7 @@ test_description='prune $GIT_DIR/worktrees'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success initialize '
diff --git a/t/t2406-worktree-repair.sh b/t/t2406-worktree-repair.sh
index 5c44453e1c1..8970780efcc 100755
--- a/t/t2406-worktree-repair.sh
+++ b/t/t2406-worktree-repair.sh
@@ -2,6 +2,7 @@
test_description='test git worktree repair'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v5 10/19] http-backend.c: fix cmd_main() memory leak, refactor reg{exec,free}()
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-avarab@gmail.com>
Fix a memory leak that's been with us ever since
2f4038ab337 (Git-aware CGI to provide dumb HTTP transport,
2009-10-30). In this case we're not calling regerror() after a failed
regexec(), and don't otherwise use "re" afterwards.
We can therefore simplify this code by calling regfree() right after
the regexec(). An alternative fix would be to add a regfree() to both
the "return" and "break" path in this for-loop.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
http-backend.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/http-backend.c b/http-backend.c
index 67819d931ce..8ab58e55f85 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -759,10 +759,14 @@ int cmd_main(int argc, const char **argv)
struct service_cmd *c = &services[i];
regex_t re;
regmatch_t out[1];
+ int ret;
if (regcomp(&re, c->pattern, REG_EXTENDED))
die("Bogus regex in service table: %s", c->pattern);
- if (!regexec(&re, dir, 1, out, 0)) {
+ ret = regexec(&re, dir, 1, out, 0);
+ regfree(&re);
+
+ if (!ret) {
size_t n;
if (strcmp(method, c->method))
@@ -774,7 +778,6 @@ int cmd_main(int argc, const char **argv)
dir[out[0].rm_so] = 0;
break;
}
- regfree(&re);
}
if (!cmd)
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v5 05/19] various: add missing clear_pathspec(), fix leaks
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-avarab@gmail.com>
Fix memory leaks resulting from a missing clear_pathspec().
- archive.c: Plug a leak in the "struct archiver_args", and
clear_pathspec() the "pathspec" member that the "parse_pathspec_arg()"
call in this function populates.
- builtin/clean.c: Fix a memory leak that's been with us since
893d839970c (clean: convert to use parse_pathspec, 2013-07-14).
- builtin/reset.c: Add clear_pathspec() calls to cmd_reset(),
including to the codepaths where we'd return early.
- builtin/stash.c: Call clear_pathspec() on the pathspec initialized
in push_stash().
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
archive.c | 1 +
builtin/clean.c | 1 +
builtin/reset.c | 11 ++++++++---
builtin/stash.c | 7 +++++--
t/t5001-archive-attr.sh | 1 +
t/t5004-archive-corner-cases.sh | 2 ++
t/t7105-reset-patch.sh | 2 ++
t/t7106-reset-unborn-branch.sh | 2 ++
t/t7107-reset-pathspec-file.sh | 1 +
t/t7301-clean-interactive.sh | 1 +
10 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/archive.c b/archive.c
index 941495f5d78..a2d813e50db 100644
--- a/archive.c
+++ b/archive.c
@@ -710,6 +710,7 @@ int write_archive(int argc, const char **argv, const char *prefix,
string_list_clear_func(&args.extra_files, extra_file_info_clear);
free(args.refname);
+ clear_pathspec(&args.pathspec);
return rc;
}
diff --git a/builtin/clean.c b/builtin/clean.c
index b2701a28158..b15eab328b7 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -1092,5 +1092,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
strbuf_release(&buf);
string_list_clear(&del_list, 0);
string_list_clear(&exclude_list, 0);
+ clear_pathspec(&pathspec);
return (errors != 0);
}
diff --git a/builtin/reset.c b/builtin/reset.c
index fea20a9ba0b..e9c10618cd3 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -390,7 +390,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
if (reset_type != NONE)
die(_("options '%s' and '%s' cannot be used together"), "--patch", "--{hard,mixed,soft}");
trace2_cmd_mode("patch-interactive");
- return run_add_interactive(rev, "--patch=reset", &pathspec);
+ update_ref_status = run_add_interactive(rev, "--patch=reset", &pathspec);
+ goto cleanup;
}
/* git reset tree [--] paths... can be used to
@@ -439,8 +440,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
LOCK_DIE_ON_ERROR);
if (reset_type == MIXED) {
int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;
- if (read_from_tree(&pathspec, &oid, intent_to_add))
- return 1;
+ if (read_from_tree(&pathspec, &oid, intent_to_add)) {
+ update_ref_status = 1;
+ goto cleanup;
+ }
the_index.updated_skipworktree = 1;
if (!no_refresh && get_git_work_tree()) {
uint64_t t_begin, t_delta_in_ms;
@@ -488,5 +491,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
discard_index(&the_index);
+cleanup:
+ clear_pathspec(&pathspec);
return update_ref_status;
}
diff --git a/builtin/stash.c b/builtin/stash.c
index bb0fd861434..45bffdf54bb 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1727,6 +1727,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),
OPT_END()
};
+ int ret;
if (argc) {
force_assume = !strcmp(argv[0], "-p");
@@ -1766,8 +1767,10 @@ static int push_stash(int argc, const char **argv, const char *prefix,
die(_("the option '%s' requires '%s'"), "--pathspec-file-nul", "--pathspec-from-file");
}
- return do_push_stash(&ps, stash_msg, quiet, keep_index, patch_mode,
- include_untracked, only_staged);
+ ret = do_push_stash(&ps, stash_msg, quiet, keep_index, patch_mode,
+ include_untracked, only_staged);
+ clear_pathspec(&ps);
+ return ret;
}
static int push_stash_unassumed(int argc, const char **argv, const char *prefix)
diff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh
index 2f6eef5e372..04d300eeda7 100755
--- a/t/t5001-archive-attr.sh
+++ b/t/t5001-archive-attr.sh
@@ -3,6 +3,7 @@
test_description='git archive attribute tests'
TEST_CREATE_REPO_NO_TEMPLATE=1
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
SUBSTFORMAT='%H (%h)%n'
diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
index ae508e21623..9f2c6da80e8 100755
--- a/t/t5004-archive-corner-cases.sh
+++ b/t/t5004-archive-corner-cases.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='test corner cases of git-archive'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# the 10knuls.tar file is used to test for an empty git generated tar
diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
index fc2a6cf5c7a..9b46da7aaa7 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='git reset --patch'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./lib-patch-mode.sh
test_expect_success PERL 'setup' '
diff --git a/t/t7106-reset-unborn-branch.sh b/t/t7106-reset-unborn-branch.sh
index ecb85c3b823..a0b67a0b843 100755
--- a/t/t7106-reset-unborn-branch.sh
+++ b/t/t7106-reset-unborn-branch.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='git reset should work on unborn branch'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t7107-reset-pathspec-file.sh b/t/t7107-reset-pathspec-file.sh
index 523efbecde1..af5ea406db3 100755
--- a/t/t7107-reset-pathspec-file.sh
+++ b/t/t7107-reset-pathspec-file.sh
@@ -2,6 +2,7 @@
test_description='reset --pathspec-from-file'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_tick
diff --git a/t/t7301-clean-interactive.sh b/t/t7301-clean-interactive.sh
index a07e8b86de2..d82a3210a1d 100755
--- a/t/t7301-clean-interactive.sh
+++ b/t/t7301-clean-interactive.sh
@@ -2,6 +2,7 @@
test_description='git clean -i basic tests'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-terminal.sh
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v5 04/19] clone: use free() instead of UNLEAK()
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-avarab@gmail.com>
Change an UNLEAK() added in 0c4542738e6 (clone: free or UNLEAK further
pointers when finished, 2021-03-14) to use a "to_free" pattern
instead. In this case the "repo" can be either this absolute_pathdup()
value, or in the "else if" branch seen in the context the the
"argv[0]" argument to "main()".
We can only free() the value in the former case, hence the "to_free"
pattern.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/clone.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 5453ba5277f..ba82f5e4108 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -892,6 +892,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
int is_bundle = 0, is_local;
int reject_shallow = 0;
const char *repo_name, *repo, *work_tree, *git_dir;
+ char *repo_to_free = NULL;
char *path = NULL, *dir, *display_repo = NULL;
int dest_exists, real_dest_exists = 0;
const struct ref *refs, *remote_head;
@@ -949,7 +950,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
path = get_repo_path(repo_name, &is_bundle);
if (path) {
FREE_AND_NULL(path);
- repo = absolute_pathdup(repo_name);
+ repo = repo_to_free = absolute_pathdup(repo_name);
} else if (strchr(repo_name, ':')) {
repo = repo_name;
display_repo = transport_anonymize_url(repo);
@@ -1413,7 +1414,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
free(unborn_head);
free(dir);
free(path);
- UNLEAK(repo);
+ free(repo_to_free);
junk_mode = JUNK_LEAVE_ALL;
transport_ls_refs_options_release(&transport_ls_refs_options);
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v5 03/19] commit-graph: use free_commit_graph() instead of UNLEAK()
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-avarab@gmail.com>
In 0bfb48e6723 (builtin/commit-graph.c: UNLEAK variables, 2018-10-03)
this was made to UNLEAK(), but we can just as easily invoke the
free_commit_graph() function added in c3756d5b7fc (commit-graph: add
free_commit_graph, 2018-07-11) instead.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/commit-graph.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index e8f77f535f3..0102ac8540e 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -67,6 +67,7 @@ static int graph_verify(int argc, const char **argv, const char *prefix)
int fd;
struct stat st;
int flags = 0;
+ int ret;
static struct option builtin_commit_graph_verify_options[] = {
OPT_BOOL(0, "shallow", &opts.shallow,
@@ -111,8 +112,9 @@ static int graph_verify(int argc, const char **argv, const char *prefix)
if (!graph)
return !!open_ok;
- UNLEAK(graph);
- return verify_commit_graph(the_repository, graph, flags);
+ ret = verify_commit_graph(the_repository, graph, flags);
+ free_commit_graph(graph);
+ return ret;
}
extern int read_replace_refs;
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
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