[PATCH] bundle-uri: validate that bundle entries have a uri

[PATCH] bundle-uri: validate that bundle entries have a uri

[Sam Bostock via GitGitGadget]

From: Sam Bostock <sam.bostock@shopify.com>

When a bundle list config file has a typo like 'url' instead of 'uri',
or simply omits the uri field, the bundle entry is created but
bundle->uri remains NULL. This causes a segfault when copy_uri_to_file()
passes the NULL to starts_with().

Signed-off-by: Sam Bostock <sam@sambostock.ca>
---
    bundle-uri: validate that bundle entries have a uri

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2134%2Fsambostock%2Fvalidate-bundle-uri-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2134/sambostock/validate-bundle-uri-v1
Pull-Request: https://github.com/git/git/pull/2134

 bundle-uri.c                | 22 +++++++++++++++++++++-
 t/t5750-bundle-uri-parse.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/bundle-uri.c b/bundle-uri.c
index 57cccfc6b8..022e2109a6 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -89,7 +89,8 @@ static int summarize_bundle(struct remote_bundle_info *info, void *data)
 {
 	FILE *fp = data;
 	fprintf(fp, "[bundle \"%s\"]\n", info->id);
-	fprintf(fp, "\turi = %s\n", info->uri);
+	if (info->uri)
+		fprintf(fp, "\turi = %s\n", info->uri);
 
 	if (info->creationToken)
 		fprintf(fp, "\tcreationToken = %"PRIu64"\n", info->creationToken);
@@ -267,6 +268,19 @@ int bundle_uri_parse_config_format(const char *uri,
 		result = 1;
 	}
 
+	if (!result) {
+		struct hashmap_iter iter;
+		struct remote_bundle_info *bundle;
+
+		hashmap_for_each_entry(&list->bundles, &iter, bundle, ent) {
+			if (!bundle->uri) {
+				error(_("bundle list at '%s': bundle '%s' has no uri"),
+				      uri, bundle->id ? bundle->id : "<unknown>");
+				result = 1;
+			}
+		}
+	}
+
 	return result;
 }
 
@@ -751,6 +765,12 @@ static int fetch_bundle_uri_internal(struct repository *r,
 		return -1;
 	}
 
