Re: [PATCH v6 5/7] submodule: fix segmentation fault in submodule--helper clone

From: Stefan Beller <sbeller@google.com>
Date: Feb 29, 2016, 23:20 UTC
Message-ID: <CAGZ79kZdct21==TeRoVr4NpgeKuHeUfpJ1Qsh+_tPJt0apr5DA@mail.gmail.com>
In-Reply-To: <1456786715-24256-6-git-send-email-jacob.e.keller@intel.com>

On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:

Show 26 quoted lines

> From: Jacob Keller <jacob.keller@gmail.com>
>
> The git submodule--helper clone command will fail with a segmentation
> fault when given a null url or null path variable. Since these are
> required for proper functioning of the submodule--helper clone
> subcommand, add checks to prevent running and fail gracefully when
> missing.
>
> Update the usage string to reflect the requirement that the --url and
> --path "options" are required.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> This bug was discovered when trying to test the usage string of the
> previous change. I bisected and this has been in the submodule--helper
> code since the first iteration of module_clone. I am not really sure how
> much we care, since only internal callers will be using
> submodule--helper, but I suspect that it was not intended to segfault.
>
> I fixed the usage string to remove the [] around the url and path
> options, hopefully indicating they aren't "optional". Alternatively we
> could start requiring them as regular arguments if we wanted.
>
> Note the error message resulting from --url="" or --path="" are not very
> obvious but they do not result in a segfault so I didn't check for those
> in this patch.

I think this bug was put in, by "literally" translating from shell, see ee8838d15776, where the shell code was rewritten to C, specially:

 git clone $quiet ${depth:+"$depth"} -n ${reference:+"$reference"} \
     --separate-git-dir "$gitdir" "$url" "$sm_path"

Anything except url and path are done optionally, but url, and path not so. The C code was inspired by this and aligned.

This patch makes sense to me, Thanks, Stefan

Show 30 quoted lines

>
>  builtin/submodule--helper.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 3c4d3ff7f4af..35ae85a7e1bc 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -186,15 +186,15 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>
>         const char *const git_submodule_helper_usage[] = {
>                 N_("git submodule--helper clone [--prefix=<path>] [--quiet] "
> -                  "[--reference <repository>] [--name <name>] [--url <url>]"
> -                  "[--depth <depth>] [--path <path>]"),
> +                  "[--reference <repository>] [--name <name>] [--depth <depth>] "
> +                  "--url <url> --path <path>"),
>                 NULL
>         };
>
>         argc = parse_options(argc, argv, prefix, module_clone_options,
>                              git_submodule_helper_usage, 0);
>
> -       if (argc)
> +       if (argc || !url || !path)
>                 usage_with_options(git_submodule_helper_usage,
>                                    module_clone_options);
>
> --
> 2.7.1.429.g45cd78e
>