Rewrite cat-file.c : need help to find a bug

Rewrite cat-file.c : need help to find a bug

[Оля Тележная]

Hi everyone,
I am trying to reuse formatting logic from ref-filter in cat-file
command. Now cat-file uses its own formatting code.
I am trying to achieve that step-by-step, now I want to invoke
populate_value function, and I have a bug somewhere.
My code is here.
https://github.com/telezhnaya/git/commits/catfile
All commits that starts from "cat-file: " are successfully passing all tests.
So for now my last commit fails, and I am tired of searching for the
error. Actually, I just copied some values to another variable and
move some code to other place. All "intelligent" work will go further,
but now I need to fix current situation.

You could write here or leave comments in github if you have any ideas.

Thank you in advance for your help!
Olga

Re: Rewrite cat-file.c : need help to find a bug

[Jeff King]

On Fri, Dec 29, 2017 at 01:05:50PM +0300, Оля Тележная wrote:

> Hi everyone,
> I am trying to reuse formatting logic from ref-filter in cat-file
> command. Now cat-file uses its own formatting code.
> I am trying to achieve that step-by-step, now I want to invoke
> populate_value function, and I have a bug somewhere.
> My code is here.
> https://github.com/telezhnaya/git/commits/catfile
> All commits that starts from "cat-file: " are successfully passing all tests.
> So for now my last commit fails, and I am tired of searching for the
> error. Actually, I just copied some values to another variable and
> move some code to other place. All "intelligent" work will go further,
> but now I need to fix current situation.

The problem is that "cat_file_info" is NULL when you get to
populate_value(), so the moved code doesn't trigger.

That variable is usually pulled from format->cat_file_data during
verify_ref_format(). But it triggers only when there's an actual format
atom to parse, and in this particular test the format is empty! But
cat-file is somewhat special here, in that even without any atoms it
needs to make sure the object exists.

So the patch below makes the test pass, though I think in the long run
all of this cat_file_info stuff would just get folded into regular
atoms, and you'd want cat-file to pass a special flag to ask ref-filter
to make sure the object exists.

-Peff

