DAOS-6685 Test: Fixed coverity issue in test code. #4684
Conversation
Fixed Coverity issue in test code. 313028(#2 of 2) and 313033(#1 of 1). Signed-off-by: Samir Raval <[email protected]>
src/common/tests_dmg_helpers.c
Outdated
| return -DER_INVAL; | ||
| } | ||
| strncpy(devices[*disks].host, strtok(host, ":") + 1, | ||
| sizeof(devices[*disks].host) - 1); |
There was a problem hiding this comment.
sizeof(devices[*disks].host) - 1) : Not clear why you are subtracting 1
strtok(host, ":") + 1, : Are you null terminating here...
Not clear why 1 byte is added in strtok and one byte removed from sizeof... Maybe a comment about what is going on here could be helpful.
There was a problem hiding this comment.
+1 from strtok is needed to eliminate the double quotes from hostname, for example, JSON output is
"wolf-154:10001" so strtok will return "wolf-154 and +1 will start from second-string.
I used -1 to avoid the buffer overwrite.
There was a problem hiding this comment.
Won't Coverity still complain about this?
jvolivie-desk1:[~/daos]$ cat foo.c
#include <stdio.h>
#include <string.h>
int main()
{
char x[] = "";
char y[] = ":";
char z[] = ":abc";
char m[] = "abc:";
char n[] = "abc:cba";
printf("%s\n", strtok(n, ":") + 1);
fflush(stdout);
printf("%s\n", strtok(m, ":") + 1);
fflush(stdout);
printf("%s\n", strtok(z, ":") + 1);
fflush(stdout);
printf("%s\n", strtok(y, ":") + 1);
fflush(stdout);
printf("%s\n", strtok(x, ":") + 1);
fflush(stdout);
return 0;
}
I get
bc
bc
bc
Segmentation fault (core dumped)
Additionally, with strncpy, the string will not be null terminated if the length given would result in truncation. Typically, it's good practice to explicitly make sure there is a 0 in the last character.
There was a problem hiding this comment.
I am not if that will complain about strtok or not. Let me find another way of copying it with null termination.
src/common/tests_dmg_helpers.c
Outdated
| strcpy(devices[*disks].state, json_object_to_json_string(tmp)); | ||
|
|
||
| strncpy(devices[*disks].state, json_object_to_json_string(tmp), | ||
| sizeof(devices[*disks].state) - 1); |
There was a problem hiding this comment.
Is json_object_to_json_string null terminated? Same question as above why subtracting 1 from sizeof?
There was a problem hiding this comment.
I think so. I tested it locally and it worked.
src/common/tests_dmg_helpers.c
Outdated
| return -DER_INVAL; | ||
| } | ||
| strncpy(devices[*disks].host, strtok(host, ":") + 1, | ||
| sizeof(devices[*disks].host) - 1); |
There was a problem hiding this comment.
Won't Coverity still complain about this?
jvolivie-desk1:[~/daos]$ cat foo.c
#include <stdio.h>
#include <string.h>
int main()
{
char x[] = "";
char y[] = ":";
char z[] = ":abc";
char m[] = "abc:";
char n[] = "abc:cba";
printf("%s\n", strtok(n, ":") + 1);
fflush(stdout);
printf("%s\n", strtok(m, ":") + 1);
fflush(stdout);
printf("%s\n", strtok(z, ":") + 1);
fflush(stdout);
printf("%s\n", strtok(y, ":") + 1);
fflush(stdout);
printf("%s\n", strtok(x, ":") + 1);
fflush(stdout);
return 0;
}
I get
bc
bc
bc
Segmentation fault (core dumped)
Additionally, with strncpy, the string will not be null terminated if the length given would result in truncation. Typically, it's good practice to explicitly make sure there is a 0 in the last character.
Signed-off-by: Samir Raval <[email protected]>
|
Test stage checkpatch completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4684/4/execution/node/72/log |
Co-authored-by: daosbuild1 <[email protected]>
src/common/tests_dmg_helpers.c
Outdated
| strncpy(devices[*disks].host, strtok(host, ":") + 1, | ||
| sizeof(devices[*disks].host) - 1); | ||
| tmp_var = strtok(host, ":") + 1; | ||
| if (!tmp_var) { |
There was a problem hiding this comment.
if strtok returns NULL, this will be 1
There was a problem hiding this comment.
fixed the code to check NULL
|
In general strtok_r() is strongly preferred over strtok(), however there may be an exception for test code. |
Fixed Coverity issue in the test code.
313028(#2 of 2) and 313033(#1 of 1).
Signed-off-by: Samir Raval [email protected]