Re: [PATCH v5] submodule: port subcommand 'set-url' from shell to C

Denton Liu [off-list ref] writes:

Hi Shourya,

It looks good to me except for one tiny nit:

On Fri, May 08, 2020 at 11:51:36AM +0530, Shourya Shukla wrote:

quoted

Convert submodule subcommand 'set-url' to a builtin. Port 'set-url' to
'submodule--helper.c' and call the latter via 'git-submodule.sh'.

Signed-off-by: Shourya Shukla <redacted>
---
 builtin/submodule--helper.c | 37 +++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 22 +---------------------
 2 files changed, 38 insertions(+), 21 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1a4b391c88..8bc7b4cfa6 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c

@@ -2246,6 +2246,42 @@ static int module_config(int argc, const char **argv, const char *prefix)
 	usage_with_options(git_submodule_helper_usage, module_config_options);
 }
 
+static int module_set_url(int argc, const char **argv, const char *prefix)
+{
+	int quiet = 0;
+	const char *newurl;
+	const char *path;
+	char* config_name;

The asterisk should be stuck with the name, not the type, similar to how
you wrote it above.

Right.

quoted

+
+	struct option options[] = {
+		OPT__QUIET(&quiet, N_("Suppress output for setting url of a submodule")),
+		OPT_END()
+	};
+	const char *const usage[] = {
+		N_("git submodule--helper set-url [--quiet] <path> <newurl>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	path = argv[0];
+	newurl = argv[1];
+
+	if (argc != 2 || !path || !newurl) {

Checking argc at this point is too late to protect against the
potential out-of-bounds access we have already made to argv[0]
and argv[1].