On 16/02/18 14:55, Adam Dinwoodie wrote:
> On 12 February 2018 at 08:08, Olga Telezhnaya wrote:
>> Add some tests for new formatting atoms from ref-filter.
>> Some of new atoms are supported automatically,
>> some of them are expanded into empty string
>> (because they are useless for some types of objects),
>> some of them could be supported later in other patches.
>
> This commit appears to be introducing failures in t1006 on Cygwin.
> Specifically, tests 15, 36, 58 and 89, all titled "Check %(refname)
> gives empty output", are failing on the pu branch at 21937aad4, and
> git bisect identifies this commit, 3c1571744 ("cat-file: tests for new
> atoms added", 2018-02-12), as the culprit.
Hi Adam, Olga,
Sparse has been complaining about this 'ot/cat-batch-format' branch for
a while, so I had already noted (apart from a symbol which should be
marked as static) something that looked somewhat off - namely, the on
stack initialisation of a 'struct ref_array_item' (builtin/cat-file.c,
line 246, in function batch_object_write()).
In particular, the 'struct ref_array_item' ends with a [FLEX_ARRAY] field
for the refname, so it clearly isn't meant to be declared on the stack.
The 'ref-filter' code uses a 'constructor' function called 'new_ref_array_\
item() which, among others, takes a 'refname' parameter with which to
initialise the refname FLEX_ARRAY field.
So, on Linux, if you build with SANITIZE=address and then run that test:
$ ./t1006-cat-file.sh -i -v
Initialized empty Git repository in /home/ramsay/git/t/trash directory.t1006-cat-file/.git/
expecting success:
echo_without_newline "$hello_content" > hello &&
git update-index --add hello
ok 1 - setup
...
=================================================================
==12556==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff132f04a8 at pc 0x7f1c1b3ca20b bp 0x7fff132f00f0 sp 0x7fff132ef898
READ of size 4 at 0x7fff132f04a8 thread T0
#0 0x7f1c1b3ca20a in __interceptor_strlen (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x7020a)
#1 0x6798cc in strbuf_addstr /home/ramsay/git/strbuf.h:273
#2 0x6798cc in quote_formatting /home/ramsay/git/ref-filter.c:496
#3 0x68325d in format_ref_array_item /home/ramsay/git/ref-filter.c:2238
#4 0x68358a in show_ref_array_item /home/ramsay/git/ref-filter.c:2262
#5 0x429866 in batch_object_write builtin/cat-file.c:252
#6 0x42b095 in batch_one_object builtin/cat-file.c:306
#7 0x42b095 in batch_objects builtin/cat-file.c:407
#8 0x42b095 in cmd_cat_file builtin/cat-file.c:528
#9 0x40d3cb in run_builtin /home/ramsay/git/git.c:346
#10 0x40d3cb in handle_builtin /home/ramsay/git/git.c:556
#11 0x40f8ae in run_argv /home/ramsay/git/git.c:608
#12 0x40f8ae in cmd_main /home/ramsay/git/git.c:685
#13 0x40b667 in main /home/ramsay/git/common-main.c:43
#14 0x7f1c1ab7982f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
#15 0x40ce38 in _start (/home/ramsay/git/git+0x40ce38)
Address 0x7fff132f04a8 is located in stack of thread T0 at offset 328 in frame
#0 0x4296ff in batch_object_write builtin/cat-file.c:245
This frame has 4 object(s):
[32, 36) 'type'
[96, 104) 'contents'
[160, 168) 'size'
[224, 328) 'item' <== Memory access at offset 328 overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
(longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow ??:0 __interceptor_strlen
Shadow bytes around the buggy address:
0x100062656040: 00 00 f3 f3 f3 f3 f3 f3 f3 f3 00 00 00 00 00 00
0x100062656050: 00 00 00 00 f1 f1 f1 f1 00 00 00 f4 f3 f3 f3 f3
0x100062656060: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
0x100062656070: 04 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4 f2 f2 f2 f2
0x100062656080: 00 f4 f4 f4 f2 f2 f2 f2 00 00 00 00 00 00 00 00
=>0x100062656090: 00 00 00 00 00[f4]f4 f4 f3 f3 f3 f3 00 00 00 00
0x1000626560a0: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
0x1000626560b0: 04 f4 f4 f4 f2 f2 f2 f2 04 f4 f4 f4 f2 f2 f2 f2
0x1000626560c0: 04 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4 f2 f2 f2 f2
0x1000626560d0: 00 f4 f4 f4 f2 f2 f2 f2 00 00 f4 f4 f2 f2 f2 f2
0x1000626560e0: 00 00 00 f4 f2 f2 f2 f2 00 00 00 f4 f2 f2 f2 f2
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
Heap right redzone: fb
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack partial redzone: f4
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
==12556==ABORTING
Aborted (core dumped)
not ok 15 - Check %(refname) gives empty output
#
# echo "$expected" >expect &&
# echo $sha1 | git cat-file --batch-check="$atoms" >actual &&
# test_cmp expect actual
#
$
... you get a stack-overflow at that very stackframe. Incidentally, the
output from the failed test #15 on cygwin always looked the same:
$ xxd actual
00000000: a0c4 ffff 0a .....
$
So, just as an experiment, I tried changing that variable to a heap
variable and initialising it with an empty ("") refname:
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index c9d83ceff..12a743034 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -243,19 +243,20 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
static void batch_object_write(const char *obj_name, struct batch_options *opt,
struct expand_data *data)
{
- struct ref_array_item item = {0};
+ struct ref_array_item *item;
- item.oid = data->oid;
- item.rest = data->rest;
- item.objectname = obj_name;
+ FLEX_ALLOC_STR(item, refname, "");
+ item->oid = data->oid;
+ item->rest = data->rest;
+ item->objectname = obj_name;
- if (show_ref_array_item(&item, &opt->format))
+ if (show_ref_array_item(item, &opt->format))
return;
if (!opt->buffer_output)
fflush(stdout);
if (opt->print_contents) {
- data->type = item.type;
+ data->type = item->type;
print_object_or_die(opt, data);
batch_write(opt, "\n", 1);
}
--
... and now this test passes on cygwin (and the SANITIZE build on Linux).
Of course, this is not a real fix, since this has probably only changed
the stack-overflow into an un-diagnosed heap-overflow bug! ;-)
However, the above should provide enough info for someone more familiar
with the code to implement a correct fix.
[BTW, the symbol that should be marked static is: cat_file_info, in file
ref-filter.c, line 103.]
ATB,
Ramsay Jones
