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

grom72 · 2025-02-17T07:12:12Z

There are several issues in strncpy usage that may cause memory corruption. Target string may not be ended by \0 character.

Issues detected during sanitizer integration
Ref: #15105

Co-authored-by: Cedric Koch-Hofer [email protected]
Co-autohred-by: Tomasz Gromadzki [email protected]

Signed-off-by: Tomasz Gromadzki [email protected]

Before requesting gatekeeper:

Two review approvals and any prior change requests have been resolved.
Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
Commit messages follows the guidelines outlined here.
Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

github-actions · 2025-02-17T07:12:34Z

Ticket title is 'Possible missing string termination after strncpy function call'
Status is 'Awaiting Verification'
Labels: 'scrubbed_2.8'
https://daosio.atlassian.net/browse/DAOS-17131

There are several issues in strncpy usage that may cause memory corruption. Target string may not be ended by `\0` character. Issues detected during sanitizer integration Ref: #15105 Co-authored-by: Cedric Koch-Hofer <[email protected]> Co-autohred-by: Tomasz Gromadzki <[email protected]> Signed-off-by: Tomasz Gromadzki <[email protected]>

mchaarawi · 2025-02-17T15:24:54Z

src/client/dfs/cont.c

@@ -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 */


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

mchaarawi

patch looks good to me, but request to remove all the comments and the ticket number. there is no information gained from those comments overall in the code base.
ticket number should be used to note a known or TBD issue, but not fixes; otherwise code become unreadable if everyone does that.

mchaarawi · 2025-02-18T00:31:11Z

Also please make sure to run with Features: dfs commit pragma. thanks

Possible missing string termination after strncpy function call. There are several places where using strncpy can cause memory corruption - the target string may not be terminated with `\0` character. Issue has been detected during sanitizer integration: #15105 Features: dfs Signed-off-by: Tomasz Gromadzki <[email protected]>

grom72 · 2025-02-18T07:36:12Z

Also please make sure to run with Features: dfs commit pragma. thanks

Done

grom72 · 2025-02-18T07:42:47Z

patch looks good to me, but request to remove all the comments and the ticket number. there is no information gained from those comments overall in the code base. ticket number should be used to note a known or TBD issue, but not fixes; otherwise code become unreadable if everyone does that.

A new ticket has been created DAOS-17131. It is only related to issues detected by the sanitizer in #15105.

There is another issue DAOS-17042 that addresses the general problem of possible misuse of strncpy().

daosbuild1 · 2025-02-18T08:20:19Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15920/4/testReport/

Parallel-build: true Skip-build-el8-rpm: true Skip-build-el9-rpm: true Skip-build-leap15-rpm: true Skip-build-ubuntu20-rpm: true Skip-build-el8-gcc-dev: true Skip-build-leap15-icc: true Skip-test: true Skip-unit-test: true Skip-unit-test-memcheck: true Skip-bullseye: true Signed-off-by: Tomasz Gromadzki <[email protected]>

grom72 · 2025-02-20T16:52:14Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15920/4/testReport/

Transient issue in CI fixed in the next build without any source code modification.
https://build.hpdd.intel.com/job/daos-stack/job/daos/view/change-requests/job/PR-15920/5/testReport/

mchaarawi · 2025-02-20T17:00:55Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15920/4/testReport/

Transient issue in CI fixed in the next build without any source code modification. https://build.hpdd.intel.com/job/daos-stack/job/daos/view/change-requests/job/PR-15920/5/testReport/

what is the transient error? is it a known issue? more info is needed when saying the error is transient. it should be a known issue with a ticket.

jxiong

There is nothing wrong with the original implementation, IMO. We should address the consistency issue though

jxiong · 2025-02-20T20:01:03Z

src/client/dfs/cont.c

@@ -83,7 +84,7 @@ 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);
+			strncpy(dattr.da_hints, attr->da_hints, DAOS_CONT_HINT_MAX_LEN - 1);
 			dattr.da_hints[DAOS_CONT_HINT_MAX_LEN - 1] = '\0';


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

Yes, please take a look at #15105

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"

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

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

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.

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);

I see. thanks for the explanation

jxiong · 2025-02-20T20:05:22Z

src/client/dfs/obj.c

@@ -510,7 +511,8 @@ 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);


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.

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

grom72 · 2025-02-21T09:00:14Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15920/4/testReport/