+	if (!bundle->uri) {
+		error(_("bundle '%s' has no uri"),
+		      bundle->id ? bundle->id : "<unknown>");
+		return -1;
+	}
+
 	if (!bundle->file &&
 	    !(bundle->file = find_temp_filename())) {
 		result = -1;
diff --git a/t/t5750-bundle-uri-parse.sh b/t/t5750-bundle-uri-parse.sh
index 80a3f83ffb..294f9d9c64 100755
--- a/t/t5750-bundle-uri-parse.sh
+++ b/t/t5750-bundle-uri-parse.sh
@@ -286,4 +286,30 @@ test_expect_success 'parse config format edge cases: creationToken heuristic' '
 	grep "could not parse bundle list key creationToken with value '\''bogus'\''" err
 '
 
+test_expect_success 'parse config format: bundle with missing uri' '
+	cat >input <<-\EOF &&
+	[bundle]
+		version = 1
+		mode = all
+	[bundle "missing-uri"]
+		creationToken = 1
+	EOF
+
+	test_must_fail test-tool bundle-uri parse-config input 2>err &&
+	grep "bundle '\''missing-uri'\'' has no uri" err
+'
+
+test_expect_success 'parse config format: bundle with url instead of uri' '
+	cat >input <<-\EOF &&
+	[bundle]
+		version = 1
+		mode = all
+	[bundle "typo"]
+		url = https://example.com/bundle.bdl
+	EOF
+
+	test_must_fail test-tool bundle-uri parse-config input 2>err &&
+	grep "bundle '\''typo'\'' has no uri" err
+'
+
 test_done

base-commit: c4a0c8845e2426375ad257b6c221a3a7d92ecfda
-- 
gitgitgadget

Re: [PATCH] bundle-uri: validate that bundle entries have a uri

[Junio C Hamano]

"Sam Bostock via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  bundle-uri.c                | 22 +++++++++++++++++++++-
>  t/t5750-bundle-uri-parse.sh | 26 ++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/bundle-uri.c b/bundle-uri.c
> index 57cccfc6b8..022e2109a6 100644
> --- a/bundle-uri.c
> +++ b/bundle-uri.c
> @@ -89,7 +89,8 @@ static int summarize_bundle(struct remote_bundle_info *info, void *data)
>  {
>  	FILE *fp = data;
>  	fprintf(fp, "[bundle \"%s\"]\n", info->id);
> -	fprintf(fp, "\turi = %s\n", info->uri);
> +	if (info->uri)
> +		fprintf(fp, "\turi = %s\n", info->uri);

All the other code paths error out when info->uri is missing; I can
understand that print_bundle_list() want to keep going as it is
primarily for debugging, but then don't we want to more loudly
report that a mandatory thing info->uri is missing, rather than a
subtle hint that is lack of expected line that shows "uri = ..."?

[PATCH v2] bundle-uri: validate that bundle entries have a uri

[Sam Bostock via GitGitGadget]

From: Sam Bostock <sam.bostock@shopify.com>

When a bundle list config file has a typo like 'url' instead of 'uri',
or simply omits the uri field, the bundle entry is created but
bundle->uri remains NULL. This causes a segfault when copy_uri_to_file()
passes the NULL to starts_with().

Signed-off-by: Sam Bostock <sam@sambostock.ca>
---
    bundle-uri: validate that bundle entries have a uri
    
    Changes since v1:
    
     * Updated summarize_bundle() to print # uri = (missing) as a comment
       instead of silently omitting the line.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2134%2Fsambostock%2Fvalidate-bundle-uri-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2134/sambostock/validate-bundle-uri-v2
Pull-Request: https://github.com/git/git/pull/2134

Range-diff vs v1:

 1:  3d8a014490 ! 1:  fb66352093 bundle-uri: validate that bundle entries have a uri
     @@ bundle-uri.c: static int summarize_bundle(struct remote_bundle_info *info, void
      -	fprintf(fp, "\turi = %s\n", info->uri);
      +	if (info->uri)
      +		fprintf(fp, "\turi = %s\n", info->uri);
     ++	else
     ++		fprintf(fp, "\t# uri = (missing)\n");
       
       	if (info->creationToken)
       		fprintf(fp, "\tcreationToken = %"PRIu64"\n", info->creationToken);


 bundle-uri.c                | 24 +++++++++++++++++++++++-
 t/t5750-bundle-uri-parse.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/bundle-uri.c b/bundle-uri.c
index 57cccfc6b8..3b2e347288 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -89,7 +89,10 @@ static int summarize_bundle(struct remote_bundle_info *info, void *data)
 {
 	FILE *fp = data;
 	fprintf(fp, "[bundle \"%s\"]\n", info->id);
-	fprintf(fp, "\turi = %s\n", info->uri);
+	if (info->uri)
+		fprintf(fp, "\turi = %s\n", info->uri);
+	else
+		fprintf(fp, "\t# uri = (missing)\n");
 
 	if (info->creationToken)
 		fprintf(fp, "\tcreationToken = %"PRIu64"\n", info->creationToken);
@@ -267,6 +270,19 @@ int bundle_uri_parse_config_format(const char *uri,
 		result = 1;
 	}
 
+	if (!result) {
+		struct hashmap_iter iter;
+		struct remote_bundle_info *bundle;
+
+		hashmap_for_each_entry(&list->bundles, &iter, bundle, ent) {
+			if (!bundle->uri) {
+				error(_("bundle list at '%s': bundle '%s' has no uri"),
+				      uri, bundle->id ? bundle->id : "<unknown>");
+				result = 1;
+			}
+		}
+	}
+
 	return result;
 }
 
@@ -751,6 +767,12 @@ static int fetch_bundle_uri_internal(struct repository *r,
 		return -1;
 	}
 
+	if (!bundle->uri) {
+		error(_("bundle '%s' has no uri"),
+		      bundle->id ? bundle->id : "<unknown>");
+		return -1;
+	}
+
 	if (!bundle->file &&
 	    !(bundle->file = find_temp_filename())) {
 		result = -1;
diff --git a/t/t5750-bundle-uri-parse.sh b/t/t5750-bundle-uri-parse.sh
index 80a3f83ffb..294f9d9c64 100755
--- a/t/t5750-bundle-uri-parse.sh
+++ b/t/t5750-bundle-uri-parse.sh
@@ -286,4 +286,30 @@ test_expect_success 'parse config format edge cases: creationToken heuristic' '
 	grep "could not parse bundle list key creationToken with value '\''bogus'\''" err
 '
 
+test_expect_success 'parse config format: bundle with missing uri' '
+	cat >input <<-\EOF &&
+	[bundle]
+		version = 1
+		mode = all
+	[bundle "missing-uri"]
+		creationToken = 1
+	EOF
+
+	test_must_fail test-tool bundle-uri parse-config input 2>err &&
+	grep "bundle '\''missing-uri'\'' has no uri" err
+'
+
+test_expect_success 'parse config format: bundle with url instead of uri' '
+	cat >input <<-\EOF &&
+	[bundle]
+		version = 1
+		mode = all
+	[bundle "typo"]
+		url = https://example.com/bundle.bdl
+	EOF
+
+	test_must_fail test-tool bundle-uri parse-config input 2>err &&
+	grep "bundle '\''typo'\'' has no uri" err
+'
+
 test_done

base-commit: c4a0c8845e2426375ad257b6c221a3a7d92ecfda
-- 
gitgitgadget