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-17131 dfs: Possible missing string termination after strncpy function call #15920

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

DAOS-17131 dfs: Possible missing string termination after strncpy function call #15920

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d7915ac
DAOS-16501 dfs: Possible missing string termination
kanard38 Dec 10, 2024
ee61c0d
DAOS-17131 dfs: Possible missing string termination
grom72 Feb 18, 2025
9b7cd41
Tests: NLT tests only
grom72 Feb 20, 2025
7b03c4d
Test: full test with proper pragmas
grom72 Feb 21, 2025
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
4 changes: 3 additions & 1 deletion src/client/dfs/cont.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/**
* (C) Copyright 2018-2024 Intel Corporation.
* (C) Copyright 2025 Hewlett Packard Enterprise Development LP
*
* SPDX-License-Identifier: BSD-2-Clause-Patent
*/
Expand Down Expand Up @@ -83,7 +84,8 @@ dfs_cont_create(daos_handle_t poh, uuid_t *cuuid, dfs_attr_t *attr, daos_handle_
dattr.da_chunk_size = DFS_DEFAULT_CHUNK_SIZE;

if (attr->da_hints[0] != 0) {
strncpy(dattr.da_hints, attr->da_hints, DAOS_CONT_HINT_MAX_LEN);
/* DAOS-17042 Replace strncpy with strncat or strlcpy */
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove all these comments from the code. they are not helpful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

strncpy(dattr.da_hints, attr->da_hints, DAOS_CONT_HINT_MAX_LEN - 1);
dattr.da_hints[DAOS_CONT_HINT_MAX_LEN - 1] = '\0';
Copy link
Contributor

Choose a reason for hiding this comment

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

The implementation is actually perfectly fine. Are you see any issues with the original code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, please take a look at #15105

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think the existing code has a bug. If attr->da_hints is exactly DAOS_CONT_HINT_MAX_LEN long, then strncpy copies the entire buffer as expected. But then dattr.da_hints[DAOS_CONT_HINT_MAX_LEN - 1] = '\0'; overwrites the last character.
E.g.

strncpy(dattr.da_hints, "1234", 4);
dattr.da_hints[4 - 1] = '\0';

Yields dattr.da_hints = "123\0"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, finally the string will be somehow OK, but it will be not the same string. The problem of strncpy misusage has been discussed in #15105

Copy link
Contributor

Choose a reason for hiding this comment

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

But then again I guess it doesn't matter because attr->da_hints could be longer than DAOS_CONT_HINT_MAX_LEN and not copied entirely anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

Reading up on this more, it's not actually a misuse of strncpy, but just simply that -Wstringop-truncation warns that you might lose data from the source string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, doing some local testing, the compiler complains either way. So this change doesn't solve anything

With

char my_string[4];
strncpy(my_string, "1234", 4);
my_string[4-1] = '\0';

It complains with

warning: ‘strncpy’ output truncated before terminating nul copying 4 bytes from a string of the same length [-Wstringop-truncation]
     strncpy(my_string, "1234", 4);

And with

char my_string[4];
strncpy(my_string, "1234", 4-1);
my_string[4-1] = '\0';

It complains with

warning: ‘strncpy’ output truncated copying 3 bytes from a string of length 4 [-Wstringop-truncation]
     strncpy(my_string, "1234", 4-1);

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. thanks for the explanation

}
} else {
Expand Down
13 changes: 9 additions & 4 deletions src/client/dfs/obj.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/**
* (C) Copyright 2018-2024 Intel Corporation.
* (C) Copyright 2025 Hewlett Packard Enterprise Development LP
*
* SPDX-License-Identifier: BSD-2-Clause-Patent
*/
Expand Down Expand Up @@ -510,7 +511,9 @@ dfs_dup(dfs_t *dfs, dfs_obj_t *obj, int flags, dfs_obj_t **_new_obj)
D_GOTO(err, rc = EINVAL);
}

