Re: [PATCH] Add a check to prevent max_children from being 0.
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 23, 2025, 19:30 UTC
- Message-ID
- <xmqq1psfxgyv.fsf@gitster.g>
- In-Reply-To
- <pull.1975.git.git.1748017238130.gitgitgadget@gmail.com>
"Alex via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: jinyaoguo <guo846@purdue.edu>
This name (i.e. the author ident when you do "git comimt") ...
Show 7 quoted lines
> > 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.
Show 27 quoted lines
> 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: 8613c2bb6cd16ef530dc5dd74d3b818a1ccbf1c0I 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) */