On 16/02/18 14:55, Adam Dinwoodie wrote:

On 12 February 2018 at 08:08, Olga Telezhnaya wrote:

quoted

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