wt-status: use strncmp() for length-limited string comparison
PATCH 1/1 wt-status: use strncmp() for length-limited string comparison
When a branch name is longer than four characters, memcmp() can read past the end of the string literal "HEAD". Use strncmp() instead, which stops at the end of a string. This fixes the following test failures with AddressSanitizer:
t3203-branch-output.sh (Wstat: 256 Tests: 18 Failed: 4)
Failed tests: 12, 15-17
Non-zero exit status: 1t3412-rebase-root.sh (Wstat: 256 Tests: 31 Failed: 3)
Failed tests: 28-29, 31
Non-zero exit status: 1t3507-cherry-pick-conflict.sh (Wstat: 256 Tests: 31 Failed: 4)
Failed tests: 14, 29-31
Non-zero exit status: 1t3510-cherry-pick-sequence.sh (Wstat: 256 Tests: 39 Failed: 14)
Failed tests: 17, 22-26, 28-30, 34-35, 37-39 Non-zero exit status: 1t3420-rebase-autostash.sh (Wstat: 256 Tests: 28 Failed: 4)
Failed tests: 24-27
Non-zero exit status: 1t3404-rebase-interactive.sh (Wstat: 256 Tests: 91 Failed: 57)
Failed tests: 17, 19, 21-42, 44, 46-74, 77, 81-82 Non-zero exit status: 1t3900-i18n-commit.sh (Wstat: 256 Tests: 34 Failed: 1)
Failed test: 34
Non-zero exit status: 1t5407-post-rewrite-hook.sh (Wstat: 256 Tests: 14 Failed: 6)
Failed tests: 9-14
Non-zero exit status: 1t7001-mv.sh (Wstat: 256 Tests: 46 Failed: 5)
Failed tests: 39-43
Non-zero exit status: 1t7509-commit.sh (Wstat: 256 Tests: 12 Failed: 2)
Failed tests: 11-12
Non-zero exit status: 1t7512-status-help.sh (Wstat: 256 Tests: 39 Failed: 35)
Failed tests: 5-39
Non-zero exit status: 1t6030-bisect-porcelain.sh (Wstat: 256 Tests: 70 Failed: 1)
Failed test: 13
Non-zero exit status: 1
--- wt-status.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
wt-status.c +1 -1
@@ -1319,7 +1319,7 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,hashcpy (cb->nsha1, nsha1);for (end = target; *end && *end !='\n' ; end++) ; -if (!memcmp (target,"HEAD" , end - target)) { +if (!strncmp (target,"HEAD" , end - target)) {/* HEAD is relative. Resolve it to the right reflog entry. */ strbuf_addstr (&cb->buf,find_unique_abbrev (nsha1,DEFAULT_ABBREV ));
Signature · 1 line
2.6.3
Quoted 4 lines · René Scharfe
When a branch name is longer than four characters,
memcmp()can read past the end of the string literal "HEAD". Usestrncmp()instead, which stops at the end of a string. This fixes the following test failures with AddressSanitizer:
Hmm. I think this is mostly harmless, as a comparison like:
memcmp("HEAD and more", "HEAD", strlen("HEAD"))
would yield non-zero when we compare the NUL in the second string to whatever is in the first. So I assume what is going on is that memcmp is doing larger compares than byte by byte, and is examining 4 or 8 bytes starting at that NUL.
The outcome is equivalent, but we do touch memory that is not ours, so I think this is a positive direction in that sense.
But...
wt-status.c static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1, · line 1322
Quoted 10 lines
wt-status.c +1
-1@@ -1319,7 +1319,7 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,hashcpy (cb->nsha1, nsha1);for (end = target; *end && *end !='\n' ; end++) ; -if (!memcmp (target,"HEAD" , end - target)) { +if (!strncmp (target,"HEAD" , end - target)) {
This will match prefixes like "HEA" in the target, won't it?
I think you want something more like:
if (end - target == 4 && !memcmp(target, "HEAD", 4))
I tried to think of a way that didn't involve a magic number. The best I came up with is:
if (skip_prefix(target, "HEAD", &v) && v == end)
but that requires an extra variable, and is arguably more obfuscated.
-Peff
Am 24.11.2015 um 22:36 schrieb Jeff King:
Quoted 11 lines · René Scharfe
Quoted 5 lines
When a branch name is longer than four characters,
memcmp()can read past the end of the string literal "HEAD". Usestrncmp()instead, which stops at the end of a string. This fixes the following test failures with AddressSanitizer:Hmm. I think this is mostly harmless, as a comparison like:
memcmp("HEAD and more", "HEAD", strlen("HEAD"))
Quoted 7 lines
would yield non-zero when we compare the NUL in the second string to whatever is in the first. So I assume what is going on is that memcmp is doing larger compares than byte by byte, and is examining 4 or 8 bytes starting at that NUL.
The outcome is equivalent, but we do touch memory that is not ours, so I think this is a positive direction in that sense.
Yes, except it should be strlen("HEAD and more") in your example code; with strlen("HEAD") it would compare just 4 bytes and return 0.
Quoted 14 lines
But...
wt-status.c static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1, · line 1322
Quoted 10 lines
wt-status.c +1
-1@@ -1319,7 +1319,7 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,hashcpy (cb->nsha1, nsha1);for (end = target; *end && *end !='\n' ; end++) ; -if (!memcmp (target,"HEAD" , end - target)) { +if (!strncmp (target,"HEAD" , end - target)) {This will match prefixes like "HEA" in the target, won't it?
Oww, yes. :-/ NB: The existing code does the same.
Quoted 10 lines
I think you want something more like:
if (end - target == 4 && !memcmp(target, "HEAD", 4))I tried to think of a way that didn't involve a magic number. The best I came up with is:
if (skip_prefix(target, "HEAD", &v) && v == end)but that requires an extra variable, and is arguably more obfuscated.
Using one more variable isn't that bad, as long as it gets a fitting name. Or we could reuse "end" (I'm not worrying about scanning "HEAD" twice very much):
wt-status.c +3 -3
@@ -1317,14 +1317,14 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1, target +=strlen (" to " );strbuf_reset (&cb->buf);hashcpy (cb->nsha1, nsha1); -for (end = target; *end && *end !='\n' ; end++) - ; -if (!memcmp (target,"HEAD" , end - target)) { +if (skip_prefix (target,"HEAD" , &end) && (!*end || *end =='\n' )) {/* HEAD is relative. Resolve it to the right reflog entry. */ strbuf_addstr (&cb->buf,find_unique_abbrev (nsha1,DEFAULT_ABBREV ));return 1 ; } +for (end = target; *end && *end !='\n' ; end++) + ;strbuf_add (&cb->buf, target, end - target);return 1 ; }
Quoted 7 lines · René Scharfe
Quoted 3 lines
Hmm. I think this is mostly harmless, as a comparison like:
memcmp("HEAD and more", "HEAD", strlen("HEAD"))[...]
Yes, except it should be
strlen("HEAD and more")in your example code; withstrlen("HEAD")it would compare just 4 bytes and return 0.
Whoops, yeah. Thank you for figuring out what I meant. :)
Quoted 11 lines
Using one more variable isn't that bad, as long as it gets a fitting name. Or we could reuse "end" (I'm not worrying about scanning "HEAD" twice very much):
wt-status.c +0
-0@@ -1317,14 +1317,14 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1, target +=strlen (" to " );strbuf_reset (&cb->buf);
wt-status.c static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1, · line 1325
Quoted 10 lines
wt-status.c +1
-3hashcpy (cb->nsha1, nsha1); -for (end = target; *end && *end !='\n' ; end++) - ; -if (!memcmp (target,"HEAD" , end - target)) { +if (skip_prefix (target,"HEAD" , &end) && (!*end || *end =='\n' )) {/* HEAD is relative. Resolve it to the right reflog entry. */ strbuf_addstr (&cb->buf,find_unique_abbrev (nsha1,DEFAULT_ABBREV ));return 1 ; }
Yeah, I think parsing left-to-right like this makes things much more obvious. And regarding scanning HEAD twice, I think we already do that (we find the trailing newline first in the current code). Though I agree that is absurd premature optimization.
wt-status.c static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1, · line 1327
Quoted 2 lines
wt-status.c +2
-0+for (end = target; *end && *end !='\n' ; end++) + ;
This loop (which I know you just moved, not wrote) is basically strchrnul, isn't it? That might be more readable.
-Peff
PATCH v2 1/1 wt-status: correct and simplify check for detached HEAD
If a branch name is longer than four characters then memcmp() reads over the end of the static string "HEAD". This causes the following test failures with AddressSanitizer:
t3203-branch-output.sh (Wstat: 256 Tests: 18 Failed: 4)
Failed tests: 12, 15-17
Non-zero exit status: 1t3412-rebase-root.sh (Wstat: 256 Tests: 31 Failed: 3)
Failed tests: 28-29, 31
Non-zero exit status: 1t3507-cherry-pick-conflict.sh (Wstat: 256 Tests: 31 Failed: 4)
Failed tests: 14, 29-31
Non-zero exit status: 1t3510-cherry-pick-sequence.sh (Wstat: 256 Tests: 39 Failed: 14)
Failed tests: 17, 22-26, 28-30, 34-35, 37-39 Non-zero exit status: 1t3420-rebase-autostash.sh (Wstat: 256 Tests: 28 Failed: 4)
Failed tests: 24-27
Non-zero exit status: 1t3404-rebase-interactive.sh (Wstat: 256 Tests: 91 Failed: 57)
Failed tests: 17, 19, 21-42, 44, 46-74, 77, 81-82 Non-zero exit status: 1t3900-i18n-commit.sh (Wstat: 256 Tests: 34 Failed: 1)
Failed test: 34
Non-zero exit status: 1t5407-post-rewrite-hook.sh (Wstat: 256 Tests: 14 Failed: 6)
Failed tests: 9-14
Non-zero exit status: 1t7001-mv.sh (Wstat: 256 Tests: 46 Failed: 5)
Failed tests: 39-43
Non-zero exit status: 1t7509-commit.sh (Wstat: 256 Tests: 12 Failed: 2)
Failed tests: 11-12
Non-zero exit status: 1t7512-status-help.sh (Wstat: 256 Tests: 39 Failed: 35)
Failed tests: 5-39
Non-zero exit status: 1t6030-bisect-porcelain.sh (Wstat: 256 Tests: 70 Failed: 1)
Failed test: 13
Non-zero exit status: 1
And if a branch is named "H", "HE", or "HEA" then the current if clause erroneously considers it as matching "HEAD" because it only compares up to the end of the branch name.
Fix that by doing the comparison using strcmp() and only after the branch name is extracted. This way neither too less nor too many characters are checked. While at it call strchrnul() to find the end of the branch name instead of open-coding it.
---
We can be more careful when parsing -- or avoid parsing and backtrack if we found "HEAD" after all. The latter is simpler, and string parsing is tricky enough that we better take such opportunities to simplify the code..
Changes since v1:
* strcmp() instead of strncmp()
* strchrnul() (unrelated cleanup)
* adjusted subject (and commit message)
wt-status.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
wt-status.c +4 -5
@@ -1317,15 +1317,14 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1, target +=strlen (" to " );strbuf_reset (&cb->buf);hashcpy (cb->nsha1, nsha1); -for (end = target; *end && *end !='\n' ; end++) - ; -if (!memcmp (target,"HEAD" , end - target)) { + end =strchrnul (target,'\n' ); +strbuf_add (&cb->buf, target, end - target); +if (!strcmp (cb->buf.buf,"HEAD" )) {/* HEAD is relative. Resolve it to the right reflog entry. */ +strbuf_reset (&cb->buf);strbuf_addstr (&cb->buf,find_unique_abbrev (nsha1,DEFAULT_ABBREV )); -return 1 ; } -strbuf_add (&cb->buf, target, end - target);return 1 ; }
Signature · 1 line
2.6.3
Re: [PATCH v2] wt-status: correct and simplify check for detached HEAD
Quoted 13 lines · René Scharfe
wt-status.c +2
-3@@ -1317,15 +1317,14 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1, target +=strlen (" to " );strbuf_reset (&cb->buf);hashcpy (cb->nsha1, nsha1); -for (end = target; *end && *end !='\n' ; end++) - ; -if (!memcmp (target,"HEAD" , end - target)) { + end =strchrnul (target,'\n' ); +strbuf_add (&cb->buf, target, end - target);
wt-status.c static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1, · line 1329
Quoted 10 lines
wt-status.c +2
-2+if (!strcmp (cb->buf.buf,"HEAD" )) {/* HEAD is relative. Resolve it to the right reflog entry. */ +strbuf_reset (&cb->buf);strbuf_addstr (&cb->buf,find_unique_abbrev (nsha1,DEFAULT_ABBREV )); -return 1 ; } -strbuf_add (&cb->buf, target, end - target);return 1 ; }
Indeed, the code is much simpler like this than with the previous attempts. Looks all right to me.
Signature · 2 lines
Matthieu Moy http://www-verimag.imag.fr/~moy/
Re: [PATCH v2] wt-status: correct and simplify check for detached HEAD
In reply to René ScharfeQuoted 5 lines · René Scharfe
Fix that by doing the comparison using
strcmp()and only after the branch name is extracted. This way neither too less nor too many characters are checked. While at it callstrchrnul()to find the end of the branch name instead of open-coding it.
Quoted 7 lines
Helped-by Jeff King <[email protected]>Signed-off-by Rene Scharfe <[email protected]>---
We can be more careful when parsing -- or avoid parsing and backtrack if we found "HEAD" after all. The latter is simpler, and string parsing is tricky enough that we better take such opportunities to simplify the code..
Yeah, I think this is nicer. We end up allocating either way anyway, so the only extra effort is copy the string.
-Peff
Quoted 12 lines · Jeff King
Quoted 12 lines
wt-status.c +1
-3@@ -1317,14 +1317,14 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1, target +=strlen (" to " );strbuf_reset (&cb->buf);hashcpy (cb->nsha1, nsha1); -for (end = target; *end && *end !='\n' ; end++) - ; -if (!memcmp (target,"HEAD" , end - target)) { +if (skip_prefix (target,"HEAD" , &end) && (!*end || *end =='\n' )) {
Quoted 8 lines
wt-status.c static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1, · line 1325
Quoted 5 lines
wt-status.c +0
-0/* HEAD is relative. Resolve it to the right reflog entry. */ strbuf_addstr (&cb->buf,find_unique_abbrev (nsha1,DEFAULT_ABBREV ));return 1 ; }Yeah, I think parsing left-to-right like this makes things much more obvious.
Agreed.
Quoted 5 lines
wt-status.c static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1, · line 1327
Quoted 2 lines
wt-status.c +2
-0+for (end = target; *end && *end !='\n' ; end++) + ;This loop (which I know you just moved, not wrote) is basically strchrnul, isn't it? That might be more readable.
Agreed too.
Signature · 2 lines
Matthieu Moy http://www-verimag.imag.fr/~moy/