Re: [PATCH v3 22/23] cat-file: tests for new atoms added

[Ramsay Jones <ramsay@ramsayjones.plus.com>]

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