Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DAOS-6685 Test: Fixed coverity issue in test code. #4684

Merged
merged 5 commits into from
Feb 26, 2021
Merged

DAOS-6685 Test: Fixed coverity issue in test code. #4684

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2299998
DAOS-6685 Test: Fixed coverity issue in test code.
ravalsam Feb 18, 2021
8dc4ab8
Merge branch 'master' into samirrav/DAOS-6685_1
ravalsam Feb 18, 2021
691d481
Updated code based on review comments.
ravalsam Feb 19, 2021
35458c6
Apply suggestions from code review
ravalsam Feb 19, 2021
0f17012
Update tests_dmg_helpers.c
ravalsam Feb 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 9 additions & 13 deletions src/common/tests_dmg_helpers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,13 +583,8 @@ parse_device_info(struct json_object *smd_dev, device_list *devices,
for (i = 0; i < dev_length; i++) {
dev = json_object_array_get_idx(smd_dev, i);

if (strlen(host + 2) <= DSS_HOSTNAME_MAX_LEN)
strcpy(devices[*disks].host, strtok(host, ":") + 1);
else {
D_ERROR("Hostname is larger than %d\n",
DSS_HOSTNAME_MAX_LEN);
return -DER_INVAL;
}
strncpy(devices[*disks].host, strtok(host, ":") + 1,
sizeof(devices[*disks].host) - 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ravalsam ravalsam Feb 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not if that will complain about strtok or not. Let me find another way of copying it with null termination.


if (!json_object_object_get_ex(dev, "uuid", &tmp)) {
D_ERROR("unable to extract uuid from JSON\n");
Expand All @@ -615,7 +610,9 @@ parse_device_info(struct json_object *smd_dev, device_list *devices,
D_ERROR("unable to extract state from JSON\n");
return -DER_INVAL;
}
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is json_object_to_json_string null terminated? Same question as above why subtracting 1 from sizeof?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. I tested it locally and it worked.


if (!json_object_object_get_ex(dev, "rank", &tmp)) {
D_ERROR("unable to extract rank from JSON\n");
Expand Down Expand Up @@ -656,19 +653,17 @@ dmg_storage_device_list(const char *dmg_config_file, int *ndisks,

if (!json_object_object_get_ex(dmg_out, "host_storage_map",
&storage_map)) {
D_FREE(disk);
D_ERROR("unable to extract host_storage_map from JSON\n");
return -DER_INVAL;
D_GOTO(out, rc = -DER_INVAL);
}

json_object_object_foreach(storage_map, key, val) {
D_DEBUG(DB_TEST, "key:\"%s\",val=%s\n", key,
json_object_to_json_string(val));

if (!json_object_object_get_ex(val, "hosts", &hosts)) {
D_FREE(disk);
D_ERROR("unable to extract hosts from JSON\n");
return -DER_INVAL;
D_GOTO(out, rc = -DER_INVAL);
}

D_ALLOC(host, strlen(json_object_to_json_string(hosts)) + 1);
Expand All @@ -684,7 +679,7 @@ dmg_storage_device_list(const char *dmg_config_file, int *ndisks,
smd_info, "devices", &smd_dev)) {
D_ERROR("unable to extract devices\n");
D_FREE(host);
return -DER_INVAL;
D_GOTO(out, rc = -DER_INVAL);
}

if (smd_dev != NULL)
Expand Down Expand Up @@ -712,6 +707,7 @@ dmg_storage_device_list(const char *dmg_config_file, int *ndisks,
if (dmg_out != NULL)
json_object_put(dmg_out);

out:
D_FREE(disk);
return rc;
}
Expand Down