strncpy(new_obj->name, obj->name, DFS_MAX_NAME + 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Again this is fine too if the obj->name is already valid.

I think a serious issue is about the consistency of using DFS_MAX_NAME. Sometimes the name is defined as name[DFS_MAX_NAME + 1] but sometimes it's name[DFS_MAX_NAME]. I would prefer to use the latter and then redefining maximum object name as DFS_MAX_NAME including the ending zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, this is the result of #15105 + see my comment above: #15920 (comment)

/* DAOS-17042 Replace strncpy with strncat or strlcpy */
strncpy(new_obj->name, obj->name, DFS_MAX_NAME);
new_obj->name[DFS_MAX_NAME] = '\0';
new_obj->dfs = dfs;
new_obj->mode = obj->mode;
new_obj->flags = flags;
Expand Down Expand Up @@ -616,8 +619,9 @@ dfs_obj_local2global(dfs_t *dfs, dfs_obj_t *obj, d_iov_t *glob)
oid_cp(&obj_glob->parent_oid, obj->parent_oid);
uuid_copy(obj_glob->coh_uuid, coh_uuid);
uuid_copy(obj_glob->cont_uuid, cont_uuid);
strncpy(obj_glob->name, obj->name, DFS_MAX_NAME + 1);
obj_glob->name[DFS_MAX_NAME] = 0;
/* DAOS-17042 Replace strncpy with strncat or strlcpy */
strncpy(obj_glob->name, obj->name, DFS_MAX_NAME);
obj_glob->name[DFS_MAX_NAME] = '\0';
if (S_ISDIR(obj_glob->mode))
return 0;
rc = dfs_get_chunk_size(obj, &obj_glob->chunk_size);
Expand Down Expand Up @@ -674,7 +678,8 @@ dfs_obj_global2local(dfs_t *dfs, int flags, d_iov_t glob, dfs_obj_t **_obj)

oid_cp(&obj->oid, obj_glob->oid);
oid_cp(&obj->parent_oid, obj_glob->parent_oid);
strncpy(obj->name, obj_glob->name, DFS_MAX_NAME + 1);
/* DAOS-17042 Replace strncpy with strncat or strlcpy */
strncpy(obj->name, obj_glob->name, DFS_MAX_NAME);
obj->name[DFS_MAX_NAME] = '\0';
obj->mode = obj_glob->mode;
obj->dfs = dfs;
Expand Down
8 changes: 7 additions & 1 deletion src/client/dfuse/ops/lookup.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/**
* (C) Copyright 2016-2024 Intel Corporation.
* (C) Copyright 2025 Hewlett Packard Enterprise Development LP
*
* SPDX-License-Identifier: BSD-2-Clause-Patent
*/
Expand Down Expand Up @@ -118,11 +119,14 @@ dfuse_reply_entry(struct dfuse_info *dfuse_info, struct dfuse_inode_entry *ie,

/* Save the old name so that we can invalidate it in later */
wipe_parent = inode->ie_parent;
/* DAOS-17042 Replace strncpy with strncat or strlcpy */
strncpy(wipe_name, inode->ie_name, NAME_MAX);
wipe_name[NAME_MAX] = '\0';

inode->ie_parent = ie->ie_parent;
strncpy(inode->ie_name, ie->ie_name, NAME_MAX + 1);
/* DAOS-17042 Replace strncpy with strncat or strlcpy */
strncpy(inode->ie_name, ie->ie_name, NAME_MAX);
inode->ie_name[NAME_MAX] = '\0';
}
atomic_fetch_sub_relaxed(&ie->ie_ref, 1);
dfuse_ie_close(dfuse_info, ie);
Expand Down Expand Up @@ -294,7 +298,9 @@ dfuse_cb_lookup(fuse_req_t req, struct dfuse_inode_entry *parent,
if (attr_len)
DFUSE_TRA_DEBUG(ie, "Attr len is %zi", attr_len);

/* DAOS-17042 Replace strncpy with strncat or strlcpy */
strncpy(ie->ie_name, name, NAME_MAX);
ie->ie_name[NAME_MAX] = '\0';

dfs_obj2id(ie->ie_obj, &ie->ie_oid);

Expand Down
4 changes: 3 additions & 1 deletion src/tests/obj_ctl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/**
* (C) Copyright 2017-2022 Intel Corporation.
* (C) Copyright 2025 Hewlett Packard Enterprise Development LP
*
* SPDX-License-Identifier: BSD-2-Clause-Patent
*/
Expand Down Expand Up @@ -198,7 +199,8 @@ ctl_cmd_run(char opc, char *args)
int rc;

if (args) {
strncpy(buf, args, CTL_BUF_LEN);
/* DAOS-17042 Replace strncpy with strncat or strlcpy */
strncpy(buf, args, CTL_BUF_LEN - 1);
buf[CTL_BUF_LEN - 1] = '\0';
str = daos_str_trimwhite(buf);
} else {
Expand Down