Re: [PATCH v3 22/23] cat-file: tests for new atoms added
On 18/02/18 22:55, Ramsay Jones wrote:
>
>
> 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:
BTW, I forgot to mention that, if you run that test without -i, then
more than tests #15,36,58 and 89 will fail. In addition, tests #19-20,
#40-41, #62-63 and #93-94 failed - I did not investigate these
failures at all.
ATB,
Ramsay Jones
Re: [PATCH v3 22/23] cat-file: tests for new atoms added
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
Re: [PATCH v3 22/23] cat-file: tests for new atoms added
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.
I'm afraid I'm not going to have the time to investigate the failure
any further in the immediate future, but I wanted to report it
promptly in case you / someone else can see what's going wrong.
Adam
