fetch: fix wrong evaluation order in URL trailing-slash trimming
PATCH 1/1 fetch: fix wrong evaluation order in URL trailing-slash trimming
From: cuiweixie <[email protected]>
if i == -1, url[i] will be UB.
---
fetch: fix wrong evaluation order in URL trailing-slash trimming
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2211%2Fcuiweixie%2Fbugfix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2211/cuiweixie/bugfix-v1
Pull-Request: https://github.com/git/git/pull/2211
builtin/fetch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/fetch.c +1 -1
@@ -722,7 +722,7 @@ static void display_state_init(struct display_state *display_state, struct ref * display_state->url =xstrdup ("foreign" ); display_state->url_len =strlen (display_state->url); -for (i = display_state->url_len -1 ; display_state->url[i] =='/' &&0 <= i; i--) +for (i = display_state->url_len -1 ;0 <= i && display_state->url[i] =='/' ; i--) ; display_state->url_len = i +1 ;if (4 < i && !strncmp (".git" , display_state->url + i -3 ,4 ))
base-commit: 7c02d39fc2ed2702223c7674f73150d9a7e61ba4
Signature · 1 line
gitgitgadget
builtin/fetch.c static void display_state_init(struct display_state *display_state, struct ref * · line 728
Quoted 8 lines · cui via GitGitGadget
if i == -1, url[i] will be UB.
[...]builtin/fetch.c +1
-1display_state->url_len =strlen (display_state->url); -for (i = display_state->url_len -1 ; display_state->url[i] =='/' &&0 <= i; i--) +for (i = display_state->url_len -1 ;0 <= i && display_state->url[i] =='/' ; i--) ; display_state->url_len = i +1 ;if (4 < i && !strncmp (".git" , display_state->url + i -3 ,4 ))
Yeah, the original is obviously nonsense. Probably it is not worth too much effort to add a test here, but I wondered if this can even be triggered in practice.
We would hit it when there is no non-slash character in the URL. I'm not sure it's possible to get this far with that, but it makes sense to me to write it as you have.
I can't help but think this would be easier to read without an empty loop body, like:
for (len = strlen(display_state->url); len > 0; len--) {
if (display_state->url[len-1] != '/')
break;}
which makes it much more clear we never leave the bounds of the string (and also works with a size_t, which is a more appropriate type than int here).
But it may not be worth polishing this bit of code too much (if we did, I'd also suggest strip_suffix_mem() to drop ".git" rather than all of those magic numbers. Or even stuffing it in a strbuf and using strbuf_setlen() and strbuf_strip_suffix().
But anyway, your patch seems like an obvious improvement in the meantime.
-Peff
Quoted 11 lines · Jeff King
I can't help but think this would be easier to read without an empty loop body, like:
for (len = strlen(display_state->url); len > 0; len--) { if (display_state->url[len-1] != '/') break;}
which makes it much more clear we never leave the bounds of the string (and also works with a
size_t, which is a more appropriate type than int here).
Yes, this is vastly more readable, even though what it does is exactly the same as the original.
Or instead of having strlen() to scan the entire string once and then ourselves scan backwards from the end, scan forward ourselves only once while noting where the last non-slash byte was, or something.
Quoted 4 lines
But it may not be worth polishing this bit of code too much (if we did, I'd also suggest
strip_suffix_mem()to drop ".git" rather than all of those magic numbers. Or even stuffing it in a strbuf and usingstrbuf_setlen()andstrbuf_strip_suffix().
True. None of these "we could do it this way too" bikeshedding has much value. The patch posted is an obvious and trivial enough improvement.
Thanks.