{"thread":{"id":"63509","subject":"[PATCH] Add a check to prevent max_children from being 0.","startedAt":"2025-05-23T16:20:44Z","lastAt":"2025-05-25T15:20:02Z","messageCount":3,"participants":["Alex via GitGitGadget","Junio C Hamano","Jinyao Guo"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"518777","messageId":"pull.1975.git.git.1748017238130.gitgitgadget@gmail.com","threadId":"63509","inReplyTo":null,"subject":"[PATCH] Add a check to prevent max_children from being 0.","fromName":"Alex via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-23T16:20:37Z","receivedAt":"2025-05-23T16:20:44Z","isPatch":true,"sender":{"key":"ajb44.geo@yahoo.com","avatar":null},"body":"From: jinyaoguo <guo846@purdue.edu>\n\nIn function fetch_multiple and fetch_submodules, `multiple` is\nstored in `opt.process` and later used as a divisor in function\n`pp_collect_finished`, creating a potential divide-by-zero if it\nremains zero.\n\nSigned-off-by: Alex Guo <alexguo1023@gmail.com>\n---\n    Add a check to prevent max_children from being 0, which may cause\n    potential divide-by-zero.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1975%2Fmugitya03%2Fint2-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1975/mugitya03/int2-v1\nPull-Request: https://github.com/git/git/pull/1975\n\n builtin/fetch.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex cda6eaf1fd6..b668187627a 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2591,7 +2591,7 @@ int cmd_fetch(int argc,\n \t\t\tdie(_(\"--stdin can only be used when fetching \"\n \t\t\t      \"from one remote\"));\n \n-\t\tif (max_children < 0)\n+\t\tif (max_children <= 0)\n \t\t\tmax_children = config.parallel;\n \n \t\t/* TODO should this also die if we have a previous partial-clone? */\n@@ -2613,9 +2613,9 @@ int cmd_fetch(int argc,\n \t\tstruct strvec options = STRVEC_INIT;\n \t\tint max_children = max_jobs;\n \n-\t\tif (max_children < 0)\n+\t\tif (max_children <= 0)\n \t\t\tmax_children = config.submodule_fetch_jobs;\n-\t\tif (max_children < 0)\n+\t\tif (max_children <= 0)\n \t\t\tmax_children = config.parallel;\n \n \t\tadd_options_to_argv(&options, &config);\n\nbase-commit: 8613c2bb6cd16ef530dc5dd74d3b818a1ccbf1c0\n-- \ngitgitgadget\n"},{"id":"518787","messageId":"xmqq1psfxgyv.fsf@gitster.g","threadId":"63509","inReplyTo":"pull.1975.git.git.1748017238130.gitgitgadget@gmail.com","subject":"Re: [PATCH] Add a check to prevent max_children from being 0.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-23T19:30:32Z","receivedAt":"2025-05-23T19:30:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: jinyaoguo <guo846@purdue.edu>\n\nThis name (i.e. the author ident when you do \"git comimt\") ...\n\n>\n> In function fetch_multiple and fetch_submodules, `multiple` is\n> stored in `opt.process` and later used as a divisor in function\n> `pp_collect_finished`, creating a potential divide-by-zero if it\n> remains zero.\n>\n> Signed-off-by: Alex Guo <alexguo1023@gmail.com>\n\n... must match the name used here you sign your work off as.\n\nUnless you are forwarding a patch that is signed-off by somebody\nelse, in which case, their sign-off comes first and then yours.\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index cda6eaf1fd6..b668187627a 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -2591,7 +2591,7 @@ int cmd_fetch(int argc,\n>  \t\t\tdie(_(\"--stdin can only be used when fetching \"\n>  \t\t\t      \"from one remote\"));\n>  \n> -\t\tif (max_children < 0)\n> +\t\tif (max_children <= 0)\n>  \t\t\tmax_children = config.parallel;\n>  \n>  \t\t/* TODO should this also die if we have a previous partial-clone? */\n> @@ -2613,9 +2613,9 @@ int cmd_fetch(int argc,\n>  \t\tstruct strvec options = STRVEC_INIT;\n>  \t\tint max_children = max_jobs;\n>  \n> -\t\tif (max_children < 0)\n> +\t\tif (max_children <= 0)\n>  \t\t\tmax_children = config.submodule_fetch_jobs;\n> -\t\tif (max_children < 0)\n> +\t\tif (max_children <= 0)\n>  \t\t\tmax_children = config.parallel;\n>  \n>  \t\tadd_options_to_argv(&options, &config);\n>\n> base-commit: 8613c2bb6cd16ef530dc5dd74d3b818a1ccbf1c0\n\nI think you may have identified the right problem to fix, but I do\nnot know if the solution is correct.\n\nIf max_children can be 0 at this point due to loose parsing of the\nend-user input, the config.parallel or config.submodule_fetch_jobs\nconfiguration variables may be set to 0 due to the same kind of\nloose parsing.\n\nThe command line parser parses -j0 as max_jobs==0 and then calls\nonline_cpus() to use.  If the function returned 0 on a platform\nwhose online_cpus() implementation is buggy, max_children may be\ninitialized to 0 there.  If fetch.parallel is given 0 by the user,\nconfig.parallel gets value from online_cpus(), so it has the same\nproblem.  submodule.fetchjobs has exactly the same issue in\nsubmodule-config.c::parse_submodule_fetchjobs().\n\nBut otherwise, I see no plausible way to have max_children to be 0\nhere.\n\nAnd if we want to protect a buggy online_cpus() that returns 0 or\nnegative, which probably is a good thing to do anyway, perhaps we\nshould do so at the source of the issue, perhaps like the attached\npatch.\n\nOr if you are trying to be defensive to withstand the change to\nother parts of the code that may affect max_children coming into\nthis function, I think it is better to add\n\n\n\tif (max_children <= 0)\n\t\tmax_children = 1;\n\nbefore we enter the trace2_region that calls fetch_multiple() and\nfetch_submodules().\n\nHmm?\n\n\n thread-utils.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git c/thread-utils.c w/thread-utils.c\nindex 1f89ffab4c..a5d644bb38 100644\n--- c/thread-utils.c\n+++ w/thread-utils.c\n@@ -36,7 +36,8 @@ int online_cpus(void)\n #elif defined(hpux) || defined(__hpux) || defined(_hpux)\n \tstruct pst_dynamic psd;\n \n-\tif (!pstat_getdynamic(&psd, sizeof(psd), (size_t)1, 0))\n+\tif (!pstat_getdynamic(&psd, sizeof(psd), (size_t)1, 0) &&\n+\t    0 < psd.psd_proc_cnt)\n \t\treturn (int)psd.psd_proc_cnt;\n #elif defined(HAVE_BSD_SYSCTL) && defined(HW_NCPU)\n \tint mib[2];\n@@ -47,12 +48,14 @@ int online_cpus(void)\n #  ifdef HW_AVAILCPU\n \tmib[1] = HW_AVAILCPU;\n \tlen = sizeof(cpucount);\n-\tif (!sysctl(mib, 2, &cpucount, &len, NULL, 0))\n+\tif (!sysctl(mib, 2, &cpucount, &len, NULL, 0) &&\n+\t    0 < cpucount)\n \t\treturn cpucount;\n #  endif /* HW_AVAILCPU */\n \tmib[1] = HW_NCPU;\n \tlen = sizeof(cpucount);\n-\tif (!sysctl(mib, 2, &cpucount, &len, NULL, 0))\n+\tif (!sysctl(mib, 2, &cpucount, &len, NULL, 0) &&\n+\t    0 < cpucount)\n \t\treturn cpucount;\n #endif /* defined(HAVE_BSD_SYSCTL) && defined(HW_NCPU) */\n \n\n"},{"id":"518860","messageId":"SA1PR22MB3999A0C9E99E607A793A591DE49AA@SA1PR22MB3999.namprd22.prod.outlook.com","threadId":"63509","inReplyTo":"xmqq1psfxgyv.fsf@gitster.g","subject":"Re: [PATCH] Add a check to prevent max_children from being 0.","fromName":"Jinyao Guo","fromEmail":"guo846@purdue.edu","sentAt":"2025-05-25T15:19:59Z","receivedAt":"2025-05-25T15:20:02Z","isPatch":true,"sender":{"key":"guo846@purdue.edu","avatar":"https://avatars.githubusercontent.com/u/70044335?v=4"},"body":"\"Alex via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: jinyaoguo <guo846@purdue.edu>\n\nThis name (i.e. the author ident when you do \"git comimt\") ...\n\n>\n> In function fetch_multiple and fetch_submodules, `multiple` is\n> stored in `opt.process` and later used as a divisor in function\n> `pp_collect_finished`, creating a potential divide-by-zero if it\n> remains zero.\n>\n> Signed-off-by: Alex Guo <alexguo1023@gmail.com>\n\n... must match the name used here you sign your work off as.\n\nUnless you are forwarding a patch that is signed-off by somebody\nelse, in which case, their sign-off comes first and then yours.\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index cda6eaf1fd6..b668187627a 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -2591,7 +2591,7 @@ int cmd_fetch(int argc,\n>                       die(_(\"--stdin can only be used when fetching \"\n>                             \"from one remote\"));\n>\n> -             if (max_children < 0)\n> +             if (max_children <= 0)\n>                       max_children = config.parallel;\n>\n>               /* TODO should this also die if we have a previous partial-clone? */\n> @@ -2613,9 +2613,9 @@ int cmd_fetch(int argc,\n>               struct strvec options = STRVEC_INIT;\n>               int max_children = max_jobs;\n>\n> -             if (max_children < 0)\n> +             if (max_children <= 0)\n>                       max_children = config.submodule_fetch_jobs;\n> -             if (max_children < 0)\n> +             if (max_children <= 0)\n>                       max_children = config.parallel;\n>\n>               add_options_to_argv(&options, &config);\n>\n> base-commit: 8613c2bb6cd16ef530dc5dd74d3b818a1ccbf1c0\n\nI think you may have identified the right problem to fix, but I do\nnot know if the solution is correct.\n\nIf max_children can be 0 at this point due to loose parsing of the\nend-user input, the config.parallel or config.submodule_fetch_jobs\nconfiguration variables may be set to 0 due to the same kind of\nloose parsing.\n\nThe command line parser parses -j0 as max_jobs==0 and then calls\nonline_cpus() to use.  If the function returned 0 on a platform\nwhose online_cpus() implementation is buggy, max_children may be\ninitialized to 0 there.  If fetch.parallel is given 0 by the user,\nconfig.parallel gets value from online_cpus(), so it has the same\nproblem.  submodule.fetchjobs has exactly the same issue in\nsubmodule-config.c::parse_submodule_fetchjobs().\n\nBut otherwise, I see no plausible way to have max_children to be 0\nhere.\n\nAnd if we want to protect a buggy online_cpus() that returns 0 or\nnegative, which probably is a good thing to do anyway, perhaps we\nshould do so at the source of the issue, perhaps like the attached\npatch.\n\nOr if you are trying to be defensive to withstand the change to\nother parts of the code that may affect max_children coming into\nthis function, I think it is better to add\n\n\n        if (max_children <= 0)\n                max_children = 1;\n\nbefore we enter the trace2_region that calls fetch_multiple() and\nfetch_submodules().\n\nHmm?\n\n\n thread-utils.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git c/thread-utils.c w/thread-utils.c\nindex 1f89ffab4c..a5d644bb38 100644\n--- c/thread-utils.c\n+++ w/thread-utils.c\n@@ -36,7 +36,8 @@ int online_cpus(void)\n #elif defined(hpux) || defined(__hpux) || defined(_hpux)\n        struct pst_dynamic psd;\n\n-       if (!pstat_getdynamic(&psd, sizeof(psd), (size_t)1, 0))\n+       if (!pstat_getdynamic(&psd, sizeof(psd), (size_t)1, 0) &&\n+           0 < psd.psd_proc_cnt)\n                return (int)psd.psd_proc_cnt;\n #elif defined(HAVE_BSD_SYSCTL) && defined(HW_NCPU)\n        int mib[2];\n@@ -47,12 +48,14 @@ int online_cpus(void)\n #  ifdef HW_AVAILCPU\n        mib[1] = HW_AVAILCPU;\n        len = sizeof(cpucount);\n-       if (!sysctl(mib, 2, &cpucount, &len, NULL, 0))\n+       if (!sysctl(mib, 2, &cpucount, &len, NULL, 0) &&\n+           0 < cpucount)\n                return cpucount;\n #  endif /* HW_AVAILCPU */\n        mib[1] = HW_NCPU;\n        len = sizeof(cpucount);\n-       if (!sysctl(mib, 2, &cpucount, &len, NULL, 0))\n+       if (!sysctl(mib, 2, &cpucount, &len, NULL, 0) &&\n+           0 < cpucount)\n                return cpucount;\n #endif /* defined(HAVE_BSD_SYSCTL) && defined(HW_NCPU) */\n\n\nThe patch to `online_cpus` looks good to me. We can ensure online_cpus() will never return 0 or a negative value under any circumstance. \n"}]}