# Fwd: New Defects reported by Coverity Scan for git 8 messages from 2017-07-18 to 2017-07-20. Participants: Stefan Beller, Junio C Hamano, Brandon Williams, René Scharfe. Thread: https://gitlist.dev/t/46414 ## Stefan Beller, 2017-07-18 16:59 Subject: Fwd: New Defects reported by Coverity Scan for git Message-ID: URL: https://gitlist.dev/e/CAGZ79kaPFDMn2K0f529-Crzv%2BvhU3XUMsSM6w3QV4RXKBv5s_g%40mail.gmail.com In-Reply-To: <596ddaa620821_77f83e7330107c4@ss1435.mail> ``` I looked at this report for a while. My current understanding: * its detection was triggered by including rs/move-array, f331ab9d4c (use MOVE_ARRAY, 2017-07-15) * But it is harmless, because the scan logic does not understand how ALLOC_GROW works. It assumes that done_pbase_paths_alloc can be larger than done_pbase_paths_num + 1, while done_pbase_paths is NULL, such that the memory allocation is not triggered. If that were the case, then we have 2 subsequent dereferences of a NULL pointer right after that. But by inspecting the use of _alloc and _num the initial assumption does not seem possible. Stefan ---------- Forwarded message ---------- From: Date: Tue, Jul 18, 2017 at 2:53 AM Subject: New Defects reported by Coverity Scan for git To: sbeller@google.com Hi, Please find the latest report on new defect(s) introduced to git found with Coverity Scan. 2 new defect(s) introduced to git found with Coverity Scan. 1 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan. New defect(s) Reported-by: Coverity Scan Showing 2 of 2 defect(s) ** CID 1415508: Null pointer dereferences (FORWARD_NULL) /builtin/pack-objects.c: 1292 in check_pbase_path() ________________________________________________________________________________________________________ *** CID 1415508: Null pointer dereferences (FORWARD_NULL) /builtin/pack-objects.c: 1292 in check_pbase_path() 1286 } 1287 return -lo-1; 1288 } 1289 1290 static int check_pbase_path(unsigned hash) 1291 { >>> CID 1415508: Null pointer dereferences (FORWARD_NULL) >>> Comparing "done_pbase_paths" to null implies that "done_pbase_paths" might be null. 1292 int pos = (!done_pbase_paths) ? -1 : done_pbase_path_pos(hash); 1293 if (0 <= pos) 1294 return 1; 1295 pos = -pos - 1; 1296 ALLOC_GROW(done_pbase_paths, 1297 done_pbase_paths_num + 1, ** CID 1415507: Null pointer dereferences (FORWARD_NULL) /builtin/pack-objects.c: 1303 in check_pbase_path() ________________________________________________________________________________________________________ *** CID 1415507: Null pointer dereferences (FORWARD_NULL) /builtin/pack-objects.c: 1303 in check_pbase_path() 1297 done_pbase_paths_num + 1, 1298 done_pbase_paths_alloc); 1299 done_pbase_paths_num++; 1300 if (pos < done_pbase_paths_num) 1301 MOVE_ARRAY(done_pbase_paths + pos + 1, done_pbase_paths + pos, 1302 done_pbase_paths_num - pos - 1); >>> CID 1415507: Null pointer dereferences (FORWARD_NULL) >>> Dereferencing null pointer "done_pbase_paths". 1303 done_pbase_paths[pos] = hash; 1304 return 0; 1305 } 1306 1307 static void add_preferred_base_object(const char *name) 1308 { ________________________________________________________________________________________________________ To view the defects in Coverity Scan visit, https://u2389337.ct.sendgrid.net/wf/click?upn=08onrYu34A-2BWcWUl-2F-2BfV0V05UPxvVjWch-2Bd2MGckcRb8HAP5hlBzHe8sORKm64S-2F81GsNbRdSrOteP-2FXoviMkw-3D-3D_PwiGIFugGOKqSZ6DZhASdI2SvWKInry4kHBXrJUc9pnRRRwN8fPiR-2BR4LTK2qB-2F8DwbeZJjY7Zg2FBdb8jgiAk7m6rh1YdNCvPYCPUewgRcPRcmkOFDltPB2GLYjg5Pl86kCKSRkx6inI-2BuknVr53Cjba4HgtlWmCuW5A0WMiIFvSKDW3-2BKYfPjiZDMCOFSGSLivQrUyaTeOHAHjl-2FNvbw-3D-3D To manage Coverity Scan email notifications for "sbeller@google.com", click 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 ``` ## Junio C Hamano, 2017-07-18 17:23 Subject: Re: Fwd: New Defects reported by Coverity Scan for git Message-ID: URL: https://gitlist.dev/e/xmqqa841fylf.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Stefan Beller writes: > I looked at this report for a while. My current understanding: > * its detection was triggered by including rs/move-array, > f331ab9d4c (use MOVE_ARRAY, 2017-07-15) > * But it is harmless, because the scan logic does not understand > how ALLOC_GROW works. It assumes that > done_pbase_paths_alloc can be larger > than done_pbase_paths_num + 1, while done_pbase_paths > is NULL, such that the memory allocation is not triggered. > If that were the case, then we have 2 subsequent dereferences > of a NULL pointer right after that. But by inspecting the use > of _alloc and _num the initial assumption does not seem possible. Yes, it does appear that way. ALLOC_GROW() calls REALLOC_ARRAY() which safely can realloc NULL to specified size via xrealloc(). I'd be more worried about segfault we seem to be getting only on Windows from this: git -C parent grep -e "(1|2)d(3|4)" --recurse-submodules HEAD^ > actual in https://travis-ci.org/git/git/jobs/254654195 by the way. ``` ## Stefan Beller, 2017-07-18 17:41 Subject: Re: Fwd: New Defects reported by Coverity Scan for git Message-ID: URL: https://gitlist.dev/e/CAGZ79kYohev8BdBEL9qnfmZ6quMwZPDRqZ44qCKV_wMwR%3DEtiw%40mail.gmail.com In-Reply-To: ``` On Tue, Jul 18, 2017 at 10:23 AM, Junio C Hamano wrote: > Stefan Beller writes: > >> I looked at this report for a while. My current understanding: >> * its detection was triggered by including rs/move-array, >> f331ab9d4c (use MOVE_ARRAY, 2017-07-15) >> * But it is harmless, because the scan logic does not understand >> how ALLOC_GROW works. It assumes that >> done_pbase_paths_alloc can be larger >> than done_pbase_paths_num + 1, while done_pbase_paths >> is NULL, such that the memory allocation is not triggered. >> If that were the case, then we have 2 subsequent dereferences >> of a NULL pointer right after that. But by inspecting the use >> of _alloc and _num the initial assumption does not seem possible. > > Yes, it does appear that way. ALLOC_GROW() calls REALLOC_ARRAY() > which safely can realloc NULL to specified size via xrealloc(). > > I'd be more worried about segfault we seem to be getting only on > Windows from this: > > git -C parent grep -e "(1|2)d(3|4)" --recurse-submodules HEAD^ > actual > > in https://travis-ci.org/git/git/jobs/254654195 by the way. Thanks for bringing that to my attention, (I do not follow the travis builds as closely, as there is no yelling email in my inbox). Windows builds on travis seem to be flaky. (sometimes they do not start), but there are also successful builds, including the -rc0, which may indicate bw/grep-recurse-submodules may be faulty on Windows. ``` ## Junio C Hamano, 2017-07-18 17:55 Subject: Re: Fwd: New Defects reported by Coverity Scan for git Message-ID: URL: https://gitlist.dev/e/xmqqy3rleijv.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Stefan Beller writes: >> I'd be more worried about segfault we seem to be getting only on >> Windows from this: >> >> git -C parent grep -e "(1|2)d(3|4)" --recurse-submodules HEAD^ > actual >> >> in https://travis-ci.org/git/git/jobs/254654195 by the way. > > Thanks for bringing that to my attention, (I do not follow the travis builds > as closely, as there is no yelling email in my inbox). > > Windows builds on travis seem to be flaky. > (sometimes they do not start), but there are also > successful builds, including the -rc0, which may indicate > bw/grep-recurse-submodules may be faulty on Windows. I can get valgrind complaints locally from $ cd t && sh t7814-grep*.sh --valgrind -i -v so this may not be Windows only. Can repo_worktree_path() return a NULL in repo_read_gitmodules() to cause git_config_from_file() barf on a NULL gitmodule_path? ==20383== Syscall param open(filename) points to unaddressable byte(s) ==20383== at 0x5153140: __open_nocancel (/build/eglibc-SvCtMH/eglibc-2.19/io/../sysdeps/unix/syscall-template.S:81) ==20383== by 0x50DDED7: _IO_file_fopen@@GLIBC_2.2.5 (/build/eglibc-SvCtMH/eglibc-2.19/libio/fileops.c:228) ==20383== by 0x50D23D3: __fopen_internal (/build/eglibc-SvCtMH/eglibc-2.19/libio/iofopen.c:90) ==20383== by 0x569107: git_fopen (/home/gitster/git.git/compat/fopen.c:22) ==20383== by 0x55B1ED: fopen_or_warn (/home/gitster/git.git/wrapper.c:439) ==20383== by 0x4A2A32: git_config_from_file (/home/gitster/git.git/config.c:1442) ==20383== by 0x540317: repo_read_gitmodules (/home/gitster/git.git/submodule.c:269) ==20383== by 0x434389: grep_submodule (/home/gitster/git.git/builtin/grep.c:422) ==20383== by 0x4348C8: grep_tree (/home/gitster/git.git/builtin/grep.c:580) ==20383== by 0x434867: grep_tree (/home/gitster/git.git/builtin/grep.c:576) ==20383== by 0x436034: cmd_grep (/home/gitster/git.git/builtin/grep.c:622) ==20383== by 0x4063DC: handle_builtin (/home/gitster/git.git/git.c:330) ==20383== Address 0x0 is not stack'd, malloc'd or (recently) free'd ``` ## Brandon Williams, 2017-07-18 18:00 Subject: Re: Fwd: New Defects reported by Coverity Scan for git Message-ID: <20170718180057.GA22656@google.com> URL: https://gitlist.dev/e/20170718180057.GA22656%40google.com In-Reply-To: ``` On 07/18, Junio C Hamano wrote: > Stefan Beller writes: > > >> I'd be more worried about segfault we seem to be getting only on > >> Windows from this: > >> > >> git -C parent grep -e "(1|2)d(3|4)" --recurse-submodules HEAD^ > actual > >> > >> in https://travis-ci.org/git/git/jobs/254654195 by the way. > > > > Thanks for bringing that to my attention, (I do not follow the travis builds > > as closely, as there is no yelling email in my inbox). > > > > Windows builds on travis seem to be flaky. > > (sometimes they do not start), but there are also > > successful builds, including the -rc0, which may indicate > > bw/grep-recurse-submodules may be faulty on Windows. > > I can get valgrind complaints locally from > > $ cd t && sh t7814-grep*.sh --valgrind -i -v > > so this may not be Windows only. Can repo_worktree_path() return a NULL > in repo_read_gitmodules() to cause git_config_from_file() barf on a NULL > gitmodule_path? Yep that's most likely the cause. The issue is if a repository doesn't have a worktree then what should a worktree path look like? repo_read_gitmodules() should check if there is a worktree before trying to load the gitmodules file. I actually noticed this and have it fixed locally in another series I'm working on right now. Looks like I may have to get that change in first though. Thanks for finding this. > > ==20383== Syscall param open(filename) points to unaddressable byte(s) > ==20383== at 0x5153140: __open_nocancel (/build/eglibc-SvCtMH/eglibc-2.19/io/../sysdeps/unix/syscall-template.S:81) > ==20383== by 0x50DDED7: _IO_file_fopen@@GLIBC_2.2.5 (/build/eglibc-SvCtMH/eglibc-2.19/libio/fileops.c:228) > ==20383== by 0x50D23D3: __fopen_internal (/build/eglibc-SvCtMH/eglibc-2.19/libio/iofopen.c:90) > ==20383== by 0x569107: git_fopen (/home/gitster/git.git/compat/fopen.c:22) > ==20383== by 0x55B1ED: fopen_or_warn (/home/gitster/git.git/wrapper.c:439) > ==20383== by 0x4A2A32: git_config_from_file (/home/gitster/git.git/config.c:1442) > ==20383== by 0x540317: repo_read_gitmodules (/home/gitster/git.git/submodule.c:269) > ==20383== by 0x434389: grep_submodule (/home/gitster/git.git/builtin/grep.c:422) > ==20383== by 0x4348C8: grep_tree (/home/gitster/git.git/builtin/grep.c:580) > ==20383== by 0x434867: grep_tree (/home/gitster/git.git/builtin/grep.c:576) > ==20383== by 0x436034: cmd_grep (/home/gitster/git.git/builtin/grep.c:622) > ==20383== by 0x4063DC: handle_builtin (/home/gitster/git.git/git.c:330) > ==20383== Address 0x0 is not stack'd, malloc'd or (recently) free'd > > -- Brandon Williams ``` ## Stefan Beller, 2017-07-18 18:06 Subject: Re: Fwd: New Defects reported by Coverity Scan for git Message-ID: URL: https://gitlist.dev/e/CAGZ79kZrybXdUXosb%3D2qoS9jaSYvrkhiBOb6ka%3DnPZMO3Xt2WA%40mail.gmail.com In-Reply-To: <20170718180057.GA22656@google.com> ``` On Tue, Jul 18, 2017 at 11:00 AM, Brandon Williams wrote: > On 07/18, Junio C Hamano wrote: >> Stefan Beller writes: >> >> >> I'd be more worried about segfault we seem to be getting only on >> >> Windows from this: >> >> >> >> git -C parent grep -e "(1|2)d(3|4)" --recurse-submodules HEAD^ > actual >> >> >> >> in https://travis-ci.org/git/git/jobs/254654195 by the way. >> > >> > Thanks for bringing that to my attention, (I do not follow the travis builds >> > as closely, as there is no yelling email in my inbox). >> > >> > Windows builds on travis seem to be flaky. >> > (sometimes they do not start), but there are also >> > successful builds, including the -rc0, which may indicate >> > bw/grep-recurse-submodules may be faulty on Windows. >> >> I can get valgrind complaints locally from >> >> $ cd t && sh t7814-grep*.sh --valgrind -i -v >> >> so this may not be Windows only. Can repo_worktree_path() return a NULL >> in repo_read_gitmodules() to cause git_config_from_file() barf on a NULL >> gitmodule_path? > > Yep that's most likely the cause. The issue is if a repository doesn't > have a worktree then what should a worktree path look like? > repo_read_gitmodules() should check if there is a worktree before trying > to load the gitmodules file. I actually noticed this and have it fixed locally in > another series I'm working on right now. Looks like I may have to get > that change in first though. Thanks for finding this. If there is no worktree, we could fallback to read it from the tree HEAD:.gitmodules, if that doesn't exist, then there are no submodules. Thanks, Stefan ``` ## René Scharfe, 2017-07-20 20:32 Subject: Re: Fwd: New Defects reported by Coverity Scan for git Message-ID: <0110b370-a515-df44-8d46-9a72ac336ee7@web.de> URL: https://gitlist.dev/e/0110b370-a515-df44-8d46-9a72ac336ee7%40web.de In-Reply-To: ``` Am 18.07.2017 um 19:23 schrieb Junio C Hamano: > Stefan Beller writes: > >> I looked at this report for a while. My current understanding: >> * its detection was triggered by including rs/move-array, >> f331ab9d4c (use MOVE_ARRAY, 2017-07-15) >> * But it is harmless, because the scan logic does not understand >> how ALLOC_GROW works. It assumes that >> done_pbase_paths_alloc can be larger >> than done_pbase_paths_num + 1, while done_pbase_paths >> is NULL, such that the memory allocation is not triggered. >> If that were the case, then we have 2 subsequent dereferences >> of a NULL pointer right after that. But by inspecting the use >> of _alloc and _num the initial assumption does not seem possible. > > Yes, it does appear that way. ALLOC_GROW() calls REALLOC_ARRAY() > which safely can realloc NULL to specified size via xrealloc(). MOVE_ARRAY is passing its pointer arguments to memmove(); all it adds is a check for (done_pbase_paths_num - pos - 1) being zero. I don't understand how that change can make it more likely for one of the pointers to be NULL. I guess the first message ('Comparing "done_pbase_paths" to null implies that "done_pbase_paths" might be null.') has to be understood as an explanation of how the checker arrived at the second one? We could remove that NULL check -- it's effectively just a shortcut. But how would that improve safety? Well, if the array is unallocated (NULL) and _num is greater than zero we'd get a segfault without it, and thus would notice it. That check currently papers over such a hypothetical bug. Makes sense? -- >8 -- Subject: [PATCH] pack-objects: remove unnecessary NULL check If done_pbase_paths is NULL then done_pbase_paths_num must be zero and done_pbase_path_pos() returns -1 without accessing the array, so the check is not necessary. If the invariant was violated then the check would make sure we keep on going and allocate the necessary amount of memory in the next ALLOC_GROW call. That sounds nice, but all array entries except for one would contain garbage data. If the invariant was violated without the check we'd get a segfault in done_pbase_path_pos(), i.e. an observable crash, alerting us of the presence of a bug. Currently there is no such bug: Only the functions check_pbase_path() and cleanup_preferred_base() change pointer and counter, and both make sure to keep them in sync. Get rid of the check anyway to allow us to see if later changes introduce such a defect, and to simplify the code. Detected by Coverity Scan. Signed-off-by: Rene Scharfe --- builtin/pack-objects.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index e730b415bf..c753e9237a 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1289,7 +1289,7 @@ static int done_pbase_path_pos(unsigned hash) static int check_pbase_path(unsigned hash) { - int pos = (!done_pbase_paths) ? -1 : done_pbase_path_pos(hash); + int pos = done_pbase_path_pos(hash); if (0 <= pos) return 1; pos = -pos - 1; -- 2.13.3 ``` ## Junio C Hamano, 2017-07-20 21:52 Subject: Re: Fwd: New Defects reported by Coverity Scan for git Message-ID: URL: https://gitlist.dev/e/xmqq7ez2kc7y.fsf%40gitster.mtv.corp.google.com In-Reply-To: <0110b370-a515-df44-8d46-9a72ac336ee7@web.de> ``` René Scharfe writes: > We could remove that NULL check -- it's effectively just a shortcut. > But how would that improve safety? Well, if the array is unallocated > (NULL) and _num is greater than zero we'd get a segfault without it, > and thus would notice it. That check currently papers over such a > hypothetical bug. Makes sense? > > -- >8 -- > Subject: [PATCH] pack-objects: remove unnecessary NULL check > > If done_pbase_paths is NULL then done_pbase_paths_num must be zero and > done_pbase_path_pos() returns -1 without accessing the array, so the > check is not necessary. > > If the invariant was violated then the check would make sure we keep > on going and allocate the necessary amount of memory in the next > ALLOC_GROW call. That sounds nice, but all array entries except for > one would contain garbage data. > > If the invariant was violated without the check we'd get a segfault in > done_pbase_path_pos(), i.e. an observable crash, alerting us of the > presence of a bug. > > Currently there is no such bug: Only the functions check_pbase_path() > and cleanup_preferred_base() change pointer and counter, and both make > sure to keep them in sync. Get rid of the check anyway to allow us to > see if later changes introduce such a defect, and to simplify the code. > > Detected by Coverity Scan. > > Signed-off-by: Rene Scharfe > --- It's always amusing to see that a removal of conditional codepath would result in better chance of finding possible invariant breakers, as we often think that belt-and-suspenders safety would require more conditionals and asserts ;-) > builtin/pack-objects.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c > index e730b415bf..c753e9237a 100644 > --- a/builtin/pack-objects.c > +++ b/builtin/pack-objects.c > @@ -1289,7 +1289,7 @@ static int done_pbase_path_pos(unsigned hash) > > static int check_pbase_path(unsigned hash) > { > - int pos = (!done_pbase_paths) ? -1 : done_pbase_path_pos(hash); > + int pos = done_pbase_path_pos(hash); > if (0 <= pos) > return 1; > pos = -pos - 1; ```