---
diff --git a/ref-filter.c b/ref-filter.c
index 62ca83807f..3acea4d2ef 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -742,6 +742,10 @@ int verify_ref_format(struct ref_format *format)
 {
 	const char *cp, *sp;
 
+	/* do this unconditionally, even if we have no atoms! */
+	if (format->cat_file_data)
+		cat_file_info = format->cat_file_data;
+
 	format->need_color_reset_at_eol = 0;
 	for (cp = format->format; *cp && (sp = find_next(cp)); ) {
 		const char *color, *ep = strchr(sp, ')');
@@ -753,7 +757,6 @@ int verify_ref_format(struct ref_format *format)
 
 		if (format->cat_file_data)
 		{
-			cat_file_info = format->cat_file_data;
 			at = parse_ref_filter_atom(format, valid_cat_file_atoms,
 						   ARRAY_SIZE(valid_cat_file_atoms), sp + 2, ep);
 		} else

Re: Rewrite cat-file.c : need help to find a bug

[Оля Тележная]

2017-12-29 16:22 GMT+03:00 Jeff King <peff@peff.net>:
> On Fri, Dec 29, 2017 at 01:05:50PM +0300, Оля Тележная wrote:
>
>> Hi everyone,
>> I am trying to reuse formatting logic from ref-filter in cat-file
>> command. Now cat-file uses its own formatting code.
>> I am trying to achieve that step-by-step, now I want to invoke
>> populate_value function, and I have a bug somewhere.
>> My code is here.
>> https://github.com/telezhnaya/git/commits/catfile
>> All commits that starts from "cat-file: " are successfully passing all tests.
>> So for now my last commit fails, and I am tired of searching for the
>> error. Actually, I just copied some values to another variable and
>> move some code to other place. All "intelligent" work will go further,
>> but now I need to fix current situation.
>
> The problem is that "cat_file_info" is NULL when you get to
> populate_value(), so the moved code doesn't trigger.
>

 Thanks a lot!

Re: Rewrite cat-file.c : need help to find a bug

[Оля Тележная]

2017-12-29 17:04 GMT+03:00 Оля Тележная <olyatelezhnaya@gmail.com>:
> 2017-12-29 16:22 GMT+03:00 Jeff King <peff@peff.net>:
>> On Fri, Dec 29, 2017 at 01:05:50PM +0300, Оля Тележная wrote:
>>
>>> Hi everyone,
>>> I am trying to reuse formatting logic from ref-filter in cat-file
>>> command. Now cat-file uses its own formatting code.
>>> I am trying to achieve that step-by-step,

Happy to say that my previous bug is fixed, and I am struggling with
the next one.
Now I want to get rid of using expand_data structure (I took it from
cat-file.c to simplify migrating process and now it's time to delete
it).

>>> My code is here.
>>> https://github.com/telezhnaya/git/commits/catfile
>>> All commits that starts from "cat-file: " are successfully passing all tests.

So for now 2 of my last commits fail, and I am tired of searching for the error.
I was also trying to leave cat_file_info variable and fill in both new
and old variables and then compare resulting values by printing them
into file. Everything is OK, but I find it dudpicious that the
resulting file is too small (fprintf was invoked only 3 times, it was
here: https://github.com/telezhnaya/git/commit/54a5b5a0167ad634c26e4fd7df234a46286ede0a#diff-2846189963e8aec1bcb559b69b7f20d0R1489)

I have left few comments in github to simplify your understanding what
I was trying to achieve. Feel free to ask any questions if you find
the code strange, unclear or suspicious.
Thanks!

Olga

Re: Rewrite cat-file.c : need help to find a bug

[Christian Couder]

On Thu, Jan 4, 2018 at 11:23 PM, Оля Тележная <olyatelezhnaya@gmail.com> wrote:
>
> So for now 2 of my last commits fail, and I am tired of searching for the error.
> I was also trying to leave cat_file_info variable and fill in both new
> and old variables and then compare resulting values by printing them
> into file. Everything is OK, but I find it dudpicious that the
> resulting file is too small (fprintf was invoked only 3 times, it was
> here: https://github.com/telezhnaya/git/commit/54a5b5a0167ad634c26e4fd7df234a46286ede0a#diff-2846189963e8aec1bcb559b69b7f20d0R1489)
>
> I have left few comments in github to simplify your understanding what
> I was trying to achieve. Feel free to ask any questions if you find
> the code strange, unclear or suspicious.

Let me try to see how I can debug it.

Running `./t1006-cat-file.sh -v -i` gives:

---------------
expecting success:
        maybe_remove_timestamp "$batch_output" $no_ts >expect &&
        maybe_remove_timestamp "$(echo $sha1 | git cat-file --batch)"
$no_ts >actual &&
        test_cmp expect actual

Segmentation fault (core dumped)
--- expect      2018-01-04 23:31:20.515114634 +0000
+++ actual      2018-01-04 23:31:20.635114274 +0000
@@ -1,2 +0,0 @@
-5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689 blob 11
-Hello World
\ No newline at end of file
not ok 9 - --batch output of blob is correct
#
#               maybe_remove_timestamp "$batch_output" $no_ts >expect &&
#               maybe_remove_timestamp "$(echo $sha1 | git cat-file
--batch)" $no_ts >actual &&
#               test_cmp expect actual
#
---------------

So there is a segfault probably when running $(echo $sha1 | git
cat-file --batch). Let's try to run that manually.

$ cd trash\ directory.t1006-cat-file/
$  echo 5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689 | git cat-file --batch
Segmentation fault (core dumped)

That's it. Now let's use gdb to see where it comes from:

$ echo 5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689 > myarg.txt
$ gdb git
GNU gdb (Ubuntu 8.0.1-0ubuntu1) 8.0.1
Copyright (C) 2017 Free Software Foundation, Inc.
...
(gdb)

Let's run the cat-file command inside gdb:

(gdb) run cat-file --batch < myarg.txt
Starting program: /home/ubuntu/bin/git cat-file --batch < myarg.txt
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
0x00005555556e88e6 in populate_value (ref=0x7fffffffd430) at ref-filter.c:1496
1496                    ref->disk_size = *obj_info.disk_sizep;
(gdb)

Let's get a backtrace:

(gdb)  bt
#0  0x00005555556e88e6 in populate_value (ref=0x7fffffffd430) at
ref-filter.c:1496
#1  0x00005555555783f1 in batch_object_write (
    obj_name=0x555555a655f0
"5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689", opt=0x7fffffffd6e0,
    data=0x7fffffffd5e0) at builtin/cat-file.c:291
#2  0x0000555555578660 in batch_one_object (
    obj_name=0x555555a655f0
"5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689", opt=0x7fffffffd6e0,
    data=0x7fffffffd5e0) at builtin/cat-file.c:346

Let's see what's the code that makes it segfault:

(gdb) l
1491                            fflush(stdout);
1492                            return -1;
1493                    }
1494                    ref->type = *obj_info.typep;
1495                    ref->size = *obj_info.sizep;
1496                    ref->disk_size = *obj_info.disk_sizep;
1497                    hashcpy(ref->delta_base_oid.hash,
obj_info.delta_base_sha1);
1498            }
1499
1500            /* Fill in specials first */

Line 1496 has "ref->disk_size = *obj_info.disk_sizep;" so let's look
at those variables:

(gdb) p *ref
$1 = {objectname = {hash =
"^\034\060\235\256\177E\340\363\233\033\363\254<\331\333\022\347\326\211"},
  flag = 0, kind = 4148386208, symref = 0x7ffff778b9e0
<_IO_2_1_stdin_> "\210 \255\373",
  commit = 0x7fffffffd510, values = 0x555555a66cb0, type = OBJ_BLOB, size = 11,
  disk_size = -7613955248136140544, rest = 0x0, delta_base_oid = {
    hash = "-\334qUUU\000\000\360\324\377\377\377\177\000\000\340\325\377\377"},
  start_of_request = 0x555555a655f0 "5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689",
  refname = 0x7fffffffd4a8 ""}
(gdb)  p obj_info
$2 = {typep = 0x555555a53df8 <o_type>, sizep = 0x555555a66c30,
disk_sizep = 0x0, delta_base_sha1 = 0x0,
  typename = 0x0, contentp = 0x0, whence = OI_LOOSE, u = {packed =
{pack = 0x0, offset = 0,
      is_delta = 0}}}

Ok we can see that "disk_sizep = 0x0" which means that it segfault
because line 1496 tries to read the value pointed to by disk_sizep
which is NULL.

I hope this will help you.

Best,
Christian.