Re: [PATCH 10/11] relative_url(): fix incorrect condition

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 16, 2022, 13:09 UTC
Message-ID: <220616.86k09gwx6i.gmgdl@evledraar.gmail.com>
In-Reply-To: <0bf70e65d2c9e187203a77088ff0f7d18510caca.1655336146.git.gitgitgadget@gmail.com>

On Wed, Jun 15 2022, Johannes Schindelin via GitGitGadget wrote:

Show 8 quoted lines

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 63e95beb085c (submodule: port resolve_relative_url from shell to C,
> 2016-04-15), we added a loop over `url` where we are looking for `../`
> or `./` components.
>
> The loop condition we used is the pointer `url` itself, which is clearly
> not what we wanted.

Clearly, but having looked at this I think it's useful to note that coverity must be (I don't know what it actually emitted) be complaining about the "url" condition being useless, i.e. it's the same as a "while (1)", not that we run off the end of the string.

I.e. due to the way those start_with*() functions work we would abort early if we were running off th end of the string.

Show 20 quoted lines

> Pointed out by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  remote.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/remote.c b/remote.c
> index 9b9bbfe80ec..bded6acbfe8 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -2846,7 +2846,7 @@ char *relative_url(const char *remote_url, const char *url,
>  	 * When the url starts with '../', remove that and the
>  	 * last directory in remoteurl.
>  	 */
> -	while (url) {
> +	while (*url) {
>  		if (starts_with_dot_dot_slash_native(url)) {
>  			url += 3;
>  			colonsep |= chop_last_dir(&remoteurl, is_relative);

Which I tested with this:
	
	diff --git a/remote.c b/remote.c
	index 9b9bbfe80ec..e049bbb791c 100644
	--- a/remote.c
	+++ b/remote.c
	@@ -2846,14 +2846,17 @@ char *relative_url(const char *remote_url, const char *url,
	 	 * When the url starts with '../', remove that and the
	 	 * last directory in remoteurl.
	 	 */
	-	while (url) {
	+	while (1) {
	 		if (starts_with_dot_dot_slash_native(url)) {
	 			url += 3;
	 			colonsep |= chop_last_dir(&remoteurl, is_relative);
	-		} else if (starts_with_dot_slash_native(url))
	+		} else if (starts_with_dot_slash_native(url)) {
	 			url += 2;
	-		else
	-			break;
	+		} else if (!*url) {
	+			BUG("ran off the end of our url?");
	+		} else {
	+			break; 
	+		}
	 	}
	 	strbuf_reset(&sb);
	 	strbuf_addf(&sb, "%s%s%s", remoteurl, colonsep ? ":" : "/", url);
	
Which will fail e.g. on this test in t3420-rebase-autostash.sh:

	+ git submodule add ./ sub
	BUG: remote.c:2856: ran off the end of our url?
	Aborted

I worried a bit about this since we released this with v2.9.0, so in all this time we haven't been infinitely looping on this case, or at least haven't had any reports about that.

So if we hadn't been catching this in starts_with_*() I wouldn't be confident that this was the correct fix, yes it's almost certainly not what not what was intended, but if we change it to that are we confident that the side-effects are going to be what we want?

But in this case I'm pretty sure that the behavior before/after will be the same, and that the only change will be to make coverity happier, and the code less confusing.