Transient issue in CI fixed in the next build without any source code modification. https://build.hpdd.intel.com/job/daos-stack/job/daos/view/change-requests/job/PR-15920/5/testReport/

what is the transient error? is it a known issue? more info is needed when saying the error is transient. it should be a known issue with a ticket.

Issue has been created: https://daosio.atlassian.net/browse/DAOS-17149

Features: dfs Signed-off-by: Tomasz Gromadzki <[email protected]>

daosbuild1 · 2025-02-21T21:38:56Z

Test stage Functional Hardware Large completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15920/6/execution/node/1420/log

grom72 requested review from a team as code owners February 17, 2025 07:12

grom72 marked this pull request as draft February 17, 2025 07:12

grom72 force-pushed the grom72/daos-16501 branch from da1f492 to 6624f78 Compare February 17, 2025 07:16

grom72 force-pushed the grom72/daos-16501 branch from 6624f78 to d7915ac Compare February 17, 2025 07:26

grom72 marked this pull request as ready for review February 17, 2025 07:27

grom72 requested a review from knard38 February 17, 2025 07:27

grom72 mentioned this pull request Feb 17, 2025

DAOS-16501 build: Add libsanitize #15105

Open

18 tasks

grom72 requested a review from liw February 17, 2025 07:39

knard38 previously approved these changes Feb 17, 2025

View reviewed changes

liw previously approved these changes Feb 17, 2025

View reviewed changes

mchaarawi reviewed Feb 17, 2025

View reviewed changes

mchaarawi requested changes Feb 17, 2025

View reviewed changes

grom72 changed the title ~~DAOS-16501 dfs: Possible missing string termination~~ DAOS-17131 dfs: Possible missing string termination after strncpy function call Feb 18, 2025

grom72 dismissed stale reviews from liw and knard38 via ee61c0d February 18, 2025 07:31

knard38 approved these changes Feb 18, 2025

View reviewed changes

grom72 requested a review from mchaarawi February 18, 2025 07:42

mchaarawi approved these changes Feb 18, 2025

View reviewed changes

grom72 requested a review from a team February 20, 2025 16:54

jxiong requested changes Feb 20, 2025

View reviewed changes

Test: full test with proper pragmas

7b03c4d

Features: dfs Signed-off-by: Tomasz Gromadzki <[email protected]>

jxiong self-requested a review February 21, 2025 17:49

jxiong approved these changes Feb 21, 2025

View reviewed changes

daltonbohning removed the request for review from a team February 21, 2025 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

grom72 commented Feb 17, 2025 •

edited

Loading

github-actions bot commented Feb 17, 2025 •

edited

Loading

mchaarawi Feb 17, 2025

grom72 Feb 21, 2025

mchaarawi left a comment •

edited

Loading

mchaarawi commented Feb 18, 2025

grom72 commented Feb 18, 2025

grom72 commented Feb 18, 2025

daosbuild1 commented Feb 18, 2025

grom72 commented Feb 20, 2025 •

edited

Loading

mchaarawi commented Feb 20, 2025

jxiong left a comment

jxiong Feb 20, 2025

grom72 Feb 21, 2025

daltonbohning Feb 21, 2025

grom72 Feb 21, 2025

daltonbohning Feb 21, 2025

daltonbohning Feb 21, 2025

daltonbohning Feb 21, 2025

jxiong Feb 21, 2025

jxiong Feb 20, 2025

grom72 Feb 21, 2025

grom72 commented Feb 21, 2025

daosbuild1 commented Feb 21, 2025

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

Are you sure you want to change the base?

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

Conversation

grom72 commented Feb 17, 2025 • edited Loading

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchaarawi left a comment • edited Loading

Choose a reason for hiding this comment

mchaarawi commented Feb 18, 2025

grom72 commented Feb 18, 2025

grom72 commented Feb 18, 2025

daosbuild1 commented Feb 18, 2025

grom72 commented Feb 20, 2025 • edited Loading

mchaarawi commented Feb 20, 2025

jxiong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grom72 commented Feb 21, 2025

daosbuild1 commented Feb 21, 2025

grom72 commented Feb 17, 2025 •

edited

Loading

github-actions bot commented Feb 17, 2025 •

edited

Loading

mchaarawi left a comment •

edited

Loading

grom72 commented Feb 20, 2025 •

edited

Loading