[OrHayat]

daos_is_valid_uuid_string() checked strnlen(uuid, 36) == 36, which is true for any string of at least 36 characters, so a label such as -clone was taken for a UUID and rejected by daos_label_is_valid(). Read one character past 36 so only an exact 36-character string passes.

Steps for the author:

• Commit message follows the guidelines.
• Appropriate Features or Test-tag pragmas were used.
• Appropriate Functional Test Stages were run.
• At least two positive code reviews including at least one code owner from each category referenced in the PR.
• Testing is complete. If necessary, forced-landing label added and a reason added in a comment.

After all prior steps are complete:

• Gatekeeper requested (daos-gatekeeper added as a reviewer).

[github-actions]

Ticket title is 'daos_label_is_valid() rejects labels that start with a UUID'
Status is 'Open'
https://daosio.atlassian.net/browse/DAOS-19697

[OrHayat]

hi i fixed the clang format issue and the compilation issue that it caused.

[mchaarawi]

The fix is correct for labels, but it changes the shared helper requirement: it now reads byte 36, so it assumes the input is NULL terminated.
The checker callers (chk_pools_pause_cb, chk_pools_cleanup_cb, chk_pools_load_from_db) pass raw VOS akey buffers that were stored with strlen(uuid_str). That's exactly 36 bytes with no terminator, so byte 36 is outside the key. @Nasf-Fan any thoughts?

This isn't a real regression: chk_bk_fetch_pool() already calls strlen() on the same buffer, and uuid_parse() also reads it. But the change does add another reader that relies on the byte after the key.

Could we keep the helper as it is on master and fix only the label path? daos_label_is_valid() has already computed a bounded length:

--- a/src/include/daos_prop.h
+++ b/src/include/daos_prop.h
-	if (maybe_uuid && daos_is_valid_uuid_string(label, UUID_SST_NONE))
+	if (maybe_uuid && len == DAOS_UUID_STR_SIZE - 1 &&
+	    daos_is_valid_uuid_string(label, UUID_SST_NONE))

This fixes -clone, leaves the checker untouched, and the new test case still covers it. I'll file a separate ticket for the checker to use key->iov_len and a local NUL-terminated copy instead of strlen() on the raw key.

[OrHayat]

Hi @mchaarawi @Nasf-Fan

I don't think fixing only the label path is the right approach: the helper would stay broken for every other existing and future caller that passes a NUL-terminated string
(<uuid>-clone still counts as a UUID).

What I propose instead is to keep the string helper correct and add a variant for raw bytes + length:

• daos_is_valid_uuid_string(const char *uuid, int sensitive): same name and signature; bounded by strnlen(uuid, DAOS_UUID_STR_SIZE) and calls the _iov variant.
• daos_is_valid_uuid_iov(const d_iov_t *iov, int sensitive): new, reads at most iov_len bytes. The checker callbacks pass their key directly, so nothing reads past the
  akey.

For the checker ticket, I don't think a NUL-terminated copy is needed. All three sys_db callbacks receive the akey as a d_iov_t *key, so the length is always available, but
each one keeps only key->iov_buf and drops iov_len:

Callback (has key)	Passes only key->iov_buf to
chk_pools_pause_cb	chk_bk_fetch_pool(), chk_bk_update_pool()
chk_pools_cleanup_cb	chk_bk_fetch_pool(), chk_bk_delete_pool()
chk_pools_load_from_db	chk_bk_fetch_pool(), uuid_parse()

The callees then recover the length the length-less way:

1. chk_bk_fetch_pool(), chk_bk_update_pool(), chk_bk_delete_pool() call strlen() on the key and log it with %s. They could take the d_iov_t instead. Their other
   callers (chk_pool_start_one, chk_pools_load_list, chk_pools_update_bk) already have a real string from chk_uuid_unparse() and would just wrap it.
2. uuid_parse() needs a NUL-terminated string, and its return value is ignored. If the byte after the key isn't \0, libuuid sees a length other than 36, fails, and uuid is
   used uninitialized. Parsing is no harder than the validation DAOS already does, so a matching daos_uuid_parse_iov() would replace it and its result could be checked.

What do you think?

[mchaarawi]

i still think there is an issue with the checker overreading after the check (this is an existing behavior that requires a different PR).

[kjacque]

@mchaarawi Is there already a PR to run this in our CI before landing?

[mchaarawi]

@mchaarawi Is there already a PR to run this in our CI before landing?

i just pushed #19326 to get it through our CI

[Nasf-Fan]

How do we guarantee uuid is not shorter than DAOS_UUID_STR_SIZE? If shorter, then the buffer is out of boundary? It is not fatal if we do not access the space after the uuid tail, but it is potential risk.

[OrHayat]

Fixed: iov_buf_len is now set to the strnlen() result. It was a true bug but wasn't a real risk before either, since nothing reads that field, but now it's correct.
@mchaarawi this needs to be moved to mschaara/19697 (#19326) if you want to merge that one instead.

[mchaarawi]

Fixed: iov_buf_len is now set to the strnlen() result. It was a true bug but wasn't a real risk before either, since nothing reads that field, but now it's correct. @mchaarawi this needs to be moved to mschaara/19697 (#19326) if you want to merge that one instead.

we will merge this one. the other one is just for testing CI since our CI does not work with external branches yet. i updated it

[mchaarawi]

#19326 passed all tests