syscalls/renameat202: initialize str with zero
PATCH 1/1 syscalls/renameat202: initialize str with zero
In renameat2_verify(), str is allocated on stack with size 8 (sizeof content) and filled with random data. Then file content (7 bytes) is read into str and the last byte in str is left unchanged with garbage, and test fails if that last byte is not zero.
Fix it by initializing the str with zeros, make sure str is null terminated as long as correct data has been read in.
Also remove the "str[strlen(content)] == '\0'" check, which is not necessary, because strcmp will catch the failure anyway.
Also improve the failure message to print more useful debug message.
--- testcases/kernel/syscalls/renameat2/renameat202.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
testcases/kernel/syscalls/renameat2/renameat202.c +9 -8
@@ -118,7 +118,7 @@ static void cleanup(void)static void renameat2_verify (void ) { -char str[sizeof (content)]; +char str[sizeof (content)] = {0 };struct stat st;char *emptyfile;char *contentfile;
@@ -152,11 +152,12 @@ static void renameat2_verify(void)tst_brkm (TERRNO |TFAIL , cleanup,"close fd failed" ); fd =0 ; -if (str[strlen (content)] =='\0' && !strcmp (content, str) - && !st.st_size) -tst_resm (TPASS , -"renameat2() swapped the content of the two files" ); -else -tst_resm (TFAIL , -"renameat2() didn't swap the content of the two files" ); +if (strcmp (content, str)) { +tst_resm (TFAIL ,"file content changed after renameat2()" ); +tst_resm (TFAIL ,"expect \"%s\", got \"%s\"" , content, str); + }else if (st.st_size) { +tst_resm (TFAIL ,"emptyfile has non-zero file size" ); + }else { +tst_resm (TPASS ,"renameat2() test passed" ); + } }
Signature · 1 line
2.5.0
PATCH 1/1 syscalls/renameat202: initialize str with zero
Hi!
Quoted 4 lines
In
renameat2_verify(), str is allocated on stack with size 8 (sizeof content) and filled with random data. Then file content (7 bytes) is read into str and the last byte in str is left unchanged with garbage, and test fails if that last byte is not zero.
Good catch.
Quoted 2 lines
Fix it by initializing the str with zeros, make sure str is null terminated as long as correct data has been read in.
Looking at the code it may be easier to use write() to write the string including the null byte in the setup. But either one is fine.
Quoted 2 lines
Also remove the "str[strlen(content)] == '\0'" check, which is not necessary, because strcmp will catch the failure anyway.
This has been added there on purpose so that we are sure the test wouldn't segfault in case that the str is not null terminated which is unlikely but still possible. And it's still possible even when you zero the str buffer since the read may overwrite whole str and passing non-null terminated buffer to strcmp() is still undefined operation.
Maybe it would be much better to try to read a buffer larger than the expected content, check that the value from read was size of the data and then do memcmp() with explicit size.
BTW: The size of the str should be increased anyway since the read gets
strlen(content) + 10 as a size parameter which is larger than sizeof(str). We should just change it to some arbitrary large value like 128.
Quoted 24 lines
Also improve the failure message to print more useful debug message.
Signed-off-by Eryu Guan <[email protected]>--- testcases/kernel/syscalls/renameat2/renameat202.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)testcases/kernel/syscalls/renameat2/renameat202.c +1
-1@@ -118,7 +118,7 @@ static void cleanup(void)static void renameat2_verify (void ) { -char str[sizeof (content)]; +char str[sizeof (content)] = {0 };struct stat st;char *emptyfile;char *contentfile;@@ -152,11 +152,12 @@ static void renameat2_verify(void)tst_brkm (TERRNO |TFAIL , cleanup,"close fd failed" ); fd =0 ;
testcases/kernel/syscalls/renameat2/renameat202.c static void renameat2_verify(void) · line 157
Quoted 10 lines
testcases/kernel/syscalls/renameat2/renameat202.c +3
-7-if (str[strlen (content)] =='\0' && !strcmp (content, str) - && !st.st_size) -tst_resm (TPASS , -"renameat2() swapped the content of the two files" ); -else -tst_resm (TFAIL , -"renameat2() didn't swap the content of the two files" ); +if (strcmp (content, str)) { +tst_resm (TFAIL ,"file content changed after renameat2()" ); +tst_resm (TFAIL ,"expect \"%s\", got \"%s\"" , content, str);
This adds two FAIL statements in the output for one failure. Either print the message in one tst_resm() statement or change the second one to TINFO.
testcases/kernel/syscalls/renameat2/renameat202.c static void renameat2_verify(void) · line 159
Quoted 2 lines
testcases/kernel/syscalls/renameat2/renameat202.c +2
-0+ }else if (st.st_size) { +tst_resm (TFAIL ,"emptyfile has non-zero file size" );
You should do return in the previous if() so that you don't have to do the else if () spagetthi code here.
And if you do return here the TPASS message does not have to be in the else block as well.
Quoted 4 lines
testcases/kernel/syscalls/renameat2/renameat202.c +3
-0+ }else { +tst_resm (TPASS ,"renameat2() test passed" ); + } }
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH 1/1 syscalls/renameat202: initialize str with zero
Quoted 14 lines · Cyril Hrubis
Hi!
Quoted 4 lines
In
renameat2_verify(), str is allocated on stack with size 8 (sizeof content) and filled with random data. Then file content (7 bytes) is read into str and the last byte in str is left unchanged with garbage, and test fails if that last byte is not zero.Good catch.
Quoted 2 lines
Fix it by initializing the str with zeros, make sure str is null terminated as long as correct data has been read in.
Looking at the code it may be easier to use
write()to write the string including the null byte in the setup. But either one is fine.
Quoted 8 lines
Quoted 2 lines
Also remove the "str[strlen(content)] == '\0'" check, which is not necessary, because strcmp will catch the failure anyway.
This has been added there on purpose so that we are sure the test wouldn't segfault in case that the str is not null terminated which is unlikely but still possible. And it's still possible even when you zero the str buffer since the read may overwrite whole str and passing non-null terminated buffer to
strcmp()is still undefined operation.
Makes sense, I'll add it back in v2.
Quoted 4 lines
Maybe it would be much better to try to read a buffer larger than the expected content, check that the value from read was size of the data and then do
memcmp()with explicit size.
Agreed, do memcmp only if read(2) returned the expected size.
Quoted 5 lines
BTW: The size of the str should be increased anyway since the read gets
strlen(content) + 10 as a size parameter which is larger than sizeof(str). We should just change it to some arbitrary large value like 128.
Agreed.
Quoted 25 lines
Quoted 25 lines
Also improve the failure message to print more useful debug message.
Signed-off-by Eryu Guan <[email protected]>--- testcases/kernel/syscalls/renameat2/renameat202.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)testcases/kernel/syscalls/renameat2/renameat202.c +1
-1@@ -118,7 +118,7 @@ static void cleanup(void)static void renameat2_verify (void ) { -char str[sizeof (content)]; +char str[sizeof (content)] = {0 };struct stat st;char *emptyfile;char *contentfile;@@ -152,11 +152,12 @@ static void renameat2_verify(void)tst_brkm (TERRNO |TFAIL , cleanup,"close fd failed" ); fd =0 ;
Quoted 14 lines
testcases/kernel/syscalls/renameat2/renameat202.c static void renameat2_verify(void) · line 157
Quoted 10 lines
testcases/kernel/syscalls/renameat2/renameat202.c +3
-7-if (str[strlen (content)] =='\0' && !strcmp (content, str) - && !st.st_size) -tst_resm (TPASS , -"renameat2() swapped the content of the two files" ); -else -tst_resm (TFAIL , -"renameat2() didn't swap the content of the two files" ); +if (strcmp (content, str)) { +tst_resm (TFAIL ,"file content changed after renameat2()" ); +tst_resm (TFAIL ,"expect \"%s\", got \"%s\"" , content, str);This adds two FAIL statements in the output for one failure. Either print the message in one
tst_resm()statement or change the second one to TINFO.
I used TINFO at first, but thought TFAIL might be more clearer. I'll change it back.
Quoted 9 lines
testcases/kernel/syscalls/renameat2/renameat202.c static void renameat2_verify(void) · line 159
Quoted 3 lines
testcases/kernel/syscalls/renameat2/renameat202.c +2
-0+ }else if (st.st_size) { +tst_resm (TFAIL ,"emptyfile has non-zero file size" );You should do return in the previous
if()so that you don't have to do the else if () spagetthi code here.And if you do return here the TPASS message does not have to be in the else block as well.
Fair enough.
Thanks for the review!
Eryu
Quoted 9 lines
Quoted 5 lines
testcases/kernel/syscalls/renameat2/renameat202.c +3
-0+ }else { +tst_resm (TPASS ,"renameat2() test passed" ); + } }Signature · 2 lines
Cyril Hrubis [email protected]
PATCH v2 1/1 syscalls/renameat202: improve renameat2_verify()
In reply to Eryu GuanIn renameat2_verify(), str is allocated on stack with size 8 (sizeof content) and filled with random data. Then file content (7 bytes) is read into str and the last byte in str is left unchanged with garbage, and test fails if that last byte is not zero.
Another problem is, as Cyril Hrubis pointed out, the str buffer is too small, so there's chance to overrun the str buffer, as read(2) is reading strlen(content) + 10 bytes.
Fix them by extending str buffer size and initializing str with zeros. Also improved the checking logic, do strncmp only if expected number of bytes is read from file. And improved the failure messages too.
---
v2:
- update commit summary and log
- enlarge str buffer to BUFSIZ
- check return value of read(2) and do other checks only if the value is correct
- return early on failure
- merge two tst_resm into one on failure
testcases/kernel/syscalls/renameat2/renameat202.c | 28 +++++++++++++++-------- 1 file changed, 19 insertions(+), 9 deletions(-)
testcases/kernel/syscalls/renameat2/renameat202.c +19 -9
@@ -118,10 +118,11 @@ static void cleanup(void)static void renameat2_verify (void ) { -char str[sizeof (content)]; +char str[BUFSIZ ] = {0 };struct stat st;char *emptyfile;char *contentfile; +int readn, data_len;if (TEST_ERRNO ==EINVAL &&TST_BTRFS_MAGIC == fs_type) { tst_brkm(TCONF, cleanup,
@@ -146,17 +147,26 @@ static void renameat2_verify(void)SAFE_STAT (cleanup, emptyfile, &st); -SAFE_READ (cleanup,0 , fd, str,strlen (content) +10 ); + readn =SAFE_READ (cleanup,0 , fd, str,BUFSIZ );if (close (fd) <0 )tst_brkm (TERRNO |TFAIL , cleanup,"close fd failed" ); fd =0 ; -if (str[strlen (content)] =='\0' && !strcmp (content, str) - && !st.st_size) -tst_resm (TPASS , -"renameat2() swapped the content of the two files" ); -else -tst_resm (TFAIL , -"renameat2() didn't swap the content of the two files" ); + data_len =strlen (content); +if (readn != data_len) { +tst_resm (TFAIL ,"Wrong number of bytes read after renameat2(). " +"Expect %d, got %d" , data_len, readn); +return ; + } +if (strncmp (content, str, data_len)) { +tst_resm (TFAIL ,"File content changed after renameat2(). " +"Expect \"%s\", got \"%s\"" , content, str); +return ; + } +if (st.st_size) { +tst_resm (TFAIL ,"emptyfile has non-zero file size" ); +return ; + } +tst_resm (TPASS ,"renameat2() test passed" ); }
Signature · 1 line
2.5.0
PATCH v2 1/1 syscalls/renameat202: improve renameat2_verify()
Hi!
Pushed with two minor changes:
* used sizeof(content) - 1 instead of strlen()
* used ' to quote string in restult instead of \"
Thanks.
Signature · 2 lines
Cyril Hrubis [email protected]