Re: [PATCHv2 2/5] submodule--helper clone: simplify path check

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 31, 2016, 21:23 UTC
Message-ID: <CAPig+cSk2kDEs7nCt39LoeBjjmo-+7WH4rNNh0x6ULfeeAwQ_w@mail.gmail.com>
In-Reply-To: <1459458280-17619-3-git-send-email-sbeller@google.com>

On Thu, Mar 31, 2016 at 5:04 PM, Stefan Beller <sbeller@google.com> wrote:

> submodule--helper clone: simplify path check

I don't see anything in the patch which simplifies a path check. Instead, this version of the patch is now fixing a potential NULL-dereference.

> The calling shell code makes sure that `path` is non null and non empty.
> Side note: it cannot be null as just three lines before it is passed
> to safe_create_leading_directories_const which would crash as you feed
> it null. That means the `else` part is dead code, so remove it.

The above description has very little if anything to do with what this patch is addressing considering that this is now a crash-fix patch. While it's true that the (current) shell code won't trigger this crash, that's not particularly interesting or relevant.

> To make the code futureproof, add a check for path being NULL or empty
> and report the error accordingly.

Selling this as a future-proofing change is misleading; it's a crash fix, plain and simple. I think the entire commit message could be collapsed to something like this:

    submodule--helper: fix potential NULL-dereference

    Don't dereference NULL 'path' if it was never assigned. While
    here also protect against empty --path argument.

You don't even need to mention removal of the conditional in the second hunk since, for anyone reading the patch, that's an obvious consequence of adding the new check in the first hunk.

The patch itself looks fine.

Show 27 quoted lines

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> @@ -194,6 +194,9 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>         argc = parse_options(argc, argv, prefix, module_clone_options,
>                              git_submodule_helper_usage, 0);
>
> +       if (!path || !*path)
> +               die(_("submodule--helper: unspecified or empty --path"));
> +
>         strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
>         sm_gitdir = strbuf_detach(&sb, NULL);
>
> @@ -215,10 +218,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>         if (safe_create_leading_directories_const(path) < 0)
>                 die(_("could not create directory '%s'"), path);
>
> -       if (path && *path)
> -               strbuf_addf(&sb, "%s/.git", path);
> -       else
> -               strbuf_addstr(&sb, ".git");
> +       strbuf_addf(&sb, "%s/.git", path);
>
>         if (safe_create_leading_directories_const(sb.buf) < 0)
>                 die(_("could not create leading directories of '%s'"), sb.buf);
> --
> 2.5.0.264.g39f00fe