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-15069 dfs: use memcpy instead of snprintf for readdir #13654

Merged
merged 2 commits into from
Jan 30, 2024
Merged

DAOS-15069 dfs: use memcpy instead of snprintf for readdir #13654

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
404118a
DAOS-15069 dfs: use strncpy instead of snprintf for readdir
mchaarawi Jan 23, 2024
f4c85e4
use memcpy instead of strncpy
mchaarawi Jan 25, 2024
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
17 changes: 5 additions & 12 deletions src/client/dfs/dfs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,7 +568,7 @@ fetch_entry(dfs_layout_ver_t ver, daos_handle_t oh, daos_handle_t th, const char

/** TODO - not supported yet */
if (strcmp(name, ".") == 0)
D_ASSERT(0);
return ENOTSUP;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is progress but wasn't there a different ticket with a reprodyucer for this? Without seeing that ticket could dfs_lookup_rel() skip over entiries named "." when doing the path walk, then the rerroducer should pass rather than exit gracefully as presumably it does with this fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can leave that case for the ticket. this is fine for now


if (xnr) {
D_ALLOC_ARRAY(pxnames, xnr);
Expand Down Expand Up @@ -3790,11 +3790,8 @@ readdir_int(dfs_t *dfs, dfs_obj_t *obj, daos_anchor_t *anchor, uint32_t *nr,
D_GOTO(out, rc = daos_der2errno(rc));

for (ptr = enum_buf, i = 0; i < number; i++) {
int len;

len = snprintf(dirs[key_nr].d_name,
kds[i].kd_key_len + 1, "%s", ptr);
D_ASSERT(len >= kds[i].kd_key_len);
memcpy(dirs[key_nr].d_name, ptr, kds[i].kd_key_len);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe check that kds[i].kd_key_len < sizeof(dirs[key_nr].d_name)? (I realize that this wasn't checked before, maybe it's guaranteed elsewhere)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that condition would be false, then we have landed some data corruption in the container and this would be the least of the problems :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

But if we don't detect it here, we'll have two data corruptions. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my point was we probably won't even get here if there is data corruption in the container.
but sure, i can add that check if needed to repush. otherwise can add it in another PR im working on.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding in another PR sounds good to me.

Copy link
Collaborator

@yuvale2 yuvale2 Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Alternatively, if the check is 100% redundant, a comment that explains this would help (it's not obvious unless you know the server side, I guess).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is an issue here but adding in another PR seems the way to go.

dirs[key_nr].d_name[kds[i].kd_key_len] = '\0';
ptr += kds[i].kd_key_len;

/** stat the entry if requested */
Expand Down Expand Up @@ -6542,8 +6539,6 @@ dfs_listxattr(dfs_t *dfs, dfs_obj_t *obj, char *list, daos_size_t *size)
continue;

for (ptr = enum_buf, i = 0; i < number; i++) {
int len;

if (strncmp("x:", ptr, 2) != 0) {
ptr += kds[i].kd_key_len;
continue;
Expand All @@ -6556,10 +6551,8 @@ dfs_listxattr(dfs_t *dfs, dfs_obj_t *obj, char *list, daos_size_t *size)
if (list_size < kds[i].kd_key_len - 2)
continue;

len = snprintf(ptr_list, kds[i].kd_key_len - 1, "%s",
ptr + 2);
D_ASSERT(len >= kds[i].kd_key_len - 2);

memcpy(ptr_list, ptr + 2, kds[i].kd_key_len - 2);
ptr_list[kds[i].kd_key_len - 2] = '\0';
list_size -= kds[i].kd_key_len - 1;
ptr_list += kds[i].kd_key_len - 1;
ptr += kds[i].kd_key_len;
Expand Down
Loading