Add a check to prevent max_children from being 0.

From: jinyaoguo <[email protected]>

In function fetch_multiple and fetch_submodules, multiple is stored in opt.process and later used as a divisor in function pp_collect_finished, creating a potential divide-by-zero if it remains zero.

---
Add a check to prevent max_children from being 0, which may cause potential divide-by-zero.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1975%2Fmugitya03%2Fint2-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1975/mugitya03/int2-v1
Pull-Request: https://github.com/git/git/pull/1975

 builtin/fetch.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index cda6eaf1fd6..b668187627a 100644
@@ -2591,7 +2591,7 @@ int cmd_fetch(int argc,
 			die(_("--stdin can only be used when fetching "
 			      "from one remote"));
 
-		if (max_children < 0)
+		if (max_children <= 0)
 			max_children = config.parallel;
 
 		/* TODO should this also die if we have a previous partial-clone? */

@@ -2613,9 +2613,9 @@ int cmd_fetch(int argc,
 		struct strvec options = STRVEC_INIT;
 		int max_children = max_jobs;
 
-		if (max_children < 0)
+		if (max_children <= 0)
 			max_children = config.submodule_fetch_jobs;
-		if (max_children < 0)
+		if (max_children <= 0)
 			max_children = config.parallel;
 
 		add_options_to_argv(&options, &config);

base-commit: 8613c2bb6cd16ef530dc5dd74d3b818a1ccbf1c0

gitgitgadget

This name (i.e. the author ident when you do "git comimt") ...

... must match the name used here you sign your work off as.

Unless you are forwarding a patch that is signed-off by somebody else, in which case, their sign-off comes first and then yours.

I think you may have identified the right problem to fix, but I do not know if the solution is correct.

If max_children can be 0 at this point due to loose parsing of the end-user input, the config.parallel or config.submodule_fetch_jobs configuration variables may be set to 0 due to the same kind of loose parsing.

The command line parser parses -j0 as max_jobs==0 and then calls online_cpus() to use. If the function returned 0 on a platform whose online_cpus() implementation is buggy, max_children may be initialized to 0 there. If fetch.parallel is given 0 by the user, config.parallel gets value from online_cpus(), so it has the same problem. submodule.fetchjobs has exactly the same issue in submodule-config.c::parse_submodule_fetchjobs().

But otherwise, I see no plausible way to have max_children to be 0 here.

And if we want to protect a buggy online_cpus() that returns 0 or negative, which probably is a good thing to do anyway, perhaps we should do so at the source of the issue, perhaps like the attached patch.

Or if you are trying to be defensive to withstand the change to other parts of the code that may affect max_children coming into this function, I think it is better to add

	if (max_children <= 0)
		max_children = 1;

before we enter the trace2_region that calls fetch_multiple() and fetch_submodules().

Hmm?

 thread-utils.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

index 1f89ffab4c..a5d644bb38 100644
@@ -36,7 +36,8 @@ int online_cpus(void)
 #elif defined(hpux) || defined(__hpux) || defined(_hpux)
 	struct pst_dynamic psd;
 
-	if (!pstat_getdynamic(&psd, sizeof(psd), (size_t)1, 0))
+	if (!pstat_getdynamic(&psd, sizeof(psd), (size_t)1, 0) &&
+	    0 < psd.psd_proc_cnt)
 		return (int)psd.psd_proc_cnt;
 #elif defined(HAVE_BSD_SYSCTL) && defined(HW_NCPU)
 	int mib[2];

@@ -47,12 +48,14 @@ int online_cpus(void)
 #  ifdef HW_AVAILCPU
 	mib[1] = HW_AVAILCPU;
 	len = sizeof(cpucount);
-	if (!sysctl(mib, 2, &cpucount, &len, NULL, 0))
+	if (!sysctl(mib, 2, &cpucount, &len, NULL, 0) &&
+	    0 < cpucount)
 		return cpucount;
 #  endif /* HW_AVAILCPU */
 	mib[1] = HW_NCPU;
 	len = sizeof(cpucount);
-	if (!sysctl(mib, 2, &cpucount, &len, NULL, 0))
+	if (!sysctl(mib, 2, &cpucount, &len, NULL, 0) &&
+	    0 < cpucount)
 		return cpucount;
 #endif /* defined(HAVE_BSD_SYSCTL) && defined(HW_NCPU) */
 

This name (i.e. the author ident when you do "git comimt") ...

... must match the name used here you sign your work off as.

Unless you are forwarding a patch that is signed-off by somebody else, in which case, their sign-off comes first and then yours.

I think you may have identified the right problem to fix, but I do not know if the solution is correct.

If max_children can be 0 at this point due to loose parsing of the end-user input, the config.parallel or config.submodule_fetch_jobs configuration variables may be set to 0 due to the same kind of loose parsing.

The command line parser parses -j0 as max_jobs==0 and then calls online_cpus() to use.  If the function returned 0 on a platform whose online_cpus() implementation is buggy, max_children may be initialized to 0 there.  If fetch.parallel is given 0 by the user, config.parallel gets value from online_cpus(), so it has the same problem.  submodule.fetchjobs has exactly the same issue in submodule-config.c::parse_submodule_fetchjobs().

But otherwise, I see no plausible way to have max_children to be 0 here.

And if we want to protect a buggy online_cpus() that returns 0 or negative, which probably is a good thing to do anyway, perhaps we should do so at the source of the issue, perhaps like the attached patch.

Or if you are trying to be defensive to withstand the change to other parts of the code that may affect max_children coming into this function, I think it is better to add

        if (max_children <= 0)
                max_children = 1;

before we enter the trace2_region that calls fetch_multiple() and fetch_submodules().

Hmm?

 thread-utils.c | 9 ++++++---

 1 file changed, 6 insertions(+), 3 deletions(-)

index 1f89ffab4c..a5d644bb38 100644
@@ -36,7 +36,8 @@ int online_cpus(void)

 #elif defined(hpux) || defined(__hpux) || defined(_hpux) struct pst_dynamic psd;

-       if (!pstat_getdynamic(&psd, sizeof(psd), (size_t)1, 0))
+       if (!pstat_getdynamic(&psd, sizeof(psd), (size_t)1, 0) &&
+           0 < psd.psd_proc_cnt)
                return (int)psd.psd_proc_cnt;
 #elif defined(HAVE_BSD_SYSCTL) && defined(HW_NCPU)
        int mib[2];
@@ -47,12 +48,14 @@ int online_cpus(void)
 #  ifdef HW_AVAILCPU
        mib[1] = HW_AVAILCPU;
        len = sizeof(cpucount);
-       if (!sysctl(mib, 2, &cpucount, &len, NULL, 0))
+       if (!sysctl(mib, 2, &cpucount, &len, NULL, 0) &&
+           0 < cpucount)
                return cpucount;
 #  endif /* HW_AVAILCPU */
        mib[1] = HW_NCPU;
        len = sizeof(cpucount);
-       if (!sysctl(mib, 2, &cpucount, &len, NULL, 0))
+       if (!sysctl(mib, 2, &cpucount, &len, NULL, 0) &&
+           0 < cpucount)
                return cpucount;
 #endif /* defined(HAVE_BSD_SYSCTL) && defined(HW_NCPU) */

The patch to online_cpus looks good to me. We can ensure online_cpus() will never return 0 or a negative value under any circumstance.