{"thread":{"id":"47509","subject":"Rewrite cat-file.c : need help to find a bug","startedAt":"2017-12-29T10:05:59Z","lastAt":"2018-01-04T23:58:05Z","messageCount":5,"participants":["Оля Тележная","Jeff King","Christian Couder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"335521","messageId":"CAL21BmnycG4=Wm_e1S85QVPfs3vV_Q=TAjTAG-sv+f2mK6wbBQ@mail.gmail.com","threadId":"47509","inReplyTo":null,"subject":"Rewrite cat-file.c : need help to find a bug","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2017-12-29T10:05:50Z","receivedAt":"2017-12-29T10:05:59Z","isPatch":false,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Hi everyone,\nI am trying to reuse formatting logic from ref-filter in cat-file\ncommand. Now cat-file uses its own formatting code.\nI am trying to achieve that step-by-step, now I want to invoke\npopulate_value function, and I have a bug somewhere.\nMy code is here.\nhttps://github.com/telezhnaya/git/commits/catfile\nAll commits that starts from \"cat-file: \" are successfully passing all tests.\nSo for now my last commit fails, and I am tired of searching for the\nerror. Actually, I just copied some values to another variable and\nmove some code to other place. All \"intelligent\" work will go further,\nbut now I need to fix current situation.\n\nYou could write here or leave comments in github if you have any ideas.\n\nThank you in advance for your help!\nOlga\n"},{"id":"335523","messageId":"20171229132237.GA12561@sigill.intra.peff.net","threadId":"47509","inReplyTo":"CAL21BmnycG4=Wm_e1S85QVPfs3vV_Q=TAjTAG-sv+f2mK6wbBQ@mail.gmail.com","subject":"Re: Rewrite cat-file.c : need help to find a bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-29T13:22:37Z","receivedAt":"2017-12-29T13:22:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 29, 2017 at 01:05:50PM +0300, Оля Тележная wrote:\n\n> Hi everyone,\n> I am trying to reuse formatting logic from ref-filter in cat-file\n> command. Now cat-file uses its own formatting code.\n> I am trying to achieve that step-by-step, now I want to invoke\n> populate_value function, and I have a bug somewhere.\n> My code is here.\n> https://github.com/telezhnaya/git/commits/catfile\n> All commits that starts from \"cat-file: \" are successfully passing all tests.\n> So for now my last commit fails, and I am tired of searching for the\n> error. Actually, I just copied some values to another variable and\n> move some code to other place. All \"intelligent\" work will go further,\n> but now I need to fix current situation.\n\nThe problem is that \"cat_file_info\" is NULL when you get to\npopulate_value(), so the moved code doesn't trigger.\n\nThat variable is usually pulled from format->cat_file_data during\nverify_ref_format(). But it triggers only when there's an actual format\natom to parse, and in this particular test the format is empty! But\ncat-file is somewhat special here, in that even without any atoms it\nneeds to make sure the object exists.\n\nSo the patch below makes the test pass, though I think in the long run\nall of this cat_file_info stuff would just get folded into regular\natoms, and you'd want cat-file to pass a special flag to ask ref-filter\nto make sure the object exists.\n\n-Peff\n\n---\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 62ca83807f..3acea4d2ef 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -742,6 +742,10 @@ int verify_ref_format(struct ref_format *format)\n {\n \tconst char *cp, *sp;\n \n+\t/* do this unconditionally, even if we have no atoms! */\n+\tif (format->cat_file_data)\n+\t\tcat_file_info = format->cat_file_data;\n+\n \tformat->need_color_reset_at_eol = 0;\n \tfor (cp = format->format; *cp && (sp = find_next(cp)); ) {\n \t\tconst char *color, *ep = strchr(sp, ')');\n@@ -753,7 +757,6 @@ int verify_ref_format(struct ref_format *format)\n \n \t\tif (format->cat_file_data)\n \t\t{\n-\t\t\tcat_file_info = format->cat_file_data;\n \t\t\tat = parse_ref_filter_atom(format, valid_cat_file_atoms,\n \t\t\t\t\t\t   ARRAY_SIZE(valid_cat_file_atoms), sp + 2, ep);\n \t\t} else\n"},{"id":"335529","messageId":"CAL21BmkcDK+VRc2CBTKnV5eubLh8u37THYjXhuTgFKR4Z=VtTA@mail.gmail.com","threadId":"47509","inReplyTo":"20171229132237.GA12561@sigill.intra.peff.net","subject":"Re: Rewrite cat-file.c : need help to find a bug","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2017-12-29T14:04:03Z","receivedAt":"2017-12-29T14:04:11Z","isPatch":false,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"2017-12-29 16:22 GMT+03:00 Jeff King <peff@peff.net>:\n> On Fri, Dec 29, 2017 at 01:05:50PM +0300, Оля Тележная wrote:\n>\n>> Hi everyone,\n>> I am trying to reuse formatting logic from ref-filter in cat-file\n>> command. Now cat-file uses its own formatting code.\n>> I am trying to achieve that step-by-step, now I want to invoke\n>> populate_value function, and I have a bug somewhere.\n>> My code is here.\n>> https://github.com/telezhnaya/git/commits/catfile\n>> All commits that starts from \"cat-file: \" are successfully passing all tests.\n>> So for now my last commit fails, and I am tired of searching for the\n>> error. Actually, I just copied some values to another variable and\n>> move some code to other place. All \"intelligent\" work will go further,\n>> but now I need to fix current situation.\n>\n> The problem is that \"cat_file_info\" is NULL when you get to\n> populate_value(), so the moved code doesn't trigger.\n>\n\n Thanks a lot!\n"},{"id":"335875","messageId":"CAL21BmkMrpEoijmMJ=2Y330shxABerc4tCBRpM2d4h0DypbYqA@mail.gmail.com","threadId":"47509","inReplyTo":"CAL21BmkcDK+VRc2CBTKnV5eubLh8u37THYjXhuTgFKR4Z=VtTA@mail.gmail.com","subject":"Re: Rewrite cat-file.c : need help to find a bug","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-01-04T22:23:22Z","receivedAt":"2018-01-04T22:23:27Z","isPatch":false,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"2017-12-29 17:04 GMT+03:00 Оля Тележная <olyatelezhnaya@gmail.com>:\n> 2017-12-29 16:22 GMT+03:00 Jeff King <peff@peff.net>:\n>> On Fri, Dec 29, 2017 at 01:05:50PM +0300, Оля Тележная wrote:\n>>\n>>> Hi everyone,\n>>> I am trying to reuse formatting logic from ref-filter in cat-file\n>>> command. Now cat-file uses its own formatting code.\n>>> I am trying to achieve that step-by-step,\n\nHappy to say that my previous bug is fixed, and I am struggling with\nthe next one.\nNow I want to get rid of using expand_data structure (I took it from\ncat-file.c to simplify migrating process and now it's time to delete\nit).\n\n>>> My code is here.\n>>> https://github.com/telezhnaya/git/commits/catfile\n>>> All commits that starts from \"cat-file: \" are successfully passing all tests.\n\nSo for now 2 of my last commits fail, and I am tired of searching for the error.\nI was also trying to leave cat_file_info variable and fill in both new\nand old variables and then compare resulting values by printing them\ninto file. Everything is OK, but I find it dudpicious that the\nresulting file is too small (fprintf was invoked only 3 times, it was\nhere: https://github.com/telezhnaya/git/commit/54a5b5a0167ad634c26e4fd7df234a46286ede0a#diff-2846189963e8aec1bcb559b69b7f20d0R1489)\n\nI have left few comments in github to simplify your understanding what\nI was trying to achieve. Feel free to ask any questions if you find\nthe code strange, unclear or suspicious.\nThanks!\n\nOlga\n"},{"id":"335897","messageId":"CAP8UFD3Bd4Af1XZ00VyuHnQs=MFrdUufKeePO1tyedWoReRjwQ@mail.gmail.com","threadId":"47509","inReplyTo":"CAL21BmkMrpEoijmMJ=2Y330shxABerc4tCBRpM2d4h0DypbYqA@mail.gmail.com","subject":"Re: Rewrite cat-file.c : need help to find a bug","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-01-04T23:57:59Z","receivedAt":"2018-01-04T23:58:05Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Jan 4, 2018 at 11:23 PM, Оля Тележная <olyatelezhnaya@gmail.com> wrote:\n>\n> So for now 2 of my last commits fail, and I am tired of searching for the error.\n> I was also trying to leave cat_file_info variable and fill in both new\n> and old variables and then compare resulting values by printing them\n> into file. Everything is OK, but I find it dudpicious that the\n> resulting file is too small (fprintf was invoked only 3 times, it was\n> here: https://github.com/telezhnaya/git/commit/54a5b5a0167ad634c26e4fd7df234a46286ede0a#diff-2846189963e8aec1bcb559b69b7f20d0R1489)\n>\n> I have left few comments in github to simplify your understanding what\n> I was trying to achieve. Feel free to ask any questions if you find\n> the code strange, unclear or suspicious.\n\nLet me try to see how I can debug it.\n\nRunning `./t1006-cat-file.sh -v -i` gives:\n\n---------------\nexpecting success:\n        maybe_remove_timestamp \"$batch_output\" $no_ts >expect &&\n        maybe_remove_timestamp \"$(echo $sha1 | git cat-file --batch)\"\n$no_ts >actual &&\n        test_cmp expect actual\n\nSegmentation fault (core dumped)\n--- expect      2018-01-04 23:31:20.515114634 +0000\n+++ actual      2018-01-04 23:31:20.635114274 +0000\n@@ -1,2 +0,0 @@\n-5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689 blob 11\n-Hello World\n\\ No newline at end of file\nnot ok 9 - --batch output of blob is correct\n#\n#               maybe_remove_timestamp \"$batch_output\" $no_ts >expect &&\n#               maybe_remove_timestamp \"$(echo $sha1 | git cat-file\n--batch)\" $no_ts >actual &&\n#               test_cmp expect actual\n#\n---------------\n\nSo there is a segfault probably when running $(echo $sha1 | git\ncat-file --batch). Let's try to run that manually.\n\n$ cd trash\\ directory.t1006-cat-file/\n$  echo 5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689 | git cat-file --batch\nSegmentation fault (core dumped)\n\nThat's it. Now let's use gdb to see where it comes from:\n\n$ echo 5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689 > myarg.txt\n$ gdb git\nGNU gdb (Ubuntu 8.0.1-0ubuntu1) 8.0.1\nCopyright (C) 2017 Free Software Foundation, Inc.\n...\n(gdb)\n\nLet's run the cat-file command inside gdb:\n\n(gdb) run cat-file --batch < myarg.txt\nStarting program: /home/ubuntu/bin/git cat-file --batch < myarg.txt\n[Thread debugging using libthread_db enabled]\nUsing host libthread_db library \"/lib/x86_64-linux-gnu/libthread_db.so.1\".\n\nProgram received signal SIGSEGV, Segmentation fault.\n0x00005555556e88e6 in populate_value (ref=0x7fffffffd430) at ref-filter.c:1496\n1496                    ref->disk_size = *obj_info.disk_sizep;\n(gdb)\n\nLet's get a backtrace:\n\n(gdb)  bt\n#0  0x00005555556e88e6 in populate_value (ref=0x7fffffffd430) at\nref-filter.c:1496\n#1  0x00005555555783f1 in batch_object_write (\n    obj_name=0x555555a655f0\n\"5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689\", opt=0x7fffffffd6e0,\n    data=0x7fffffffd5e0) at builtin/cat-file.c:291\n#2  0x0000555555578660 in batch_one_object (\n    obj_name=0x555555a655f0\n\"5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689\", opt=0x7fffffffd6e0,\n    data=0x7fffffffd5e0) at builtin/cat-file.c:346\n\nLet's see what's the code that makes it segfault:\n\n(gdb) l\n1491                            fflush(stdout);\n1492                            return -1;\n1493                    }\n1494                    ref->type = *obj_info.typep;\n1495                    ref->size = *obj_info.sizep;\n1496                    ref->disk_size = *obj_info.disk_sizep;\n1497                    hashcpy(ref->delta_base_oid.hash,\nobj_info.delta_base_sha1);\n1498            }\n1499\n1500            /* Fill in specials first */\n\nLine 1496 has \"ref->disk_size = *obj_info.disk_sizep;\" so let's look\nat those variables:\n\n(gdb) p *ref\n$1 = {objectname = {hash =\n\"^\\034\\060\\235\\256\\177E\\340\\363\\233\\033\\363\\254<\\331\\333\\022\\347\\326\\211\"},\n  flag = 0, kind = 4148386208, symref = 0x7ffff778b9e0\n<_IO_2_1_stdin_> \"\\210 \\255\\373\",\n  commit = 0x7fffffffd510, values = 0x555555a66cb0, type = OBJ_BLOB, size = 11,\n  disk_size = -7613955248136140544, rest = 0x0, delta_base_oid = {\n    hash = \"-\\334qUUU\\000\\000\\360\\324\\377\\377\\377\\177\\000\\000\\340\\325\\377\\377\"},\n  start_of_request = 0x555555a655f0 \"5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689\",\n  refname = 0x7fffffffd4a8 \"\"}\n(gdb)  p obj_info\n$2 = {typep = 0x555555a53df8 <o_type>, sizep = 0x555555a66c30,\ndisk_sizep = 0x0, delta_base_sha1 = 0x0,\n  typename = 0x0, contentp = 0x0, whence = OI_LOOSE, u = {packed =\n{pack = 0x0, offset = 0,\n      is_delta = 0}}}\n\nOk we can see that \"disk_sizep = 0x0\" which means that it segfault\nbecause line 1496 tries to read the value pointed to by disk_sizep\nwhich is NULL.\n\nI hope this will help you.\n\nBest,\nChristian.\n"}]}