git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ramsay Jones <ramsay@ramsayjones.plus.com>
Date
Feb 18, 2018, 22:55 UTC
Message-ID
<58b2bdcd-d621-fd21-ab4d-6a9478319b19@ramsayjones.plus.com>
In-Reply-To
<CA+kUOamJowkxp0xAPf5FA+wBkiOjQeYzW1sKMwcatQBBA1qWpw@mail.gmail.com>
On 16/02/18 14:55, Adam Dinwoodie wrote:
Show 12 quoted lines
> 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

Previous: Adam DinwoodieNext: Ramsay Jones
Message 52 of 56 in “ref-filter: get rid of goto”
  1. 01/23 ref-filter: get rid of gotoOlga Telezhnaya, Feb 12, 2018
  2. 03/23 cat-file: reuse struct ref_formatOlga Telezhnaya, Feb 12, 2018
  3. Jeff KingFeb 15, 2018
  4. 08/23 ref-filter: reuse parse_ref_filter_atom()Olga Telezhnaya, Feb 12, 2018
  5. Jeff KingFeb 15, 2018
  6. Оля ТележнаяFeb 15, 2018
  7. 04/23 ref-filter: make valid_atom as function parameterOlga Telezhnaya, Feb 12, 2018
  8. Jeff KingFeb 15, 2018
  9. Оля ТележнаяFeb 15, 2018
  10. 11/23 ref-filter: rename field in ref_array_item stuctOlga Telezhnaya, Feb 12, 2018
  11. 07/23 cat-file: start migrating formatting to ref-filterOlga Telezhnaya, Feb 12, 2018
  12. Jeff KingFeb 15, 2018
  13. 19/23 ref-filter: make populate_value() internal againOlga Telezhnaya, Feb 12, 2018
  14. 16/23 ref-filter: make cat_file_info independentOlga Telezhnaya, Feb 12, 2018
  15. Jeff KingFeb 15, 2018
  16. Оля ТележнаяFeb 15, 2018
  17. Jeff KingFeb 16, 2018
  18. 20/23 ref-filter: unifying formatting of cat-file optsOlga Telezhnaya, Feb 12, 2018
  19. Jeff KingFeb 15, 2018
  20. Оля ТележнаяFeb 15, 2018
  21. Junio C HamanoFeb 15, 2018
  22. 10/23 ref-filter: make populate_value() globalOlga Telezhnaya, Feb 12, 2018
  23. 15/23 cat-file: move skip_object_info into ref-filterOlga Telezhnaya, Feb 12, 2018
  24. Jeff KingFeb 15, 2018
  25. Оля ТележнаяFeb 15, 2018
  26. 21/23 for-each-ref: tests for new atoms addedOlga Telezhnaya, Feb 12, 2018
  27. Jeff KingFeb 15, 2018
  28. Оля ТележнаяFeb 15, 2018
  29. 02/23 ref-filter: add return value to some functionsOlga Telezhnaya, Feb 12, 2018
  30. Jeff KingFeb 15, 2018
  31. Оля ТележнаяFeb 15, 2018
  32. 09/23 cat-file: start use ref_array_item structOlga Telezhnaya, Feb 12, 2018
  33. Jeff KingFeb 15, 2018
  34. Оля ТележнаяFeb 15, 2018
  35. 18/23 cat-file: reuse printing logic from ref-filterOlga Telezhnaya, Feb 12, 2018
  36. 05/23 cat-file: move struct expand_data into ref-filterOlga Telezhnaya, Feb 12, 2018
  37. Jeff KingFeb 15, 2018
  38. 23/23 cat-file: update of docsOlga Telezhnaya, Feb 12, 2018
  39. Jeff KingFeb 15, 2018
  40. 12/23 cat-file: start reusing populate_value()Olga Telezhnaya, Feb 12, 2018
  41. Jeff KingFeb 15, 2018
  42. 14/23 ref_filter: add is_atom_used functionOlga Telezhnaya, Feb 12, 2018
  43. Jeff KingFeb 15, 2018
  44. Оля ТележнаяFeb 15, 2018
  45. 17/23 ref-filter: make valid_atom general againOlga Telezhnaya, Feb 12, 2018
  46. Jeff KingFeb 15, 2018
  47. 13/23 ref-filter: get rid of mark_atom_in_object_info()Olga Telezhnaya, Feb 12, 2018
  48. Jeff KingFeb 15, 2018
  49. Оля ТележнаяFeb 15, 2018
  50. 22/23 cat-file: tests for new atoms addedOlga Telezhnaya, Feb 12, 2018
  51. Adam DinwoodieFeb 16, 2018
  52. Ramsay JonesFeb 18, 2018
  53. Ramsay JonesFeb 19, 2018
  54. 06/23 cat-file: split expand_atom() into 2 functionsOlga Telezhnaya, Feb 12, 2018
  55. Jeff KingFeb 15, 2018
  56. Jeff KingFeb 15, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.