# Rewrite cat-file.c : need help to find a bug 5 messages from 2017-12-29 to 2018-01-04. Participants: Оля Тележная, Jeff King, Christian Couder. Thread: https://gitlist.dev/t/47509 ## Оля Тележная, 2017-12-29 10:05 Subject: Rewrite cat-file.c : need help to find a bug Message-ID: URL: https://gitlist.dev/e/CAL21BmnycG4%3DWm_e1S85QVPfs3vV_Q%3DTAjTAG-sv%2Bf2mK6wbBQ%40mail.gmail.com ``` 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 ``` ## Jeff King, 2017-12-29 13:22 Subject: Re: Rewrite cat-file.c : need help to find a bug Message-ID: <20171229132237.GA12561@sigill.intra.peff.net> URL: https://gitlist.dev/e/20171229132237.GA12561%40sigill.intra.peff.net In-Reply-To: ``` 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 ``` ## Оля Тележная, 2017-12-29 14:04 Subject: Re: Rewrite cat-file.c : need help to find a bug Message-ID: URL: https://gitlist.dev/e/CAL21BmkcDK%2BVRc2CBTKnV5eubLh8u37THYjXhuTgFKR4Z%3DVtTA%40mail.gmail.com In-Reply-To: <20171229132237.GA12561@sigill.intra.peff.net> ``` 2017-12-29 16:22 GMT+03:00 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. > Thanks a lot! ``` ## Оля Тележная, 2018-01-04 22:23 Subject: Re: Rewrite cat-file.c : need help to find a bug Message-ID: URL: https://gitlist.dev/e/CAL21BmkMrpEoijmMJ%3D2Y330shxABerc4tCBRpM2d4h0DypbYqA%40mail.gmail.com In-Reply-To: ``` 2017-12-29 17:04 GMT+03:00 Оля Тележная : > 2017-12-29 16:22 GMT+03:00 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, 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 ``` ## Christian Couder, 2018-01-04 23:57 Subject: Re: Rewrite cat-file.c : need help to find a bug Message-ID: URL: https://gitlist.dev/e/CAP8UFD3Bd4Af1XZ00VyuHnQs%3DMFrdUufKeePO1tyedWoReRjwQ%40mail.gmail.com In-Reply-To: ``` On Thu, Jan 4, 2018 at 11:23 PM, Оля Тележная 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 , 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. ```