[PATCH] Add a check to prevent max_children from being 0.

[PATCH] Add a check to prevent max_children from being 0.

[Alex via GitGitGadget]

From: jinyaoguo <guo846@purdue.edu>

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.

Signed-off-by: Alex Guo <alexguo1023@gmail.com>
---
    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(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index cda6eaf1fd6..b668187627a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -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

Re: [PATCH] Add a check to prevent max_children from being 0.

[Junio C Hamano]

"Alex via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: jinyaoguo <guo846@purdue.edu>

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

>
> 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.
>
> Signed-off-by: Alex Guo <alexguo1023@gmail.com>

... 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.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index cda6eaf1fd6..b668187627a 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -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

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

diff --git c/thread-utils.c w/thread-utils.c
index 1f89ffab4c..a5d644bb38 100644
--- c/thread-utils.c
+++ w/thread-utils.c
@@ -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) */
 

Re: [PATCH] Add a check to prevent max_children from being 0.

[Jinyao Guo]

"Alex via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: jinyaoguo <guo846@purdue.edu>

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

>
> 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.
>
> Signed-off-by: Alex Guo <alexguo1023@gmail.com>

... 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.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index cda6eaf1fd6..b668187627a 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -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

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

diff --git c/thread-utils.c w/thread-utils.c
index 1f89ffab4c..a5d644bb38 100644
--- c/thread-utils.c
+++ w/thread-utils.c
@@ -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.