Fwd: New Defects reported by Coverity Scan for git

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:  <scan-admin@coverity.com>
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 {

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);

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

Stefan Beller <sbeller@google.com> writes:

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.

On Tue, Jul 18, 2017 at 10:23 AM, Junio C Hamano <gitster@pobox.com> wrote:

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.

Stefan Beller <sbeller@google.com> writes:

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

On 07/18, Junio C Hamano wrote:

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.

On Tue, Jul 18, 2017 at 11:00 AM, Brandon Williams <bmwill@google.com> wrote:

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

Am 18.07.2017 um 19:23 schrieb Junio C Hamano:

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 <l.s.r@web.de>
---
 builtin/pack-objects.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

René Scharfe <l.s.r@web.de> writes:

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 ;-)