Use string_list, pass argc, and list of arguments into command function #3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Stress test the usage emitted when options are combined in ways that isn't supported. Let's test various option combinations, some of these we buggily allow right now. E.g. this reveals a bug in 3214594 (cat-file: support --textconv/--filters in batch mode, 2016-09-09) that we'll fix in a subsequent commit. We're supposed to be emitting a relevant message when --batch-all-objects is combined with --textconv or --filters, but we don't. The cases of needing to assign to opt=2 in the "opt" loop are because on those we do the right thing already, in subsequent commits the "test_expect_failure" cases will be fixed, and the for-loops unified. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Add tests for the output that's emitted when we disambiguate
<obj>:<path> in cat-file. This gives us a baseline for improving these
messages.
For e.g. "git blame" we'll emit:
$ git blame HEAD:foo
fatal: no such path 'HEAD:foo' in HEAD
But cat-file doesn't disambiguate these two cases, and just gives the
rather unhelpful:
$ git cat-file --textconv HEAD:foo
fatal: Not a valid object name HEAD:foo
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Add a usage_msg_optf() as a shorthand for the sort of usage_msg_opt(xstrfmt(...)) used in builtin/stash.c. I'll make more use of this function in builtin/cat-file.c shortly. The disconnect between the "..." and "fmt" is a bit unusual, but it works just fine and this keeps it consistent with usage_msg_opt(), i.e. a caller of it can be moved to usage_msg_optf() and not have to have its arguments re-arranged. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
There were various inaccuracies in the previous SYNOPSIS output, e.g. "--path" is not something that can optionally go with any options except --textconv or --filters, as the output implied. The opening line of the DESCRIPTION section is also "In its first form[...]", which refers to "git cat-file <type> <object>", but the SYNOPSIS section wasn't showing that as the first form! That part of the documentation made sense in d83a42f (Documentation: minor grammatical fixes in git-cat-file.txt, 2009-03-22) when it was introduced, but since then various options that were added have made that intro make no sense in the context it was in. Now the two will match again. The usage output here is not properly aligned on "master" currently, but will be with my in-flight 4631cfc (parse-options: properly align continued usage output, 2021-09-21), so let's indent things correctly in the C code in anticipation of that. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
There's no benefit to defining this at a distance, and it makes the code harder to read as you've got to scroll up to see the usage that corresponds to the options. In subsequent commits I'll make use of usage_msg_opt(), which will be quite noisy if I have to use the long "cat_file_usage" variable, there's no other command being defined in this file, so let's rename it to just "usage". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
The usage of OPT_CMDMODE() in "cat-file"[1] was added in parallel with
the development of[3] the --batch-all-objects option[4], so we've
since grown[5] checks that it can't be combined with other command
modes, when it should just be made a top-level command-mode
instead. It doesn't combine with --filters, --textconv etc.
By giving parse_options() information about what options are mutually
exclusive with one another we can get the die() message being removed
here for free, we didn't even use that removed message in some cases,
e.g. for both of:
--batch-all-objects --textconv
--batch-all-objects --filters
We'd take the "goto usage" in the "if (opt)" branch, and never reach
the previous message. Now we'll emit e.g.:
$ git cat-file --batch-all-objects --filters
error: option `filters' is incompatible with --batch-all-objects
1. b48158a (cat-file: make the options mutually exclusive, 2015-05-03)
2. https://lore.kernel.org/git/xmqqtwspgusf.fsf@gitster.dls.corp.google.com/
3. https://lore.kernel.org/git/20150622104559.GG14475@peff.net/
4. 6a95193 (cat-file: add --batch-all-objects option, 2015-06-22)
5. 3214594 (cat-file: support --textconv/--filters in batch mode, 2016-09-09)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
With the migration of --batch-all-objects to OPT_CMDMODE() in the
preceding commit one bug with combining it and other OPT_CMDMODE()
options was solved, but we were still left with e.g. --buffer silently
being discarded when not in batch mode.
Fix all those bugs, and in addition emit errors telling the user
specifically what options can't be combined with what other options,
before this we'd usually just emit the cryptic usage text and leave
the users to work it out by themselves.
This change is rather large, because to do so we need to untangle the
options processing so that we can not only error out, but emit
sensible errors, and e.g. emit errors about options before errors
about stray argc elements (as they might become valid if the option
were removed).
Some of the output changes ("error:" to "fatal:" with
usage_msg_opt[f]()), but none of the exit codes change, except in
those cases where we silently accepted bad option combinations before,
now we'll error out.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Change the usage output emitted on "git cat-file -h" to group related
options, making it clear to users which options go with which other
ones.
The new output is:
Check object existence or emit object contents
-e check if <object> exists
-p pretty-print <object> content
Emit [broken] object attributes
-t show object type (one of 'blob', 'tree', 'commit', 'tag', ...)
-s show object size
--allow-unknown-type allow -s and -t to work with broken/corrupt objects
Batch objects requested on stdin (or --batch-all-objects)
--batch[=<format>] show full <object> or <rev> contents
--batch-check[=<format>]
like --batch, but don't emit <contents>
--batch-all-objects with --batch[-check]: ignores stdin, batches all known objects
Change or optimize batch output
--buffer buffer --batch output
--follow-symlinks follow in-tree symlinks
--unordered do not order objects before emitting them
Emit object (blob or tree) with conversion or filter (stand-alone, or with batch)
--textconv run textconv on object's content
--filters run filters on object's content
--path blob|tree use a <path> for (--textconv | --filters ); Not with 'batch'
The old usage was:
<type> can be one of: blob, tree, commit, tag
-t show object type
-s show object size
-e exit with zero when there's no error
-p pretty-print object's content
--textconv for blob objects, run textconv on object's content
--filters for blob objects, run filters on object's content
--batch-all-objects show all objects with --batch or --batch-check
--path <blob> use a specific path for --textconv/--filters
--allow-unknown-type allow -s and -t to work with broken/corrupt objects
--buffer buffer --batch output
--batch[=<format>] show info and content of objects fed from the standard input
--batch-check[=<format>]
show info about objects fed from the standard input
--follow-symlinks follow in-tree symlinks (used with --batch or --batch-check)
--unordered do not order --batch-all-objects output
While shorter, I think the new one is easier to understand, as
e.g. "--allow-unknown-type" is grouped with "-t" and "-s", as it can
only be combined with those options. The same goes for "--buffer",
"--unordered" etc.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Stop having GET_OID_ONLY_TO_DIE imply GET_OID_QUIETLY in get_oid_with_context_1(). The *_DIE flag was added in 33bd598 (sha1_name.c: teach lookup context to get_sha1_with_context(), 2012-07-02), and then later tweaked in 7243ffd (get_sha1: avoid repeating ourselves via ONLY_TO_DIE, 2016-09-26). Everything in that commit makes sense, but only for callers that expect to fail in an initial call to get_oid_with_context_1(), e.g. as "git show 0017" does via handle_revision_arg(), and then would like to call get_oid_with_context_1() again via this maybe_die_on_misspelt_object_name() function. In the subsequent commit we'll add a new caller that expects to call this only once, but who would still like to have all the error messaging that GET_OID_ONLY_TO_DIE gives it, in addition to any regular errors. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Change the cat_one_file() logic that calls get_oid_with_context() under --textconv and --filters to use the GET_OID_ONLY_TO_DIE flag, thus improving the error messaging emitted when e.g. <path> is missing but <rev> is not. To service the "cat-file" use-case we need to introduce a new "GET_OID_REQUIRE_PATH" flag, otherwise it would exit early as soon as a valid "HEAD" was resolved, but in the "cat-file" case being changed we always need a valid revision and path. This arguably makes the "<bad rev>:<bad path>" and "<bad rev>:<good (in HEAD) path>" use cases worse, as we won't quote the <path> component at the user anymore, but let's just use the existing logic "git log" et al use for now. We can improve the messaging for those cases as a follow-up for all callers. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
This WIP patch is mostly stealing code from builtin/update-ref.c and
implementing the same sort of prefixed command-mode that it
supports. I.e. in addition to --batch now supporting:
<object> LF
It'll support with --stdin-cmd, with and without -z, respectively:
object <object> NL
object <object> NUL
The plus being that we can now implement additional commands. Let's
start that by scratching the itch John Cai wanted to address in [1]
and implement a (with and without -z):
fflush NL
fflush NUL
That command simply calls fflush(stdout), which could be done as an
emergent effect before by feeding the input a "NL".
I think this will be useful for other things, e.g. I've observed in
the past that a not-trivial part of "cat-file --batch" time is spent
on parsing its <object> argument and seeing if it's a revision, ref
etc.
So we could e.g. add a command that only accepts a full-length 40
character SHA-1, or switch the --format output mid-request etc.
1. https://lore.kernel.org/git/pull.1124.git.git.1636149400.gitgitgadget@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
avar
force-pushed
the
avar/cat-file-stdin-cmd-mode
branch
from
9d9e36b to
67590f3
Compare
john-cai
force-pushed
the
jc/cat-file-stdin-cmd-mode
branch
5 times, most recently
from
d90999d to
cea1761
Compare
added 2 commits
January 3, 2022 14:39
WIP: To allow for future commands that may take more than 1 argument, make the signature of the fn in the parse_cmd struct include argc, argv. argc is the number of arguments that were found in the line, and argv is a NULL terminated array of strbufs that contain the arguments that were found on the line. DIE if the arguments found do not match the number specified in the parse_cmd struct. This change will allow the --stdin-cmd mode to support: <command> NUL <arg1> NUL <arg2> NUL <command> SP <arg1> SP <arg2> LF With an arbitrary amount of arguments. Signed-off-by: John Cai <johncai86@gmail.com>
WIP: Add tests that verify the behavior of the object command in --stdin-cmd mode. Signed-off-by: John Cai <johncai86@gmail.com>
john-cai
force-pushed
the
jc/cat-file-stdin-cmd-mode
branch
from
cea1761 to
f130e2d
Compare
avar
force-pushed
the
avar/cat-file-stdin-cmd-mode
branch
2 times, most recently
from
f7b2be5 to
4af8e14
Compare
avar
added a commit
that referenced
this pull request
Jan 11, 2022
=> This is the 1st commit message:
format-patch: format prettir Message-ID's
Change the Message-ID's from e.g.:
$ git --no-pager format-patch --cover-letter --stdout HEAD~5.. | grep Message-Id
Message-Id: <cover.1616506444.git.avarab@gmail.com>
Message-Id: <5f308a89d8c37a186d58beff88542df00bc67975.1616506444.git.avarab@gmail.com>
Message-Id: <23c781f1731bb77c16fb9c9a37e9d88f707af9bd.1616506444.git.avarab@gmail.com>
Message-Id: <6d875d19fd7346f4a6dc47d2a1363db91e9643d5.1616506444.git.avarab@gmail.com>
Message-Id: <98fe9e666fe4a595cf5396fd8d5c57b380c782b2.1616506444.git.avarab@gmail.com>
Message-Id: <42efa1231aee85932058cc6d1571ab4ceb3e7eff.1616506444.git.avarab@gmail.com>
Message-Id: <75555676ad3908b0f847a9ae154c35e12114c82f.1616506444.git.avarab@gmail.com>
Message-Id: <142430338477d9d1bb25be66267225fb58498d92.1616506444.git.avarab@gmail.com>
Message-Id: <bff81a3d39ed412e1b5be2cc570ab63ff6604810.1616506444.git.avarab@gmail.com>
To:
$ git --no-pager format-patch --cover-letter --stdout HEAD~5.. | grep Message-Id
Message-Id: <cover-0.9-00000000000-20210323T133337Z-avarab@gmail.com>
Message-Id: <patch-1.9-5f308a89d8c-20210323T133337Z-avarab@gmail.com>
Message-Id: <patch-2.9-23c781f1731-20210323T133337Z-avarab@gmail.com>
Message-Id: <patch-3.9-6d875d19fd7-20210323T133337Z-avarab@gmail.com>
Message-Id: <patch-4.9-98fe9e666fe-20210323T133337Z-avarab@gmail.com>
Message-Id: <patch-5.9-42efa1231ae-20210323T133337Z-avarab@gmail.com>
Message-Id: <patch-6.9-75555676ad3-20210323T133337Z-avarab@gmail.com>
Message-Id: <patch-7.9-14243033847-20210323T133337Z-avarab@gmail.com>
Message-Id: <patch-8.9-bff81a3d39e-20210323T133337Z-avarab@gmail.com>
It also does the right thing when it comes to padding the patch number
there's >9 messages (or >99, >999, ...). E.g.:
$ git --no-pager format-patch --cover-letter --stdout HEAD~10.. | grep -m2 Message-Id
Message-Id: <cover-00.19-00000000000-20210323T133308Z-avarab@gmail.com>
Message-Id: <patch-01.19-15ae82d5d6c-20210323T133308Z-avarab@gmail.com>
And when --cover-letter isn't specified:
$ git --no-pager format-patch --stdout HEAD~2.. | grep -m2 Message-Id
Message-Id: <patch-1.2-f157d3a5c2d-20210328T155559Z-avarab@gmail.com>
Message-Id: <patch-2.2-a724c3ed353-20210328T155559Z-avarab@gmail.com>
Summary of things changed and to discuss:
* We include the N/TOTAL position in the series, as noted above.
* The ".git." part is gone, it didn't add any worthwhile signal. The
main point of the Message-ID to be unique, sticking the MTA in there
isn't needed. We've got --xmailer for that.
* Let's use abbreviated OIDs, using null_oid as noted in a comment
for the cover letter
* Use ISO timestamp instead of Unix epoch. Make it easier to see at a
glance how old a patch noted e.g. with a https://lore.kernel.org/git/
discussion is.
* We're still not using any funny-but-valid characters (as before) so
e.g. gnome-terminal highlights this all as one thing to copy/paste.
Possible future improvements:
* Should we spot e.g. a --subject-prefix="PATCH v2" and change the
Message-ID to e.g. <cover-v2-$THE_REST> ?
* I emit these patches into a git repo to keep track of notes/cover
letter discussion. Sometimes I re-generate the patches and a smaller
diff would be nicer. Could we safely omit the timestamp entirely?
It would make this smaller, and presumably user.email+subject
prefix+short OID would be unique enough.
This is the first in-tree user of the %zu format specifier, which is
in new in C99. For C89 we'd use %lu and a cast to "unsigned long", but
let's see if this works. It works on the compilers I've got access
to (clang, GCC on various archs, xlc & SunCC).
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
=> This is the commit message #2:
format-patch: don't use %zu, use %lu and a cast
The Windows CI whines about it: https://github.com/avar/git/runs/2298653913
builtin/log.c: In function 'gen_message_id':
311
builtin/log.c:1047:29: error: unknown conversion type character 'z' in format [-Werror=format=]
312
1047 | strbuf_addf(&fmt, "%%s-%%0%zud.%%0%zud-%%s-%%s-%%s", tmp.len, tmp.len);
313
| ^
314
builtin/log.c:1047:37: error: unknown conversion type character 'z' in format [-Werror=format=]
315
1047 | strbuf_addf(&fmt, "%%s-%%0%zud.%%0%zud-%%s-%%s-%%s", tmp.len, tmp.len);
316
| ^
317
builtin/log.c:1047:20: error: too many arguments for format [-Werror=format-extra-args]
318
1047 | strbuf_addf(&fmt, "%%s-%%0%zud.%%0%zud-%%s-%%s-%%s", tmp.len, tmp.len);
319
CC builtin/ls-tree.o
320
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
321
CC builtin/mailinfo.o
=> This is the commit message #3:
fix screwy totals with cover letters
I just hadn't noticed this, now:
vm git (avar/format-patch-prettier-message-id $>) 0 $ ~/g/git/git format-patch --stdout HEAD~2..|grep ^Message
Message-Id: <patch-1.2-958b0f66e7-20210409T082226Z-avarab@gmail.com>
Message-Id: <patch-2.2-6f8ba890fc-20210409T082226Z-avarab@gmail.com>
vm git (avar/format-patch-prettier-message-id $>) 0 $ ~/g/git/git format-patch --cover-letter --stdout HEAD~2..|grep ^Message
Message-Id: <cover-0.2-0000000000-20210409T082231Z-avarab@gmail.com>
Message-Id: <patch-1.2-958b0f66e7-20210409T082231Z-avarab@gmail.com>
Message-Id: <patch-2.2-6f8ba890fc-20210409T082231Z-avarab@gmail.com>
Before:
vm git (avar/format-patch-prettier-message-id $>) 0 $ git format-patch --cover-letter --stdout HEAD~2..|grep ^Message
Message-Id: <cover-0.3-0000000000-20210409T082344Z-avarab@gmail.com>
Message-Id: <patch-1.3-6f8ba890fc-20210409T082344Z-avarab@gmail.com>
Message-Id: <patch-2.3-c6a79a2465-20210409T082344Z-avarab@gmail.com>
vm git (avar/format-patch-prettier-message-id $>) 0 $ git format-patch --stdout HEAD~2..|grep ^Message
Message-Id: <patch-1.2-6f8ba890fc-20210409T082347Z-avarab@gmail.com>
Message-Id: <patch-2.2-c6a79a2465-20210409T082347Z-avarab@gmail.com>
=> This is the commit message #4:
fixup conflict with master, use null_oid() as a function
avar
pushed a commit
that referenced
this pull request
Jan 13, 2022
When fetching packfiles, we write a bunch of lockfiles for the packfiles
we're writing into the repository. In order to not leave behind any
cruft in case we exit or receive a signal, we register both an exit
handler as well as signal handlers for common signals like SIGINT. These
handlers will then unlink the locks and free the data structure tracking
them. We have observed a deadlock in this logic though:
(gdb) bt
#0 __lll_lock_wait_private () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:95
#1 0x00007f4932bea2cd in _int_free (av=0x7f4932f2eb20 <main_arena>, p=0x3e3e4200, have_lock=0) at malloc.c:3969
#2 0x00007f4932bee58c in __GI___libc_free (mem=<optimized out>) at malloc.c:2975
#3 0x0000000000662ab1 in string_list_clear ()
#4 0x000000000044f5bc in unlock_pack_on_signal ()
#5 <signal handler called>
git#6 _int_free (av=0x7f4932f2eb20 <main_arena>, p=<optimized out>, have_lock=0) at malloc.c:4024
#7 0x00007f4932bee58c in __GI___libc_free (mem=<optimized out>) at malloc.c:2975
git#8 0x000000000065afd5 in strbuf_release ()
git#9 0x000000000066ddb9 in delete_tempfile ()
git#10 0x0000000000610d0b in files_transaction_cleanup.isra ()
git#11 0x0000000000611718 in files_transaction_abort ()
git#12 0x000000000060d2ef in ref_transaction_abort ()
git#13 0x000000000060d441 in ref_transaction_prepare ()
git#14 0x000000000060e0b5 in ref_transaction_commit ()
git#15 0x00000000004511c2 in fetch_and_consume_refs ()
git#16 0x000000000045279a in cmd_fetch ()
git#17 0x0000000000407c48 in handle_builtin ()
git#18 0x0000000000408df2 in cmd_main ()
git#19 0x00000000004078b5 in main ()
The process was killed with a signal, which caused the signal handler to
kick in and try free the data structures after we have unlinked the
locks. It then deadlocks while calling free(3P).
The root cause of this is that it is not allowed to call certain
functions in async-signal handlers, as specified by signal-safety(7).
Next to most I/O functions, this list of disallowed functions also
includes memory-handling functions like malloc(3P) and free(3P) because
they may not be reentrant. As a result, if we execute such functions in
the signal handler, then they may operate on inconistent state and fail
in unexpected ways.
Fix this bug by not calling non-async-signal-safe functions when running
in the signal handler. We're about to re-raise the signal anyway and
will thus exit, so it's not much of a problem to keep the string list of
lockfiles untouched. Note that it's fine though to call unlink(2), so
we'll still clean up the lockfiles correctly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Reviewed-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
avar
added a commit
that referenced
this pull request
Feb 1, 2022
Add "fast_unwind_on_malloc=0" to LSAN_OPTIONS to get more meaningful
stack traces from LSAN. This isn't required under ASAN which will emit
traces such as this one for a leak in "t/t0006-date.sh":
$ ASAN_OPTIONS=detect_leaks=1 ./t0006-date.sh -vixd
[...]
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x488b94 in strdup (t/helper/test-tool+0x488b94)
#1 0x9444a4 in xstrdup wrapper.c:29:14
#2 0x5995fa in parse_date_format date.c:991:24
#3 0x4d2056 in show_dates t/helper/test-date.c:39:2
#4 0x4d174a in cmd__date t/helper/test-date.c:116:3
#5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11
git#6 0x4cd1e3 in main common-main.c:52:11
#7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16
git#8 0x422b09 in _start (t/helper/test-tool+0x422b09)
SUMMARY: AddressSanitizer: 3 byte(s) leaked in 1 allocation(s).
Aborted
Whereas LSAN would emit this instead:
$ ./t0006-date.sh -vixd
[...]
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
#1 0x7f2be1d614aa in strdup string/strdup.c:42:15
SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
Aborted
Now we'll instead git this sensible stack trace under
LSAN. I.e. almost the same one (but starting with "malloc", as is
usual for LSAN) as under ASAN:
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
#1 0x7f012af5c4aa in strdup string/strdup.c:42:15
#2 0x5cb164 in xstrdup wrapper.c:29:14
#3 0x495ee9 in parse_date_format date.c:991:24
#4 0x453aac in show_dates t/helper/test-date.c:39:2
#5 0x453782 in cmd__date t/helper/test-date.c:116:3
git#6 0x451d95 in cmd_main t/helper/test-tool.c:127:11
#7 0x451f1e in main common-main.c:52:11
git#8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16
git#9 0x42e0a9 in _start (t/helper/test-tool+0x42e0a9)
SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
Aborted
As the option name suggests this does make things slower, e.g. for
t0001-init.sh we're around 10% slower:
$ hyperfine -L v 0,1 'LSAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 3
Benchmark 1: LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh
Time (mean ± σ): 2.135 s ± 0.015 s [User: 1.951 s, System: 0.554 s]
Range (min … max): 2.122 s … 2.152 s 3 runs
Benchmark 2: LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh
Time (mean ± σ): 1.981 s ± 0.055 s [User: 1.769 s, System: 0.488 s]
Range (min … max): 1.941 s … 2.044 s 3 runs
Summary
'LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran
1.08 ± 0.03 times faster than 'LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'
I think that's more than worth it to get the more meaningful stack
traces, we can always provide LSAN_OPTIONS=fast_unwind_on_malloc=0 for
one-off "fast" runs.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
avar
added a commit
that referenced
this pull request
Feb 4, 2022
Add "fast_unwind_on_malloc=0" to LSAN_OPTIONS to get more meaningful
stack traces from LSAN. This isn't required under ASAN which will emit
traces such as this one for a leak in "t/t0006-date.sh":
$ ASAN_OPTIONS=detect_leaks=1 ./t0006-date.sh -vixd
[...]
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x488b94 in strdup (t/helper/test-tool+0x488b94)
#1 0x9444a4 in xstrdup wrapper.c:29:14
#2 0x5995fa in parse_date_format date.c:991:24
#3 0x4d2056 in show_dates t/helper/test-date.c:39:2
#4 0x4d174a in cmd__date t/helper/test-date.c:116:3
#5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11
git#6 0x4cd1e3 in main common-main.c:52:11
#7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16
git#8 0x422b09 in _start (t/helper/test-tool+0x422b09)
SUMMARY: AddressSanitizer: 3 byte(s) leaked in 1 allocation(s).
Aborted
Whereas LSAN would emit this instead:
$ ./t0006-date.sh -vixd
[...]
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
#1 0x7f2be1d614aa in strdup string/strdup.c:42:15
SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
Aborted
Now we'll instead git this sensible stack trace under
LSAN. I.e. almost the same one (but starting with "malloc", as is
usual for LSAN) as under ASAN:
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
#1 0x7f012af5c4aa in strdup string/strdup.c:42:15
#2 0x5cb164 in xstrdup wrapper.c:29:14
#3 0x495ee9 in parse_date_format date.c:991:24
#4 0x453aac in show_dates t/helper/test-date.c:39:2
#5 0x453782 in cmd__date t/helper/test-date.c:116:3
git#6 0x451d95 in cmd_main t/helper/test-tool.c:127:11
#7 0x451f1e in main common-main.c:52:11
git#8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16
git#9 0x42e0a9 in _start (t/helper/test-tool+0x42e0a9)
SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
Aborted
As the option name suggests this does make things slower, e.g. for
t0001-init.sh we're around 10% slower:
$ hyperfine -L v 0,1 'LSAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 3
Benchmark 1: LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh
Time (mean ± σ): 2.135 s ± 0.015 s [User: 1.951 s, System: 0.554 s]
Range (min … max): 2.122 s … 2.152 s 3 runs
Benchmark 2: LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh
Time (mean ± σ): 1.981 s ± 0.055 s [User: 1.769 s, System: 0.488 s]
Range (min … max): 1.941 s … 2.044 s 3 runs
Summary
'LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran
1.08 ± 0.03 times faster than 'LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'
I think that's more than worth it to get the more meaningful stack
traces, we can always provide LSAN_OPTIONS=fast_unwind_on_malloc=0 for
one-off "fast" runs.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
avar
added a commit
that referenced
this pull request
Feb 17, 2022
Add "fast_unwind_on_malloc=0" to LSAN_OPTIONS to get more meaningful
stack traces from LSAN. This isn't required under ASAN which will emit
traces such as this one for a leak in "t/t0006-date.sh":
$ ASAN_OPTIONS=detect_leaks=1 ./t0006-date.sh -vixd
[...]
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x488b94 in strdup (t/helper/test-tool+0x488b94)
#1 0x9444a4 in xstrdup wrapper.c:29:14
#2 0x5995fa in parse_date_format date.c:991:24
#3 0x4d2056 in show_dates t/helper/test-date.c:39:2
#4 0x4d174a in cmd__date t/helper/test-date.c:116:3
#5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11
git#6 0x4cd1e3 in main common-main.c:52:11
#7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16
git#8 0x422b09 in _start (t/helper/test-tool+0x422b09)
SUMMARY: AddressSanitizer: 3 byte(s) leaked in 1 allocation(s).
Aborted
Whereas LSAN would emit this instead:
$ ./t0006-date.sh -vixd
[...]
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
#1 0x7f2be1d614aa in strdup string/strdup.c:42:15
SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
Aborted
Now we'll instead git this sensible stack trace under
LSAN. I.e. almost the same one (but starting with "malloc", as is
usual for LSAN) as under ASAN:
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
#1 0x7f012af5c4aa in strdup string/strdup.c:42:15
#2 0x5cb164 in xstrdup wrapper.c:29:14
#3 0x495ee9 in parse_date_format date.c:991:24
#4 0x453aac in show_dates t/helper/test-date.c:39:2
#5 0x453782 in cmd__date t/helper/test-date.c:116:3
git#6 0x451d95 in cmd_main t/helper/test-tool.c:127:11
#7 0x451f1e in main common-main.c:52:11
git#8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16
git#9 0x42e0a9 in _start (t/helper/test-tool+0x42e0a9)
SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
Aborted
As the option name suggests this does make things slower, e.g. for
t0001-init.sh we're around 10% slower:
$ hyperfine -L v 0,1 'LSAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 3
Benchmark 1: LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh
Time (mean ± σ): 2.135 s ± 0.015 s [User: 1.951 s, System: 0.554 s]
Range (min … max): 2.122 s … 2.152 s 3 runs
Benchmark 2: LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh
Time (mean ± σ): 1.981 s ± 0.055 s [User: 1.769 s, System: 0.488 s]
Range (min … max): 1.941 s … 2.044 s 3 runs
Summary
'LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran
1.08 ± 0.03 times faster than 'LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'
I think that's more than worth it to get the more meaningful stack
traces, we can always provide LSAN_OPTIONS=fast_unwind_on_malloc=0 for
one-off "fast" runs.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
avar
added a commit
that referenced
this pull request
Feb 18, 2022
Add "fast_unwind_on_malloc=0" to LSAN_OPTIONS to get more meaningful
stack traces from LSAN. This isn't required under ASAN which will emit
traces such as this one for a leak in "t/t0006-date.sh":
$ ASAN_OPTIONS=detect_leaks=1 ./t0006-date.sh -vixd
[...]
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x488b94 in strdup (t/helper/test-tool+0x488b94)
#1 0x9444a4 in xstrdup wrapper.c:29:14
#2 0x5995fa in parse_date_format date.c:991:24
#3 0x4d2056 in show_dates t/helper/test-date.c:39:2
#4 0x4d174a in cmd__date t/helper/test-date.c:116:3
#5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11
git#6 0x4cd1e3 in main common-main.c:52:11
#7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16
git#8 0x422b09 in _start (t/helper/test-tool+0x422b09)
SUMMARY: AddressSanitizer: 3 byte(s) leaked in 1 allocation(s).
Aborted
Whereas LSAN would emit this instead:
$ ./t0006-date.sh -vixd
[...]
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
#1 0x7f2be1d614aa in strdup string/strdup.c:42:15
SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
Aborted
Now we'll instead git this sensible stack trace under
LSAN. I.e. almost the same one (but starting with "malloc", as is
usual for LSAN) as under ASAN:
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
#1 0x7f012af5c4aa in strdup string/strdup.c:42:15
#2 0x5cb164 in xstrdup wrapper.c:29:14
#3 0x495ee9 in parse_date_format date.c:991:24
#4 0x453aac in show_dates t/helper/test-date.c:39:2
#5 0x453782 in cmd__date t/helper/test-date.c:116:3
git#6 0x451d95 in cmd_main t/helper/test-tool.c:127:11
#7 0x451f1e in main common-main.c:52:11
git#8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16
git#9 0x42e0a9 in _start (t/helper/test-tool+0x42e0a9)
SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
Aborted
As the option name suggests this does make things slower, e.g. for
t0001-init.sh we're around 10% slower:
$ hyperfine -L v 0,1 'LSAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 3
Benchmark 1: LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh
Time (mean ± σ): 2.135 s ± 0.015 s [User: 1.951 s, System: 0.554 s]
Range (min … max): 2.122 s … 2.152 s 3 runs
Benchmark 2: LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh
Time (mean ± σ): 1.981 s ± 0.055 s [User: 1.769 s, System: 0.488 s]
Range (min … max): 1.941 s … 2.044 s 3 runs
Summary
'LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran
1.08 ± 0.03 times faster than 'LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'
I think that's more than worth it to get the more meaningful stack
traces, we can always provide LSAN_OPTIONS=fast_unwind_on_malloc=0 for
one-off "fast" runs.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
avar
added a commit
that referenced
this pull request
Feb 18, 2022
Add "fast_unwind_on_malloc=0" to LSAN_OPTIONS to get more meaningful
stack traces from LSAN. This isn't required under ASAN which will emit
traces such as this one for a leak in "t/t0006-date.sh":
$ ASAN_OPTIONS=detect_leaks=1 ./t0006-date.sh -vixd
[...]
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x488b94 in strdup (t/helper/test-tool+0x488b94)
#1 0x9444a4 in xstrdup wrapper.c:29:14
#2 0x5995fa in parse_date_format date.c:991:24
#3 0x4d2056 in show_dates t/helper/test-date.c:39:2
#4 0x4d174a in cmd__date t/helper/test-date.c:116:3
#5 0x4cce89 in cmd_main t/helper/test-tool.c:127:11
git#6 0x4cd1e3 in main common-main.c:52:11
#7 0x7fef3c695e49 in __libc_start_main csu/../csu/libc-start.c:314:16
git#8 0x422b09 in _start (t/helper/test-tool+0x422b09)
SUMMARY: AddressSanitizer: 3 byte(s) leaked in 1 allocation(s).
Aborted
Whereas LSAN would emit this instead:
$ ./t0006-date.sh -vixd
[...]
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
#1 0x7f2be1d614aa in strdup string/strdup.c:42:15
SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
Aborted
Now we'll instead git this sensible stack trace under
LSAN. I.e. almost the same one (but starting with "malloc", as is
usual for LSAN) as under ASAN:
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x4323b8 in malloc (t/helper/test-tool+0x4323b8)
#1 0x7f012af5c4aa in strdup string/strdup.c:42:15
#2 0x5cb164 in xstrdup wrapper.c:29:14
#3 0x495ee9 in parse_date_format date.c:991:24
#4 0x453aac in show_dates t/helper/test-date.c:39:2
#5 0x453782 in cmd__date t/helper/test-date.c:116:3
git#6 0x451d95 in cmd_main t/helper/test-tool.c:127:11
#7 0x451f1e in main common-main.c:52:11
git#8 0x7f012aef5e49 in __libc_start_main csu/../csu/libc-start.c:314:16
git#9 0x42e0a9 in _start (t/helper/test-tool+0x42e0a9)
SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
Aborted
As the option name suggests this does make things slower, e.g. for
t0001-init.sh we're around 10% slower:
$ hyperfine -L v 0,1 'LSAN_OPTIONS=fast_unwind_on_malloc={v} make T=t0001-init.sh' -r 3
Benchmark 1: LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh
Time (mean ± σ): 2.135 s ± 0.015 s [User: 1.951 s, System: 0.554 s]
Range (min … max): 2.122 s … 2.152 s 3 runs
Benchmark 2: LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh
Time (mean ± σ): 1.981 s ± 0.055 s [User: 1.769 s, System: 0.488 s]
Range (min … max): 1.941 s … 2.044 s 3 runs
Summary
'LSAN_OPTIONS=fast_unwind_on_malloc=1 make T=t0001-init.sh' ran
1.08 ± 0.03 times faster than 'LSAN_OPTIONS=fast_unwind_on_malloc=0 make T=t0001-init.sh'
I think that's more than worth it to get the more meaningful stack
traces, we can always provide LSAN_OPTIONS=fast_unwind_on_malloc=0 for
one-off "fast" runs.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
|
Closing per this being submitted to the Git ML :) |
avar
added a commit
that referenced
this pull request
Jan 9, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Jan 10, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Jan 12, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Jan 12, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Jan 13, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
pushed a commit
that referenced
this pull request
Jan 18, 2023
There is an out-of-bounds read possible when parsing gitattributes that
have an attribute that is 2^31+1 bytes long. This is caused due to an
integer overflow when we assign the result of strlen(3P) to an `int`,
where we use the wrapped-around value in a subsequent call to
memcpy(3P). The following code reproduces the issue:
blob=$(perl -e 'print "a" x 2147483649 . " attr"' | git hash-object -w --stdin)
git update-index --add --cacheinfo 100644,$blob,.gitattributes
git check-attr --all file
AddressSanitizer:DEADLYSIGNAL
=================================================================
==8451==ERROR: AddressSanitizer: SEGV on unknown address 0x7f93efa00800 (pc 0x7f94f1f8f082 bp 0x7ffddb59b3a0 sp 0x7ffddb59ab28 T0)
==8451==The signal is caused by a READ memory access.
#0 0x7f94f1f8f082 (/usr/lib/libc.so.6+0x176082)
#1 0x7f94f2047d9c in __interceptor_strspn /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:752
#2 0x560e190f7f26 in parse_attr_line attr.c:375
#3 0x560e190f9663 in handle_attr_line attr.c:660
#4 0x560e190f9ddd in read_attr_from_index attr.c:769
#5 0x560e190f9f14 in read_attr attr.c:797
git#6 0x560e190fa24e in bootstrap_attr_stack attr.c:867
#7 0x560e190fa4a5 in prepare_attr_stack attr.c:902
git#8 0x560e190fb5dc in collect_some_attrs attr.c:1097
git#9 0x560e190fb93f in git_all_attrs attr.c:1128
git#10 0x560e18e6136e in check_attr builtin/check-attr.c:67
git#11 0x560e18e61c12 in cmd_check_attr builtin/check-attr.c:183
git#12 0x560e18e15993 in run_builtin git.c:466
git#13 0x560e18e16397 in handle_builtin git.c:721
git#14 0x560e18e16b2b in run_argv git.c:788
git#15 0x560e18e17991 in cmd_main git.c:926
git#16 0x560e190ae2bd in main common-main.c:57
git#17 0x7f94f1e3c28f (/usr/lib/libc.so.6+0x2328f)
git#18 0x7f94f1e3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349)
git#19 0x560e18e110e4 in _start ../sysdeps/x86_64/start.S:115
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/usr/lib/libc.so.6+0x176082)
==8451==ABORTING
Fix this bug by converting the variable to a `size_t` instead.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
avar
pushed a commit
that referenced
this pull request
Jan 18, 2023
It is possible to trigger an integer overflow when parsing attribute
names when there are more than 2^31 of them for a single pattern. This
can either lead to us dying due to trying to request too many bytes:
blob=$(perl -e 'print "f" . " a=" x 2147483649' | git hash-object -w --stdin)
git update-index --add --cacheinfo 100644,$blob,.gitattributes
git attr-check --all file
=================================================================
==1022==ERROR: AddressSanitizer: requested allocation size 0xfffffff800000032 (0xfffffff800001038 after adjustments for alignment, red zones etc.) exceeds maximum supported size of 0x10000000000 (thread T0)
#0 0x7fd3efabf411 in __interceptor_calloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:77
#1 0x5563a0a1e3d3 in xcalloc wrapper.c:150
#2 0x5563a058d005 in parse_attr_line attr.c:384
#3 0x5563a058e661 in handle_attr_line attr.c:660
#4 0x5563a058eddb in read_attr_from_index attr.c:769
#5 0x5563a058ef12 in read_attr attr.c:797
git#6 0x5563a058f24c in bootstrap_attr_stack attr.c:867
#7 0x5563a058f4a3 in prepare_attr_stack attr.c:902
git#8 0x5563a05905da in collect_some_attrs attr.c:1097
git#9 0x5563a059093d in git_all_attrs attr.c:1128
git#10 0x5563a02f636e in check_attr builtin/check-attr.c:67
git#11 0x5563a02f6c12 in cmd_check_attr builtin/check-attr.c:183
git#12 0x5563a02aa993 in run_builtin git.c:466
git#13 0x5563a02ab397 in handle_builtin git.c:721
git#14 0x5563a02abb2b in run_argv git.c:788
git#15 0x5563a02ac991 in cmd_main git.c:926
git#16 0x5563a05432bd in main common-main.c:57
git#17 0x7fd3ef82228f (/usr/lib/libc.so.6+0x2328f)
==1022==HINT: if you don't care about these errors you may set allocator_may_return_null=1
SUMMARY: AddressSanitizer: allocation-size-too-big /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:77 in __interceptor_calloc
==1022==ABORTING
Or, much worse, it can lead to an out-of-bounds write because we
underallocate and then memcpy(3P) into an array:
perl -e '
print "A " . "\rh="x2000000000;
print "\rh="x2000000000;
print "\rh="x294967294 . "\n"
' >.gitattributes
git add .gitattributes
git commit -am "evil attributes"
$ git clone --quiet /path/to/repo
=================================================================
==15062==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000002550 at pc 0x5555559884d5 bp 0x7fffffffbc60 sp 0x7fffffffbc58
WRITE of size 8 at 0x602000002550 thread T0
#0 0x5555559884d4 in parse_attr_line attr.c:393
#1 0x5555559884d4 in handle_attr_line attr.c:660
#2 0x555555988902 in read_attr_from_index attr.c:784
#3 0x555555988902 in read_attr_from_index attr.c:747
#4 0x555555988a1d in read_attr attr.c:800
#5 0x555555989b0c in bootstrap_attr_stack attr.c:882
git#6 0x555555989b0c in prepare_attr_stack attr.c:917
#7 0x555555989b0c in collect_some_attrs attr.c:1112
git#8 0x55555598b141 in git_check_attr attr.c:1126
git#9 0x555555a13004 in convert_attrs convert.c:1311
git#10 0x555555a95e04 in checkout_entry_ca entry.c:553
git#11 0x555555d58bf6 in checkout_entry entry.h:42
git#12 0x555555d58bf6 in check_updates unpack-trees.c:480
git#13 0x555555d5eb55 in unpack_trees unpack-trees.c:2040
git#14 0x555555785ab7 in checkout builtin/clone.c:724
git#15 0x555555785ab7 in cmd_clone builtin/clone.c:1384
git#16 0x55555572443c in run_builtin git.c:466
git#17 0x55555572443c in handle_builtin git.c:721
git#18 0x555555727872 in run_argv git.c:788
git#19 0x555555727872 in cmd_main git.c:926
git#20 0x555555721fa0 in main common-main.c:57
git#21 0x7ffff73f1d09 in __libc_start_main ../csu/libc-start.c:308
git#22 0x555555723f39 in _start (git+0x1cff39)
0x602000002552 is located 0 bytes to the right of 2-byte region [0x602000002550,0x602000002552) allocated by thread T0 here:
#0 0x7ffff768c037 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:154
#1 0x555555d7fff7 in xcalloc wrapper.c:150
#2 0x55555598815f in parse_attr_line attr.c:384
#3 0x55555598815f in handle_attr_line attr.c:660
#4 0x555555988902 in read_attr_from_index attr.c:784
#5 0x555555988902 in read_attr_from_index attr.c:747
git#6 0x555555988a1d in read_attr attr.c:800
#7 0x555555989b0c in bootstrap_attr_stack attr.c:882
git#8 0x555555989b0c in prepare_attr_stack attr.c:917
git#9 0x555555989b0c in collect_some_attrs attr.c:1112
git#10 0x55555598b141 in git_check_attr attr.c:1126
git#11 0x555555a13004 in convert_attrs convert.c:1311
git#12 0x555555a95e04 in checkout_entry_ca entry.c:553
git#13 0x555555d58bf6 in checkout_entry entry.h:42
git#14 0x555555d58bf6 in check_updates unpack-trees.c:480
git#15 0x555555d5eb55 in unpack_trees unpack-trees.c:2040
git#16 0x555555785ab7 in checkout builtin/clone.c:724
git#17 0x555555785ab7 in cmd_clone builtin/clone.c:1384
git#18 0x55555572443c in run_builtin git.c:466
git#19 0x55555572443c in handle_builtin git.c:721
git#20 0x555555727872 in run_argv git.c:788
git#21 0x555555727872 in cmd_main git.c:926
git#22 0x555555721fa0 in main common-main.c:57
git#23 0x7ffff73f1d09 in __libc_start_main ../csu/libc-start.c:308
SUMMARY: AddressSanitizer: heap-buffer-overflow attr.c:393 in parse_attr_line
Shadow bytes around the buggy address:
0x0c047fff8450: fa fa 00 02 fa fa 00 07 fa fa fd fd fa fa 00 00
0x0c047fff8460: fa fa 02 fa fa fa fd fd fa fa 00 06 fa fa 05 fa
0x0c047fff8470: fa fa fd fd fa fa 00 02 fa fa 06 fa fa fa 05 fa
0x0c047fff8480: fa fa 07 fa fa fa fd fd fa fa 00 01 fa fa 00 02
0x0c047fff8490: fa fa 00 03 fa fa 00 fa fa fa 00 01 fa fa 00 03
=>0x0c047fff84a0: fa fa 00 01 fa fa 00 02 fa fa[02]fa fa fa fa fa
0x0c047fff84b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c047fff84c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c047fff84d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c047fff84e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c047fff84f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
Shadow gap: cc
==15062==ABORTING
Fix this bug by using `size_t` instead to count the number of attributes
so that this value cannot reasonably overflow without running out of
memory before already.
Reported-by: Markus Vervier <markus.vervier@x41-dsec.de>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
avar
pushed a commit
that referenced
this pull request
Jan 18, 2023
When using a padding specifier in the pretty format passed to git-log(1)
we need to calculate the string length in several places. These string
lengths are stored in `int`s though, which means that these can easily
overflow when the input lengths exceeds 2GB. This can ultimately lead to
an out-of-bounds write when these are used in a call to memcpy(3P):
==8340==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f1ec62f97fe at pc 0x7f2127e5f427 bp 0x7ffd3bd63de0 sp 0x7ffd3bd63588
WRITE of size 1 at 0x7f1ec62f97fe thread T0
#0 0x7f2127e5f426 in __interceptor_memcpy /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827
#1 0x5628e96aa605 in format_and_pad_commit pretty.c:1762
#2 0x5628e96aa7f4 in format_commit_item pretty.c:1801
#3 0x5628e97cdb24 in strbuf_expand strbuf.c:429
#4 0x5628e96ab060 in repo_format_commit_message pretty.c:1869
#5 0x5628e96acd0f in pretty_print_commit pretty.c:2161
git#6 0x5628e95a44c8 in show_log log-tree.c:781
#7 0x5628e95a76ba in log_tree_commit log-tree.c:1117
git#8 0x5628e922bed5 in cmd_log_walk_no_free builtin/log.c:508
git#9 0x5628e922c35b in cmd_log_walk builtin/log.c:549
git#10 0x5628e922f1a2 in cmd_log builtin/log.c:883
git#11 0x5628e9106993 in run_builtin git.c:466
git#12 0x5628e9107397 in handle_builtin git.c:721
git#13 0x5628e9107b07 in run_argv git.c:788
git#14 0x5628e91088a7 in cmd_main git.c:923
git#15 0x5628e939d682 in main common-main.c:57
git#16 0x7f2127c3c28f (/usr/lib/libc.so.6+0x2328f)
git#17 0x7f2127c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349)
git#18 0x5628e91020e4 in _start ../sysdeps/x86_64/start.S:115
0x7f1ec62f97fe is located 2 bytes to the left of 4831838265-byte region [0x7f1ec62f9800,0x7f1fe62f9839)
allocated by thread T0 here:
#0 0x7f2127ebe7ea in __interceptor_realloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:85
#1 0x5628e98774d4 in xrealloc wrapper.c:136
#2 0x5628e97cb01c in strbuf_grow strbuf.c:99
#3 0x5628e97ccd42 in strbuf_addchars strbuf.c:327
#4 0x5628e96aa55c in format_and_pad_commit pretty.c:1761
#5 0x5628e96aa7f4 in format_commit_item pretty.c:1801
git#6 0x5628e97cdb24 in strbuf_expand strbuf.c:429
#7 0x5628e96ab060 in repo_format_commit_message pretty.c:1869
git#8 0x5628e96acd0f in pretty_print_commit pretty.c:2161
git#9 0x5628e95a44c8 in show_log log-tree.c:781
git#10 0x5628e95a76ba in log_tree_commit log-tree.c:1117
git#11 0x5628e922bed5 in cmd_log_walk_no_free builtin/log.c:508
git#12 0x5628e922c35b in cmd_log_walk builtin/log.c:549
git#13 0x5628e922f1a2 in cmd_log builtin/log.c:883
git#14 0x5628e9106993 in run_builtin git.c:466
git#15 0x5628e9107397 in handle_builtin git.c:721
git#16 0x5628e9107b07 in run_argv git.c:788
git#17 0x5628e91088a7 in cmd_main git.c:923
git#18 0x5628e939d682 in main common-main.c:57
git#19 0x7f2127c3c28f (/usr/lib/libc.so.6+0x2328f)
git#20 0x7f2127c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349)
git#21 0x5628e91020e4 in _start ../sysdeps/x86_64/start.S:115
SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827 in __interceptor_memcpy
Shadow bytes around the buggy address:
0x0fe458c572a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0fe458c572b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0fe458c572c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0fe458c572d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0fe458c572e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0fe458c572f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa[fa]
0x0fe458c57300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0fe458c57310: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0fe458c57320: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0fe458c57330: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0fe458c57340: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==8340==ABORTING
The pretty format can also be used in `git archive` operations via the
`export-subst` attribute. So this is what in our opinion makes this a
critical issue in the context of Git forges which allow to download an
archive of user supplied Git repositories.
Fix this vulnerability by using `size_t` instead of `int` to track the
string lengths. Add tests which detect this vulnerability when Git is
compiled with the address sanitizer.
Reported-by: Joern Schneeweisz <jschneeweisz@gitlab.com>
Original-patch-by: Joern Schneeweisz <jschneeweisz@gitlab.com>
Modified-by: Taylor Blau <me@ttalorr.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
avar
pushed a commit
that referenced
this pull request
Jan 18, 2023
With the `%>>(<N>)` pretty formatter, you can ask git-log(1) et al to
steal spaces. To do so we need to look ahead of the next token to see
whether there are spaces there. This loop takes into account ANSI
sequences that end with an `m`, and if it finds any it will skip them
until it finds the first space. While doing so it does not take into
account the buffer's limits though and easily does an out-of-bounds
read.
Add a test that hits this behaviour. While we don't have an easy way to
verify this, the test causes the following failure when run with
`SANITIZE=address`:
==37941==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000000baf at pc 0x55ba6f88e0d0 bp 0x7ffc84c50d20 sp 0x7ffc84c50d10
READ of size 1 at 0x603000000baf thread T0
#0 0x55ba6f88e0cf in format_and_pad_commit pretty.c:1712
#1 0x55ba6f88e7b4 in format_commit_item pretty.c:1801
#2 0x55ba6f9b1ae4 in strbuf_expand strbuf.c:429
#3 0x55ba6f88f020 in repo_format_commit_message pretty.c:1869
#4 0x55ba6f890ccf in pretty_print_commit pretty.c:2161
#5 0x55ba6f7884c8 in show_log log-tree.c:781
git#6 0x55ba6f78b6ba in log_tree_commit log-tree.c:1117
#7 0x55ba6f40fed5 in cmd_log_walk_no_free builtin/log.c:508
git#8 0x55ba6f41035b in cmd_log_walk builtin/log.c:549
git#9 0x55ba6f4131a2 in cmd_log builtin/log.c:883
git#10 0x55ba6f2ea993 in run_builtin git.c:466
git#11 0x55ba6f2eb397 in handle_builtin git.c:721
git#12 0x55ba6f2ebb07 in run_argv git.c:788
git#13 0x55ba6f2ec8a7 in cmd_main git.c:923
git#14 0x55ba6f581682 in main common-main.c:57
git#15 0x7f2d08c3c28f (/usr/lib/libc.so.6+0x2328f)
git#16 0x7f2d08c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349)
git#17 0x55ba6f2e60e4 in _start ../sysdeps/x86_64/start.S:115
0x603000000baf is located 1 bytes to the left of 24-byte region [0x603000000bb0,0x603000000bc8)
allocated by thread T0 here:
#0 0x7f2d08ebe7ea in __interceptor_realloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:85
#1 0x55ba6fa5b494 in xrealloc wrapper.c:136
#2 0x55ba6f9aefdc in strbuf_grow strbuf.c:99
#3 0x55ba6f9b0a06 in strbuf_add strbuf.c:298
#4 0x55ba6f9b1a25 in strbuf_expand strbuf.c:418
#5 0x55ba6f88f020 in repo_format_commit_message pretty.c:1869
git#6 0x55ba6f890ccf in pretty_print_commit pretty.c:2161
#7 0x55ba6f7884c8 in show_log log-tree.c:781
git#8 0x55ba6f78b6ba in log_tree_commit log-tree.c:1117
git#9 0x55ba6f40fed5 in cmd_log_walk_no_free builtin/log.c:508
git#10 0x55ba6f41035b in cmd_log_walk builtin/log.c:549
git#11 0x55ba6f4131a2 in cmd_log builtin/log.c:883
git#12 0x55ba6f2ea993 in run_builtin git.c:466
git#13 0x55ba6f2eb397 in handle_builtin git.c:721
git#14 0x55ba6f2ebb07 in run_argv git.c:788
git#15 0x55ba6f2ec8a7 in cmd_main git.c:923
git#16 0x55ba6f581682 in main common-main.c:57
git#17 0x7f2d08c3c28f (/usr/lib/libc.so.6+0x2328f)
git#18 0x7f2d08c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349)
git#19 0x55ba6f2e60e4 in _start ../sysdeps/x86_64/start.S:115
SUMMARY: AddressSanitizer: heap-buffer-overflow pretty.c:1712 in format_and_pad_commit
Shadow bytes around the buggy address:
0x0c067fff8120: fa fa fd fd fd fa fa fa fd fd fd fa fa fa fd fd
0x0c067fff8130: fd fd fa fa fd fd fd fd fa fa fd fd fd fa fa fa
0x0c067fff8140: fd fd fd fa fa fa fd fd fd fa fa fa fd fd fd fa
0x0c067fff8150: fa fa fd fd fd fd fa fa 00 00 00 fa fa fa fd fd
0x0c067fff8160: fd fa fa fa fd fd fd fa fa fa fd fd fd fa fa fa
=>0x0c067fff8170: fd fd fd fa fa[fa]00 00 00 fa fa fa 00 00 00 fa
0x0c067fff8180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c067fff8190: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c067fff81a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c067fff81b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c067fff81c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
Luckily enough, this would only cause us to copy the out-of-bounds data
into the formatted commit in case we really had an ANSI sequence
preceding our buffer. So this bug likely has no security consequences.
Fix it regardless by not traversing past the buffer's start.
Reported-by: Patrick Steinhardt <ps@pks.im>
Reported-by: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
avar
pushed a commit
that referenced
this pull request
Jan 18, 2023
An out-of-bounds read can be triggered when parsing an incomplete padding format string passed via `--pretty=format` or in Git archives when files are marked with the `export-subst` gitattribute. This bug exists since we have introduced support for truncating output via the `trunc` keyword a7f01c6 (pretty: support truncating in %>, %< and %><, 2013-04-19). Before this commit, we used to find the end of the formatting string by using strchr(3P). This function returns a `NULL` pointer in case the character in question wasn't found. The subsequent check whether any character was found thus simply checked the returned pointer. After the commit we switched to strcspn(3P) though, which only returns the offset to the first found character or to the trailing NUL byte. As the end pointer is now computed by adding the offset to the start pointer it won't be `NULL` anymore, and as a consequence the check doesn't do anything anymore. The out-of-bounds data that is being read can in fact end up in the formatted string. As a consequence, it is possible to leak memory contents either by calling git-log(1) or via git-archive(1) when any of the archived files is marked with the `export-subst` gitattribute. ==10888==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000000398 at pc 0x7f0356047cb2 bp 0x7fff3ffb95d0 sp 0x7fff3ffb8d78 READ of size 1 at 0x602000000398 thread T0 #0 0x7f0356047cb1 in __interceptor_strchrnul /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:725 #1 0x563b7cec9a43 in strbuf_expand strbuf.c:417 #2 0x563b7cda7060 in repo_format_commit_message pretty.c:1869 #3 0x563b7cda8d0f in pretty_print_commit pretty.c:2161 #4 0x563b7cca04c8 in show_log log-tree.c:781 #5 0x563b7cca36ba in log_tree_commit log-tree.c:1117 git#6 0x563b7c927ed5 in cmd_log_walk_no_free builtin/log.c:508 #7 0x563b7c92835b in cmd_log_walk builtin/log.c:549 git#8 0x563b7c92b1a2 in cmd_log builtin/log.c:883 git#9 0x563b7c802993 in run_builtin git.c:466 git#10 0x563b7c803397 in handle_builtin git.c:721 git#11 0x563b7c803b07 in run_argv git.c:788 git#12 0x563b7c8048a7 in cmd_main git.c:923 git#13 0x563b7ca99682 in main common-main.c:57 git#14 0x7f0355e3c28f (/usr/lib/libc.so.6+0x2328f) git#15 0x7f0355e3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) git#16 0x563b7c7fe0e4 in _start ../sysdeps/x86_64/start.S:115 0x602000000398 is located 0 bytes to the right of 8-byte region [0x602000000390,0x602000000398) allocated by thread T0 here: #0 0x7f0356072faa in __interceptor_strdup /usr/src/debug/gcc/libsanitizer/asan/asan_interceptors.cpp:439 #1 0x563b7cf7317c in xstrdup wrapper.c:39 #2 0x563b7cd9a06a in save_user_format pretty.c:40 #3 0x563b7cd9b3e5 in get_commit_format pretty.c:173 #4 0x563b7ce54ea0 in handle_revision_opt revision.c:2456 #5 0x563b7ce597c9 in setup_revisions revision.c:2850 git#6 0x563b7c9269e0 in cmd_log_init_finish builtin/log.c:269 #7 0x563b7c927362 in cmd_log_init builtin/log.c:348 git#8 0x563b7c92b193 in cmd_log builtin/log.c:882 git#9 0x563b7c802993 in run_builtin git.c:466 git#10 0x563b7c803397 in handle_builtin git.c:721 git#11 0x563b7c803b07 in run_argv git.c:788 git#12 0x563b7c8048a7 in cmd_main git.c:923 git#13 0x563b7ca99682 in main common-main.c:57 git#14 0x7f0355e3c28f (/usr/lib/libc.so.6+0x2328f) git#15 0x7f0355e3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) git#16 0x563b7c7fe0e4 in _start ../sysdeps/x86_64/start.S:115 SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:725 in __interceptor_strchrnul Shadow bytes around the buggy address: 0x0c047fff8020: fa fa fd fd fa fa 00 06 fa fa 05 fa fa fa fd fd 0x0c047fff8030: fa fa 00 02 fa fa 06 fa fa fa 05 fa fa fa fd fd 0x0c047fff8040: fa fa 00 07 fa fa 03 fa fa fa fd fd fa fa 00 00 0x0c047fff8050: fa fa 00 01 fa fa fd fd fa fa 00 00 fa fa 00 01 0x0c047fff8060: fa fa 00 06 fa fa 00 06 fa fa 05 fa fa fa 05 fa =>0x0c047fff8070: fa fa 00[fa]fa fa fd fa fa fa fd fd fa fa fd fd 0x0c047fff8080: fa fa fd fd fa fa 00 00 fa fa 00 fa fa fa fd fa 0x0c047fff8090: fa fa fd fd fa fa 00 00 fa fa fa fa fa fa fa fa 0x0c047fff80a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff80b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff80c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==10888==ABORTING Fix this bug by checking whether `end` points at the trailing NUL byte. Add a test which catches this out-of-bounds read and which demonstrates that we used to write out-of-bounds data into the formatted message. Reported-by: Markus Vervier <markus.vervier@x41-dsec.de> Original-patch-by: Markus Vervier <markus.vervier@x41-dsec.de> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
avar
pushed a commit
that referenced
this pull request
Jan 18, 2023
The return type of both `utf8_strwidth()` and `utf8_strnwidth()` is
`int`, but we operate on string lengths which are typically of type
`size_t`. This means that when the string is longer than `INT_MAX`, we
will overflow and thus return a negative result.
This can lead to an out-of-bounds write with `--pretty=format:%<1)%B`
and a commit message that is 2^31+1 bytes long:
=================================================================
==26009==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000001168 at pc 0x7f95c4e5f427 bp 0x7ffd8541c900 sp 0x7ffd8541c0a8
WRITE of size 2147483649 at 0x603000001168 thread T0
#0 0x7f95c4e5f426 in __interceptor_memcpy /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827
#1 0x5612bbb1068c in format_and_pad_commit pretty.c:1763
#2 0x5612bbb1087a in format_commit_item pretty.c:1801
#3 0x5612bbc33bab in strbuf_expand strbuf.c:429
#4 0x5612bbb110e7 in repo_format_commit_message pretty.c:1869
#5 0x5612bbb12d96 in pretty_print_commit pretty.c:2161
git#6 0x5612bba0a4d5 in show_log log-tree.c:781
#7 0x5612bba0d6c7 in log_tree_commit log-tree.c:1117
git#8 0x5612bb691ed5 in cmd_log_walk_no_free builtin/log.c:508
git#9 0x5612bb69235b in cmd_log_walk builtin/log.c:549
git#10 0x5612bb6951a2 in cmd_log builtin/log.c:883
git#11 0x5612bb56c993 in run_builtin git.c:466
git#12 0x5612bb56d397 in handle_builtin git.c:721
git#13 0x5612bb56db07 in run_argv git.c:788
git#14 0x5612bb56e8a7 in cmd_main git.c:923
git#15 0x5612bb803682 in main common-main.c:57
git#16 0x7f95c4c3c28f (/usr/lib/libc.so.6+0x2328f)
git#17 0x7f95c4c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349)
git#18 0x5612bb5680e4 in _start ../sysdeps/x86_64/start.S:115
0x603000001168 is located 0 bytes to the right of 24-byte region [0x603000001150,0x603000001168)
allocated by thread T0 here:
#0 0x7f95c4ebe7ea in __interceptor_realloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:85
#1 0x5612bbcdd556 in xrealloc wrapper.c:136
#2 0x5612bbc310a3 in strbuf_grow strbuf.c:99
#3 0x5612bbc32acd in strbuf_add strbuf.c:298
#4 0x5612bbc33aec in strbuf_expand strbuf.c:418
#5 0x5612bbb110e7 in repo_format_commit_message pretty.c:1869
git#6 0x5612bbb12d96 in pretty_print_commit pretty.c:2161
#7 0x5612bba0a4d5 in show_log log-tree.c:781
git#8 0x5612bba0d6c7 in log_tree_commit log-tree.c:1117
git#9 0x5612bb691ed5 in cmd_log_walk_no_free builtin/log.c:508
git#10 0x5612bb69235b in cmd_log_walk builtin/log.c:549
git#11 0x5612bb6951a2 in cmd_log builtin/log.c:883
git#12 0x5612bb56c993 in run_builtin git.c:466
git#13 0x5612bb56d397 in handle_builtin git.c:721
git#14 0x5612bb56db07 in run_argv git.c:788
git#15 0x5612bb56e8a7 in cmd_main git.c:923
git#16 0x5612bb803682 in main common-main.c:57
git#17 0x7f95c4c3c28f (/usr/lib/libc.so.6+0x2328f)
SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827 in __interceptor_memcpy
Shadow bytes around the buggy address:
0x0c067fff81d0: fd fd fd fa fa fa fd fd fd fa fa fa fd fd fd fa
0x0c067fff81e0: fa fa fd fd fd fd fa fa fd fd fd fd fa fa fd fd
0x0c067fff81f0: fd fa fa fa fd fd fd fa fa fa fd fd fd fa fa fa
0x0c067fff8200: fd fd fd fa fa fa fd fd fd fd fa fa 00 00 00 fa
0x0c067fff8210: fa fa fd fd fd fa fa fa fd fd fd fa fa fa fd fd
=>0x0c067fff8220: fd fa fa fa fd fd fd fa fa fa 00 00 00[fa]fa fa
0x0c067fff8230: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c067fff8240: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c067fff8250: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c067fff8260: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c067fff8270: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==26009==ABORTING
Now the proper fix for this would be to convert both functions to return
an `size_t` instead of an `int`. But given that this commit may be part
of a security release, let's instead do the minimal viable fix and die
in case we see an overflow.
Add a test that would have previously caused us to crash.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
avar
added a commit
that referenced
this pull request
Jan 23, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Feb 1, 2023
For those tests that have 3 occurances of 'test_i18ngrep', replace it with 'grep'. Splitting this incremental conversion by number of occurances in the file is arbitrary, but help so keep the commit size down. The 'test_i18ngrep' wrapper has been deprecated since d162b25 (tests: remove support for GIT_TEST_GETTEXT_POISON, 2021-01-20). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
avar
added a commit
that referenced
this pull request
Feb 2, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Feb 2, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Feb 3, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Feb 3, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Feb 6, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Feb 6, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Feb 6, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Feb 7, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Feb 7, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Feb 8, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Feb 8, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Feb 10, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Mar 6, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Mar 7, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
Mar 7, 2023
* avar/nuke-test_i18ngrep: tests with git#10 test_i18ngrep: replace it with 'grep' tests with git#9 test_i18ngrep: replace it with 'grep' tests with git#8 test_i18ngrep: replace it with 'grep' tests with #7 test_i18ngrep: replace it with 'grep' tests with git#6 test_i18ngrep: replace it with 'grep' tests with #5 test_i18ngrep: replace it with 'grep' tests with #4 test_i18ngrep: replace it with 'grep' tests with #3 test_i18ngrep: replace it with 'grep' tests with #2 test_i18ngrep: replace it with 'grep' tests with #1 test_i18ngrep: replace it with 'grep'
avar
added a commit
that referenced
this pull request
May 1, 2023
Adjust the code added in 1ff21c0 (config: store "git -c" variables using more robust format, 2021-01-12) to avoid allocating a "key" until after we've done the sanity checks on it. There reason for allocating it this in the function is because the "env_name" pointer was about to be incremented, let's instead note our position at that point. The key length is the number of characters before the first "=". As a result of this early allocation we'd have a memory leak as reported by valgrind, just not the "still reachable" kind we usually care about[1] with SANITIZE=leak: $ valgrind --leak-check=full --show-leak-kinds=all ./git --config-env=foo.flag= config --bool foo.flag [...] ==6540== 13 bytes in 1 blocks are still reachable in loss record 3 of 17 ==6540== at 0x48437B4: malloc (vg_replace_malloc.c:381) ==6540== by 0x40278B: do_xmalloc (wrapper.c:51) ==6540== by 0x402884: do_xmallocz (wrapper.c:85) ==6540== by 0x4028C0: xmallocz (wrapper.c:93) ==6540== by 0x4028FD: xmemdupz (wrapper.c:109) ==6540== by 0x402962: xstrndup (wrapper.c:115) ==6540== by 0x342F53: strip_path_suffix (path.c:1300) ==6540== by 0x2B4C89: system_prefix (exec-cmd.c:50) ==6540== by 0x2B5057: system_path (exec-cmd.c:268) ==6540== by 0x2C5E17: git_setup_gettext (gettext.c:109) ==6540== by 0x21DC57: main (common-main.c:44) Since the "key" was reachable when the "die()" ran the semantics of SANITIZE=leak wouldn't show it, but "clang" at higher optimization levels would optimize this to the point of thinking the variable wasn't reachable, e.g. with Debian clang 14.0.6-2 with CFLAGS="-O3 -g": $ ./git --config-env=foo.flag= config --bool foo.flag fatal: missing environment variable name for configuration 'foo.flag' ================================================================= ==18018==ERROR: LeakSanitizer: detected memory leaks Direct leak of 9 byte(s) in 1 object(s) allocated from: #0 0x55e40aad7cd2 in __interceptor_malloc (/home/avar/g/git/git+0x78cd2) (BuildId: b89fa8f797ccb02ef1148beed300071a5f9b9ab1) #1 0x55e40ad41071 in do_xmalloc /home/avar/g/git/wrapper.c:51:8 #2 0x55e40ad41071 in do_xmallocz /home/avar/g/git/wrapper.c:85:8 #3 0x55e40ad41071 in xmallocz /home/avar/g/git/wrapper.c:93:9 #4 0x55e40ad41071 in xmemdupz /home/avar/g/git/wrapper.c:109:16 #5 0x55e40abec960 in git_config_push_env /home/avar/g/git/config.c:521:8 git#6 0x55e40aadb8b9 in handle_options /home/avar/g/git/git.c:268:4 #7 0x55e40aada68d in cmd_main /home/avar/g/git/git.c:896:2 git#8 0x55e40abae219 in main /home/avar/g/git/common-main.c:57:11 git#9 0x7fbb5287e209 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16 git#10 0x7fbb5287e2bb in __libc_start_main csu/../csu/libc-start.c:389:3 git#11 0x55e40aaac130 in _start (/home/avar/g/git/git+0x4d130) (BuildId: b89fa8f797ccb02ef1148beed300071a5f9b9ab1) SUMMARY: LeakSanitizer: 9 byte(s) leaked in 1 allocation(s). Aborted Whatever clang has to say about it it makes sense to save ourselves the xmemdupz() if we can help it, but it's worth noting why this came up. The actual meaningful fix here is that we don't need to do this allocation at all. The only reason it's needed is because there hasn't been a variant of "sq_quote_buf()" in quote.c that takes a "len" parameter. In previous commits we added one, and will move to using it in the subsequent commit, but let's first make this smaller change. 1. https://lore.kernel.org/git/220218.861r00ib86.gmgdl@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
avar
added a commit
that referenced
this pull request
May 1, 2023
For those tests that have 3 occurances of 'test_i18ngrep', replace it with 'grep'. Splitting this incremental conversion by number of occurances in the file is arbitrary, but help so keep the commit size down. The 'test_i18ngrep' wrapper has been deprecated since d162b25 (tests: remove support for GIT_TEST_GETTEXT_POISON, 2021-01-20). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This might be purely a learning exercise for me and might not have any real value, but I made some changes to the function signature for
parse_cmd.This MR also includes some tests for
--stdin-cmd.@avar let me know what you think of these changes. I was trying to maintain consistency with the way that
update-refhandles its command mode arguments.