{"thread":{"id":"46414","subject":"Fwd: New Defects reported by Coverity Scan for git","startedAt":"2017-07-18T16:59:29Z","lastAt":"2017-07-20T21:52:48Z","messageCount":8,"participants":["Stefan Beller","Junio C Hamano","Brandon Williams","René Scharfe"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"324704","messageId":"CAGZ79kaPFDMn2K0f529-Crzv+vhU3XUMsSM6w3QV4RXKBv5s_g@mail.gmail.com","threadId":"46414","inReplyTo":"596ddaa620821_77f83e7330107c4@ss1435.mail","subject":"Fwd: New Defects reported by Coverity Scan for git","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-18T16:59:20Z","receivedAt":"2017-07-18T16:59:29Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"I looked at this report for a while. My current understanding:\n* its detection was triggered by including rs/move-array,\n f331ab9d4c (use MOVE_ARRAY, 2017-07-15)\n* But it is harmless, because the scan logic does not understand\n how ALLOC_GROW works. It assumes that\n done_pbase_paths_alloc can be larger\n than done_pbase_paths_num + 1, while done_pbase_paths\n is NULL, such that the memory allocation is not triggered.\n If that were the case, then we have 2 subsequent dereferences\n of a NULL pointer right after that. But by inspecting the use\n of _alloc and _num the initial assumption does not seem possible.\n\nStefan\n\n---------- Forwarded message ----------\nFrom: \nDate: Tue, Jul 18, 2017 at 2:53 AM\nSubject: New Defects reported by Coverity Scan for git\nTo: sbeller@google.com\n\n\n\nHi,\n\nPlease find the latest report on new defect(s) introduced to git found\nwith Coverity Scan.\n\n2 new defect(s) introduced to git found with Coverity Scan.\n1 defect(s), reported by Coverity Scan earlier, were marked fixed in\nthe recent build analyzed by Coverity Scan.\n\nNew defect(s) Reported-by: Coverity Scan\nShowing 2 of 2 defect(s)\n\n\n** CID 1415508: Null pointer dereferences (FORWARD_NULL)\n/builtin/pack-objects.c: 1292 in check_pbase_path()\n\n\n________________________________________________________________________________________________________\n*** CID 1415508: Null pointer dereferences (FORWARD_NULL)\n/builtin/pack-objects.c: 1292 in check_pbase_path()\n1286 }\n1287 return -lo-1;\n1288 }\n1289\n1290 static int check_pbase_path(unsigned hash)\n1291 {\n>>> CID 1415508: Null pointer dereferences (FORWARD_NULL)\n>>> Comparing \"done_pbase_paths\" to null implies that \"done_pbase_paths\" might be null.\n1292 int pos = (!done_pbase_paths) ? -1 : done_pbase_path_pos(hash);\n1293 if (0 <= pos)\n1294 return 1;\n1295 pos = -pos - 1;\n1296 ALLOC_GROW(done_pbase_paths,\n1297 done_pbase_paths_num + 1,\n\n** CID 1415507: Null pointer dereferences (FORWARD_NULL)\n/builtin/pack-objects.c: 1303 in check_pbase_path()\n\n\n________________________________________________________________________________________________________\n*** CID 1415507: Null pointer dereferences (FORWARD_NULL)\n/builtin/pack-objects.c: 1303 in check_pbase_path()\n1297 done_pbase_paths_num + 1,\n1298 done_pbase_paths_alloc);\n1299 done_pbase_paths_num++;\n1300 if (pos < done_pbase_paths_num)\n1301 MOVE_ARRAY(done_pbase_paths + pos + 1,\ndone_pbase_paths + pos,\n1302 done_pbase_paths_num - pos - 1);\n>>> CID 1415507: Null pointer dereferences (FORWARD_NULL)\n>>> Dereferencing null pointer \"done_pbase_paths\".\n1303 done_pbase_paths[pos] = hash;\n1304 return 0;\n1305 }\n1306\n1307 static void add_preferred_base_object(const char *name)\n1308 {\n\n\n________________________________________________________________________________________________________\nTo view the defects in Coverity Scan visit,\nhttps://u2389337.ct.sendgrid.net/wf/click?upn=08onrYu34A-2BWcWUl-2F-2BfV0V05UPxvVjWch-2Bd2MGckcRb8HAP5hlBzHe8sORKm64S-2F81GsNbRdSrOteP-2FXoviMkw-3D-3D_PwiGIFugGOKqSZ6DZhASdI2SvWKInry4kHBXrJUc9pnRRRwN8fPiR-2BR4LTK2qB-2F8DwbeZJjY7Zg2FBdb8jgiAk7m6rh1YdNCvPYCPUewgRcPRcmkOFDltPB2GLYjg5Pl86kCKSRkx6inI-2BuknVr53Cjba4HgtlWmCuW5A0WMiIFvSKDW3-2BKYfPjiZDMCOFSGSLivQrUyaTeOHAHjl-2FNvbw-3D-3D\n\nTo manage Coverity Scan email notifications for \"sbeller@google.com\",\nclick https://u2389337.ct.sendgrid.net/wf/click?upn=08onrYu34A-2BWcWUl-2F-2BfV0V05UPxvVjWch-2Bd2MGckcRbVDbis712qZDP-2FA8y06Nq4rtNFBzV5kav4CghkcEfRxSYnY6rsKHvgCYp1ThXvyV0VWbGuKIDENjx2sj6ivdYZu-2BNbJM6lgB1oY5D28iuW580xRVIt7xUSma4mf0o8-2BwE-3D_PwiGIFugGOKqSZ6DZhASdI2SvWKInry4kHBXrJUc9pnRRRwN8fPiR-2BR4LTK2qB-2F8ec7P8LTccgviKTLC0eUY7vUYOHaxCJX7GTQpS8ooD-2BtrxVu-2BilxPyHEoqsJLDaUcr6ObouH5nHR8K0ccYTKk6yC1yT-2BgMwWml4OIILno46DqjVrTy1kpeg4B-2BRv4QBTs54v6KZ4s-2FPtTLU3-2BsF7qgg-3D-3D\n"},{"id":"324705","messageId":"xmqqa841fylf.fsf@gitster.mtv.corp.google.com","threadId":"46414","inReplyTo":"CAGZ79kaPFDMn2K0f529-Crzv+vhU3XUMsSM6w3QV4RXKBv5s_g@mail.gmail.com","subject":"Re: Fwd: New Defects reported by Coverity Scan for git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-18T17:23:40Z","receivedAt":"2017-07-18T17:23:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> I looked at this report for a while. My current understanding:\n> * its detection was triggered by including rs/move-array,\n> f331ab9d4c (use MOVE_ARRAY, 2017-07-15)\n> * But it is harmless, because the scan logic does not understand\n> how ALLOC_GROW works. It assumes that\n> done_pbase_paths_alloc can be larger\n> than done_pbase_paths_num + 1, while done_pbase_paths\n> is NULL, such that the memory allocation is not triggered.\n> If that were the case, then we have 2 subsequent dereferences\n> of a NULL pointer right after that. But by inspecting the use\n> of _alloc and _num the initial assumption does not seem possible.\n\nYes, it does appear that way. ALLOC_GROW() calls REALLOC_ARRAY()\nwhich safely can realloc NULL to specified size via xrealloc().\n\nI'd be more worried about segfault we seem to be getting only on\nWindows from this:\n\n git -C parent grep -e \"(1|2)d(3|4)\" --recurse-submodules HEAD^ > actual\n\nin https://travis-ci.org/git/git/jobs/254654195 by the way.\n\n\n"},{"id":"324708","messageId":"CAGZ79kYohev8BdBEL9qnfmZ6quMwZPDRqZ44qCKV_wMwR=Etiw@mail.gmail.com","threadId":"46414","inReplyTo":"xmqqa841fylf.fsf@gitster.mtv.corp.google.com","subject":"Re: Fwd: New Defects reported by Coverity Scan for git","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-18T17:41:21Z","receivedAt":"2017-07-18T17:41:27Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jul 18, 2017 at 10:23 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> I looked at this report for a while. My current understanding:\n>> * its detection was triggered by including rs/move-array,\n>> f331ab9d4c (use MOVE_ARRAY, 2017-07-15)\n>> * But it is harmless, because the scan logic does not understand\n>> how ALLOC_GROW works. It assumes that\n>> done_pbase_paths_alloc can be larger\n>> than done_pbase_paths_num + 1, while done_pbase_paths\n>> is NULL, such that the memory allocation is not triggered.\n>> If that were the case, then we have 2 subsequent dereferences\n>> of a NULL pointer right after that. But by inspecting the use\n>> of _alloc and _num the initial assumption does not seem possible.\n>\n> Yes, it does appear that way. ALLOC_GROW() calls REALLOC_ARRAY()\n> which safely can realloc NULL to specified size via xrealloc().\n>\n> I'd be more worried about segfault we seem to be getting only on\n> Windows from this:\n>\n> git -C parent grep -e \"(1|2)d(3|4)\" --recurse-submodules HEAD^ > actual\n>\n> in https://travis-ci.org/git/git/jobs/254654195 by the way.\n\nThanks for bringing that to my attention, (I do not follow the travis builds\nas closely, as there is no yelling email in my inbox).\n\nWindows builds on travis seem to be flaky.\n(sometimes they do not start), but there are also\nsuccessful builds, including the -rc0, which may indicate\nbw/grep-recurse-submodules may be faulty on Windows.\n"},{"id":"324710","messageId":"xmqqy3rleijv.fsf@gitster.mtv.corp.google.com","threadId":"46414","inReplyTo":"CAGZ79kYohev8BdBEL9qnfmZ6quMwZPDRqZ44qCKV_wMwR=Etiw@mail.gmail.com","subject":"Re: Fwd: New Defects reported by Coverity Scan for git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-18T17:55:32Z","receivedAt":"2017-07-18T17:55:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> I'd be more worried about segfault we seem to be getting only on\n>> Windows from this:\n>>\n>> git -C parent grep -e \"(1|2)d(3|4)\" --recurse-submodules HEAD^ > actual\n>>\n>> in https://travis-ci.org/git/git/jobs/254654195 by the way.\n>\n> Thanks for bringing that to my attention, (I do not follow the travis builds\n> as closely, as there is no yelling email in my inbox).\n>\n> Windows builds on travis seem to be flaky.\n> (sometimes they do not start), but there are also\n> successful builds, including the -rc0, which may indicate\n> bw/grep-recurse-submodules may be faulty on Windows.\n\nI can get valgrind complaints locally from\n\n $ cd t && sh t7814-grep*.sh --valgrind -i -v\n\nso this may not be Windows only. Can repo_worktree_path() return a NULL\nin repo_read_gitmodules() to cause git_config_from_file() barf on a NULL\ngitmodule_path?\n\n==20383== Syscall param open(filename) points to unaddressable byte(s)\n==20383== at 0x5153140: __open_nocancel (/build/eglibc-SvCtMH/eglibc-2.19/io/../sysdeps/unix/syscall-template.S:81)\n==20383== by 0x50DDED7: _IO_file_fopen@@GLIBC_2.2.5 (/build/eglibc-SvCtMH/eglibc-2.19/libio/fileops.c:228)\n==20383== by 0x50D23D3: __fopen_internal (/build/eglibc-SvCtMH/eglibc-2.19/libio/iofopen.c:90)\n==20383== by 0x569107: git_fopen (/home/gitster/git.git/compat/fopen.c:22)\n==20383== by 0x55B1ED: fopen_or_warn (/home/gitster/git.git/wrapper.c:439)\n==20383== by 0x4A2A32: git_config_from_file (/home/gitster/git.git/config.c:1442)\n==20383== by 0x540317: repo_read_gitmodules (/home/gitster/git.git/submodule.c:269)\n==20383== by 0x434389: grep_submodule (/home/gitster/git.git/builtin/grep.c:422)\n==20383== by 0x4348C8: grep_tree (/home/gitster/git.git/builtin/grep.c:580)\n==20383== by 0x434867: grep_tree (/home/gitster/git.git/builtin/grep.c:576)\n==20383== by 0x436034: cmd_grep (/home/gitster/git.git/builtin/grep.c:622)\n==20383== by 0x4063DC: handle_builtin (/home/gitster/git.git/git.c:330)\n==20383== Address 0x0 is not stack'd, malloc'd or (recently) free'd\n\n\n"},{"id":"324711","messageId":"20170718180057.GA22656@google.com","threadId":"46414","inReplyTo":"xmqqy3rleijv.fsf@gitster.mtv.corp.google.com","subject":"Re: Fwd: New Defects reported by Coverity Scan for git","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-18T18:00:57Z","receivedAt":"2017-07-18T18:01:06Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/18, Junio C Hamano wrote:\n> Stefan Beller writes:\n> \n> >> I'd be more worried about segfault we seem to be getting only on\n> >> Windows from this:\n> >>\n> >> git -C parent grep -e \"(1|2)d(3|4)\" --recurse-submodules HEAD^ > actual\n> >>\n> >> in https://travis-ci.org/git/git/jobs/254654195 by the way.\n> >\n> > Thanks for bringing that to my attention, (I do not follow the travis builds\n> > as closely, as there is no yelling email in my inbox).\n> >\n> > Windows builds on travis seem to be flaky.\n> > (sometimes they do not start), but there are also\n> > successful builds, including the -rc0, which may indicate\n> > bw/grep-recurse-submodules may be faulty on Windows.\n> \n> I can get valgrind complaints locally from\n> \n> $ cd t && sh t7814-grep*.sh --valgrind -i -v\n> \n> so this may not be Windows only. Can repo_worktree_path() return a NULL\n> in repo_read_gitmodules() to cause git_config_from_file() barf on a NULL\n> gitmodule_path?\n\nYep that's most likely the cause. The issue is if a repository doesn't\nhave a worktree then what should a worktree path look like?\nrepo_read_gitmodules() should check if there is a worktree before trying\nto load the gitmodules file. I actually noticed this and have it fixed locally in\nanother series I'm working on right now. Looks like I may have to get\nthat change in first though. Thanks for finding this.\n\n> \n> ==20383== Syscall param open(filename) points to unaddressable byte(s)\n> ==20383== at 0x5153140: __open_nocancel (/build/eglibc-SvCtMH/eglibc-2.19/io/../sysdeps/unix/syscall-template.S:81)\n> ==20383== by 0x50DDED7: _IO_file_fopen@@GLIBC_2.2.5 (/build/eglibc-SvCtMH/eglibc-2.19/libio/fileops.c:228)\n> ==20383== by 0x50D23D3: __fopen_internal (/build/eglibc-SvCtMH/eglibc-2.19/libio/iofopen.c:90)\n> ==20383== by 0x569107: git_fopen (/home/gitster/git.git/compat/fopen.c:22)\n> ==20383== by 0x55B1ED: fopen_or_warn (/home/gitster/git.git/wrapper.c:439)\n> ==20383== by 0x4A2A32: git_config_from_file (/home/gitster/git.git/config.c:1442)\n> ==20383== by 0x540317: repo_read_gitmodules (/home/gitster/git.git/submodule.c:269)\n> ==20383== by 0x434389: grep_submodule (/home/gitster/git.git/builtin/grep.c:422)\n> ==20383== by 0x4348C8: grep_tree (/home/gitster/git.git/builtin/grep.c:580)\n> ==20383== by 0x434867: grep_tree (/home/gitster/git.git/builtin/grep.c:576)\n> ==20383== by 0x436034: cmd_grep (/home/gitster/git.git/builtin/grep.c:622)\n> ==20383== by 0x4063DC: handle_builtin (/home/gitster/git.git/git.c:330)\n> ==20383== Address 0x0 is not stack'd, malloc'd or (recently) free'd\n> \n> \n\n-- \nBrandon Williams\n"},{"id":"324712","messageId":"CAGZ79kZrybXdUXosb=2qoS9jaSYvrkhiBOb6ka=nPZMO3Xt2WA@mail.gmail.com","threadId":"46414","inReplyTo":"20170718180057.GA22656@google.com","subject":"Re: Fwd: New Defects reported by Coverity Scan for git","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-18T18:06:41Z","receivedAt":"2017-07-18T18:06:50Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jul 18, 2017 at 11:00 AM, Brandon Williams wrote:\n> On 07/18, Junio C Hamano wrote:\n>> Stefan Beller writes:\n>>\n>> >> I'd be more worried about segfault we seem to be getting only on\n>> >> Windows from this:\n>> >>\n>> >> git -C parent grep -e \"(1|2)d(3|4)\" --recurse-submodules HEAD^ > actual\n>> >>\n>> >> in https://travis-ci.org/git/git/jobs/254654195 by the way.\n>> >\n>> > Thanks for bringing that to my attention, (I do not follow the travis builds\n>> > as closely, as there is no yelling email in my inbox).\n>> >\n>> > Windows builds on travis seem to be flaky.\n>> > (sometimes they do not start), but there are also\n>> > successful builds, including the -rc0, which may indicate\n>> > bw/grep-recurse-submodules may be faulty on Windows.\n>>\n>> I can get valgrind complaints locally from\n>>\n>> $ cd t && sh t7814-grep*.sh --valgrind -i -v\n>>\n>> so this may not be Windows only. Can repo_worktree_path() return a NULL\n>> in repo_read_gitmodules() to cause git_config_from_file() barf on a NULL\n>> gitmodule_path?\n>\n> Yep that's most likely the cause. The issue is if a repository doesn't\n> have a worktree then what should a worktree path look like?\n> repo_read_gitmodules() should check if there is a worktree before trying\n> to load the gitmodules file. I actually noticed this and have it fixed locally in\n> another series I'm working on right now. Looks like I may have to get\n> that change in first though. Thanks for finding this.\n\nIf there is no worktree, we could fallback to read it from the tree\nHEAD:.gitmodules, if that doesn't exist, then there are no submodules.\n\nThanks,\nStefan\n"},{"id":"324834","messageId":"0110b370-a515-df44-8d46-9a72ac336ee7@web.de","threadId":"46414","inReplyTo":"xmqqa841fylf.fsf@gitster.mtv.corp.google.com","subject":"Re: Fwd: New Defects reported by Coverity Scan for git","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-20T20:32:56Z","receivedAt":"2017-07-20T20:33:18Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.07.2017 um 19:23 schrieb Junio C Hamano:\n> Stefan Beller writes:\n> \n>> I looked at this report for a while. My current understanding:\n>> * its detection was triggered by including rs/move-array,\n>> f331ab9d4c (use MOVE_ARRAY, 2017-07-15)\n>> * But it is harmless, because the scan logic does not understand\n>> how ALLOC_GROW works. It assumes that\n>> done_pbase_paths_alloc can be larger\n>> than done_pbase_paths_num + 1, while done_pbase_paths\n>> is NULL, such that the memory allocation is not triggered.\n>> If that were the case, then we have 2 subsequent dereferences\n>> of a NULL pointer right after that. But by inspecting the use\n>> of _alloc and _num the initial assumption does not seem possible.\n> \n> Yes, it does appear that way. ALLOC_GROW() calls REALLOC_ARRAY()\n> which safely can realloc NULL to specified size via xrealloc().\n\nMOVE_ARRAY is passing its pointer arguments to memmove(); all it adds\nis a check for (done_pbase_paths_num - pos - 1) being zero. I don't\nunderstand how that change can make it more likely for one of the\npointers to be NULL.\n\nI guess the first message ('Comparing \"done_pbase_paths\" to null\nimplies that \"done_pbase_paths\" might be null.') has to be understood\nas an explanation of how the checker arrived at the second one?\n\nWe could remove that NULL check -- it's effectively just a shortcut.\nBut how would that improve safety? Well, if the array is unallocated\n(NULL) and _num is greater than zero we'd get a segfault without it,\nand thus would notice it. That check currently papers over such a\nhypothetical bug. Makes sense?\n\n-- >8 --\nSubject: [PATCH] pack-objects: remove unnecessary NULL check\n\nIf done_pbase_paths is NULL then done_pbase_paths_num must be zero and\ndone_pbase_path_pos() returns -1 without accessing the array, so the\ncheck is not necessary.\n\nIf the invariant was violated then the check would make sure we keep\non going and allocate the necessary amount of memory in the next\nALLOC_GROW call. That sounds nice, but all array entries except for\none would contain garbage data.\n\nIf the invariant was violated without the check we'd get a segfault in\ndone_pbase_path_pos(), i.e. an observable crash, alerting us of the\npresence of a bug.\n\nCurrently there is no such bug: Only the functions check_pbase_path()\nand cleanup_preferred_base() change pointer and counter, and both make\nsure to keep them in sync. Get rid of the check anyway to allow us to\nsee if later changes introduce such a defect, and to simplify the code.\n\nDetected by Coverity Scan.\n\nSigned-off-by: Rene Scharfe \n---\n builtin/pack-objects.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex e730b415bf..c753e9237a 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1289,7 +1289,7 @@ static int done_pbase_path_pos(unsigned hash)\n \n static int check_pbase_path(unsigned hash)\n {\n-\tint pos = (!done_pbase_paths) ? -1 : done_pbase_path_pos(hash);\n+\tint pos = done_pbase_path_pos(hash);\n \tif (0 <= pos)\n \t\treturn 1;\n \tpos = -pos - 1;\n-- \n2.13.3\n"},{"id":"324846","messageId":"xmqq7ez2kc7y.fsf@gitster.mtv.corp.google.com","threadId":"46414","inReplyTo":"0110b370-a515-df44-8d46-9a72ac336ee7@web.de","subject":"Re: Fwd: New Defects reported by Coverity Scan for git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-20T21:52:33Z","receivedAt":"2017-07-20T21:52:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> We could remove that NULL check -- it's effectively just a shortcut.\n> But how would that improve safety? Well, if the array is unallocated\n> (NULL) and _num is greater than zero we'd get a segfault without it,\n> and thus would notice it. That check currently papers over such a\n> hypothetical bug. Makes sense?\n>\n> -- >8 --\n> Subject: [PATCH] pack-objects: remove unnecessary NULL check\n>\n> If done_pbase_paths is NULL then done_pbase_paths_num must be zero and\n> done_pbase_path_pos() returns -1 without accessing the array, so the\n> check is not necessary.\n>\n> If the invariant was violated then the check would make sure we keep\n> on going and allocate the necessary amount of memory in the next\n> ALLOC_GROW call. That sounds nice, but all array entries except for\n> one would contain garbage data.\n>\n> If the invariant was violated without the check we'd get a segfault in\n> done_pbase_path_pos(), i.e. an observable crash, alerting us of the\n> presence of a bug.\n>\n> Currently there is no such bug: Only the functions check_pbase_path()\n> and cleanup_preferred_base() change pointer and counter, and both make\n> sure to keep them in sync. Get rid of the check anyway to allow us to\n> see if later changes introduce such a defect, and to simplify the code.\n>\n> Detected by Coverity Scan.\n>\n> Signed-off-by: Rene Scharfe \n> ---\n\nIt's always amusing to see that a removal of conditional codepath\nwould result in better chance of finding possible invariant\nbreakers, as we often think that belt-and-suspenders safety would\nrequire more conditionals and asserts ;-)\n\n\n\n> builtin/pack-objects.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index e730b415bf..c753e9237a 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -1289,7 +1289,7 @@ static int done_pbase_path_pos(unsigned hash)\n> \n> static int check_pbase_path(unsigned hash)\n> {\n> -\tint pos = (!done_pbase_paths) ? -1 : done_pbase_path_pos(hash);\n> +\tint pos = done_pbase_path_pos(hash);\n> \tif (0 <= pos)\n> \t\treturn 1;\n> \tpos = -pos - 1;\n"}]}