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

Add check for nested optional members in key #2154

Merged
merged 3 commits into from
Jan 16, 2025
Merged

Add check for nested optional members in key #2154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bf4214f
Add check for nested optional members in key
dpotman Jan 9, 2025
88cfe84
Allow optional flag on a non-key member in case any other member in t…
dpotman Jan 15, 2025
19a9679
Add comment explaining the behaviour of the validation with respect t…
dpotman Jan 16, 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
17 changes: 13 additions & 4 deletions src/core/ddsc/tests/dynamic_type.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,11 +416,10 @@ CU_Test (ddsc_dynamic_type, struct_member_prop, .init = dynamic_type_init, .fini
dds_dynamic_type_set_autoid (&dstruct, DDS_DYNAMIC_TYPE_AUTOID_HASH);
dds_dynamic_type_add_member (&dstruct, DDS_DYNAMIC_MEMBER_PRIM(DDS_DYNAMIC_UINT16, "m1"));
dds_dynamic_type_add_member (&dstruct, DDS_DYNAMIC_MEMBER_PRIM(DDS_DYNAMIC_UINT16, "m2"));
dds_dynamic_type_add_member (&dstruct, DDS_DYNAMIC_MEMBER_PRIM(DDS_DYNAMIC_UINT16, "m3"));

dds_return_t ret = dds_dynamic_member_set_key (&dstruct, ddsi_dynamic_type_member_hashid ("m2"), true);
CU_ASSERT_EQUAL_FATAL (ret, DDS_RETCODE_OK);
ret = dds_dynamic_member_set_optional (&dstruct, ddsi_dynamic_type_member_hashid ("m2"), true);
CU_ASSERT_EQUAL_FATAL (ret, DDS_RETCODE_OK);
ret = dds_dynamic_member_set_external (&dstruct, ddsi_dynamic_type_member_hashid ("m2"), true);
CU_ASSERT_EQUAL_FATAL (ret, DDS_RETCODE_OK);
ret = dds_dynamic_member_set_hashid (&dstruct, ddsi_dynamic_type_member_hashid ("m2"), "m2_name");
Expand All @@ -429,8 +428,12 @@ CU_Test (ddsc_dynamic_type, struct_member_prop, .init = dynamic_type_init, .fini
ret = dds_dynamic_member_set_must_understand (&dstruct, ddsi_dynamic_type_member_hashid ("m2_name"), true);
CU_ASSERT_EQUAL_FATAL (ret, DDS_RETCODE_OK);

// Optional and key can't be set to the same member
ret = dds_dynamic_member_set_optional (&dstruct, ddsi_dynamic_type_member_hashid ("m3"), true);
CU_ASSERT_EQUAL_FATAL (ret, DDS_RETCODE_OK);

struct ddsi_type *type = get_ddsi_type (&dstruct);
CU_ASSERT_EQUAL_FATAL (type->xt._u.structure.members.length, 2);
CU_ASSERT_EQUAL_FATAL (type->xt._u.structure.members.length, 3);

CU_ASSERT_EQUAL_FATAL (type->xt._u.structure.members.seq[0].id, ddsi_dynamic_type_member_hashid ("m1"));
CU_ASSERT_FATAL (!(type->xt._u.structure.members.seq[0].flags & DDS_XTypes_IS_KEY));
Expand All @@ -440,10 +443,16 @@ CU_Test (ddsc_dynamic_type, struct_member_prop, .init = dynamic_type_init, .fini

CU_ASSERT_EQUAL_FATAL (type->xt._u.structure.members.seq[1].id, ddsi_dynamic_type_member_hashid ("m2_name"));
CU_ASSERT_FATAL (type->xt._u.structure.members.seq[1].flags & DDS_XTypes_IS_KEY);
CU_ASSERT_FATAL (type->xt._u.structure.members.seq[1].flags & DDS_XTypes_IS_OPTIONAL);
CU_ASSERT_FATAL (!(type->xt._u.structure.members.seq[1].flags & DDS_XTypes_IS_OPTIONAL));
CU_ASSERT_FATAL (type->xt._u.structure.members.seq[1].flags & DDS_XTypes_IS_EXTERNAL);
CU_ASSERT_FATAL (type->xt._u.structure.members.seq[1].flags & DDS_XTypes_IS_MUST_UNDERSTAND);

CU_ASSERT_EQUAL_FATAL (type->xt._u.structure.members.seq[2].id, ddsi_dynamic_type_member_hashid ("m3"));
CU_ASSERT_FATAL (!(type->xt._u.structure.members.seq[2].flags & DDS_XTypes_IS_KEY));
CU_ASSERT_FATAL (type->xt._u.structure.members.seq[2].flags & DDS_XTypes_IS_OPTIONAL);
CU_ASSERT_FATAL (!(type->xt._u.structure.members.seq[2].flags & DDS_XTypes_IS_EXTERNAL));
CU_ASSERT_FATAL (!(type->xt._u.structure.members.seq[2].flags & DDS_XTypes_IS_MUST_UNDERSTAND));

dds_dynamic_type_unref (&dstruct);
}

Expand Down
67 changes: 47 additions & 20 deletions src/core/ddsi/src/ddsi_typewrap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -853,7 +853,7 @@ static dds_return_t xt_valid_type_flags (struct ddsi_domaingv *gv, uint16_t flag
#define M DDS_XTypes_IS_MUST_UNDERSTAND
#define K DDS_XTypes_IS_KEY
#define D DDS_XTypes_IS_DEFAULT
static dds_return_t xt_valid_member_flags (struct ddsi_domaingv *gv, uint16_t flags, uint8_t member_flag_kind)
static dds_return_t xt_valid_member_flags (struct ddsi_domaingv *gv, uint16_t flags, uint8_t member_flag_kind, bool in_key)
{
dds_return_t ret = DDS_RETCODE_OK;

Expand All @@ -870,6 +870,10 @@ static dds_return_t xt_valid_member_flags (struct ddsi_domaingv *gv, uint16_t fl
case MEMBER_FLAG_STRUCT_MEMBER:
if (flags & ~(T1|T2|O|M|K|X))
ret = DDS_RETCODE_BAD_PARAMETER;
if (in_key && (flags & O))
ret = DDS_RETCODE_BAD_PARAMETER;
if ((flags & O) && (flags & K))
ret = DDS_RETCODE_BAD_PARAMETER;
break;
case MEMBER_FLAG_UNION_MEMBER:
if (flags & ~(T1|T2|D|X))
Expand All @@ -879,6 +883,8 @@ static dds_return_t xt_valid_member_flags (struct ddsi_domaingv *gv, uint16_t fl
// must-understand not in spec
if (flags & ~(T1|T2|M|K))
ret = DDS_RETCODE_BAD_PARAMETER;
if (in_key && (flags & O))
ret = DDS_RETCODE_BAD_PARAMETER;
break;
case MEMBER_FLAG_ENUM_LITERAL:
if (flags & ~(D))
Expand Down Expand Up @@ -932,11 +938,11 @@ static dds_return_t xt_valid_array_bounds (struct ddsi_domaingv *gv, const struc
return DDS_RETCODE_OK;
}

static dds_return_t xt_validate_impl (struct ddsi_domaingv *gv, const struct xt_type *t, bool validate_hash_type)
static dds_return_t xt_validate_impl (struct ddsi_domaingv *gv, const struct xt_type *t, bool validate_hash_type, bool in_key)
{
dds_return_t ret;

if (!validate_hash_type && !xt_is_non_hash (t))
if (!in_key && !validate_hash_type && !xt_is_non_hash (t))
return DDS_RETCODE_OK;

if (ddsi_xt_is_unresolved (t))
Expand All @@ -951,27 +957,48 @@ static dds_return_t xt_validate_impl (struct ddsi_domaingv *gv, const struct xt_
|| (ret = xt_valid_struct_member_ids (gv, t))
|| (ret = xt_valid_type_flags (gv, t->_u.structure.flags, t->_d)))
return ret;

bool has_key_members = false;
for (const struct xt_type *t1 = t; t1 && ddsi_xt_is_resolved (t1); t1 = t1->_u.structure.base_type ? &t1->_u.structure.base_type->xt : NULL)
for (uint32_t n = 0; !has_key_members && n < t1->_u.structure.members.length; n++)
has_key_members = (t1->_u.structure.members.seq[n].flags & DDS_XTypes_IS_KEY);

for (uint32_t n = 0; n < t->_u.structure.members.length; n++)
{
if ((ret = xt_valid_member_flags (gv, t->_u.structure.members.seq[n].flags, MEMBER_FLAG_STRUCT_MEMBER)))
DDS_XTypes_StructMemberFlag flags = t->_u.structure.members.seq[n].flags;
/* A member is considered a key-member (and therefore cannot be optional)
in case (1) the member has a key flag or (2) no member of the struct
and it's base types has a key flag and the 'parent' member (the one
that has this struct as it's member type) is a key (by either rule 1
or 2). As a result, a type can be valid or invalid based on the context
it is used in:
struct A { @optional long x; };
struct B { @key A y; };
struct C { B z; };
Type B is an invalid top-level type for a topic. Type C is a valid
top-level type, but will still be rejected because of the key flag
in B.
*/
bool key = (in_key && !has_key_members) || (flags & DDS_XTypes_IS_KEY);
Copy link
Contributor

Choose a reason for hiding this comment

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

When I reviewed it the first time I completely forgot about base types 😱 Good thing you thought of it now 🙂

So ... on looking at this again ... in

struct A { x; };
struct B { @key A y; };
struct C { B z; };

x is not a key when considering C as the type of a topic, but it would be a key if considering B as the type of a topic. Should that make C fail verification if x were optional? What if no-one tries to make a topic out of B?

I can see arguments both ways: the one combination used has no problems, but then the IDL is nonsensical. So rejecting B and thus also C if x were made optional is reasonable.

I do think it would be wise to point out in a small comment that the key can be true here without the field actually acting as a key field in the topic type, and that we choose to reject cases like the above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the comment in 19a9679

if ((ret = xt_valid_member_flags (gv, flags, MEMBER_FLAG_STRUCT_MEMBER, key)))
return ret;
if ((ret = xt_validate_impl (gv, &t->_u.structure.members.seq[n].type->xt, false)))
if ((ret = xt_validate_impl (gv, &t->_u.structure.members.seq[n].type->xt, false, key)))
return ret;
}
break;
case DDS_XTypes_TK_UNION: {
if (((ret = xt_valid_union_disc_type (gv, t)))
|| (ret = xt_valid_union_member_ids (gv, t))
|| (ret = xt_valid_type_flags (gv, t->_u.union_type.flags, t->_d))
|| (ret = xt_valid_member_flags (gv, t->_u.union_type.disc_flags, MEMBER_FLAG_UNION_DISC)))
|| (ret = xt_valid_member_flags (gv, t->_u.union_type.disc_flags, MEMBER_FLAG_UNION_DISC, in_key)))
return ret;
bool has_default = false;
for (uint32_t n = 0; n < t->_u.union_type.members.length; n++)
{
DDS_XTypes_UnionMemberFlag flags = t->_u.union_type.members.seq[n].flags;
if ((ret = xt_valid_member_flags (gv, flags, MEMBER_FLAG_UNION_MEMBER)))
if ((ret = xt_valid_member_flags (gv, flags, MEMBER_FLAG_UNION_MEMBER, in_key)))
return ret;
if ((ret = xt_validate_impl (gv, &t->_u.union_type.members.seq[n].type->xt, false)))
if ((ret = xt_validate_impl (gv, &t->_u.union_type.members.seq[n].type->xt, false, in_key)))
return ret;
if (flags & DDS_XTypes_IS_DEFAULT)
{
Expand All @@ -992,7 +1019,7 @@ static dds_return_t xt_validate_impl (struct ddsi_domaingv *gv, const struct xt_
if (t->_u.enum_type.bit_bound > 32)
return DDS_RETCODE_BAD_PARAMETER;
for (uint32_t n = 0; n < t->_u.enum_type.literals.length; n++)
if ((ret = xt_valid_member_flags (gv, t->_u.enum_type.literals.seq[n].flags, MEMBER_FLAG_ENUM_LITERAL)))
if ((ret = xt_valid_member_flags (gv, t->_u.enum_type.literals.seq[n].flags, MEMBER_FLAG_ENUM_LITERAL, in_key)))
return ret;
break;
case DDS_XTypes_TK_BITMASK:
Expand All @@ -1002,13 +1029,13 @@ static dds_return_t xt_validate_impl (struct ddsi_domaingv *gv, const struct xt_
if (t->_u.bitmask.bit_bound > 64)
return DDS_RETCODE_BAD_PARAMETER;
for (uint32_t n = 0; n < t->_u.bitmask.bitflags.length; n++)
if ((ret = xt_valid_member_flags (gv, t->_u.bitmask.bitflags.seq[n].flags, MEMBER_FLAG_BIT_FLAG)))
if ((ret = xt_valid_member_flags (gv, t->_u.bitmask.bitflags.seq[n].flags, MEMBER_FLAG_BIT_FLAG, in_key)))
return ret;
break;
case DDS_XTypes_TK_ALIAS:
if ((ret = xt_valid_type_flags (gv, t->_u.alias.flags, t->_d))
|| (ret = xt_valid_member_flags (gv, t->_u.alias.related_flags, MEMBER_FLAG_ALIAS_MEMBER))
|| (ret = xt_validate_impl (gv, &t->_u.alias.related_type->xt, false)))
|| (ret = xt_valid_member_flags (gv, t->_u.alias.related_flags, MEMBER_FLAG_ALIAS_MEMBER, in_key))
|| (ret = xt_validate_impl (gv, &t->_u.alias.related_type->xt, false, in_key)))
return ret;
break;
case DDS_XTypes_TK_BITSET:
Expand All @@ -1021,22 +1048,22 @@ static dds_return_t xt_validate_impl (struct ddsi_domaingv *gv, const struct xt_
break;
case DDS_XTypes_TK_SEQUENCE:
if ((ret = xt_valid_type_flags (gv, t->_u.seq.c.flags, t->_d))
|| (ret = xt_valid_member_flags (gv, t->_u.seq.c.element_flags, MEMBER_FLAG_COLLECTION_ELEMENT))
|| (ret = xt_validate_impl (gv, &t->_u.seq.c.element_type->xt, false)))
|| (ret = xt_valid_member_flags (gv, t->_u.seq.c.element_flags, MEMBER_FLAG_COLLECTION_ELEMENT, in_key))
|| (ret = xt_validate_impl (gv, &t->_u.seq.c.element_type->xt, false, in_key)))
return ret;
break;
case DDS_XTypes_TK_ARRAY:
if ((ret = xt_valid_type_flags (gv, t->_u.array.c.flags, t->_d))
|| (ret = xt_valid_member_flags (gv, t->_u.array.c.element_flags, MEMBER_FLAG_COLLECTION_ELEMENT))
|| (ret = xt_validate_impl (gv, &t->_u.array.c.element_type->xt, false))
|| (ret = xt_valid_member_flags (gv, t->_u.array.c.element_flags, MEMBER_FLAG_COLLECTION_ELEMENT, in_key))
|| (ret = xt_validate_impl (gv, &t->_u.array.c.element_type->xt, false, in_key))
|| (ret = xt_valid_array_bounds (gv, t)))
return ret;
break;
case DDS_XTypes_TK_MAP:
if ((ret = xt_valid_type_flags (gv, t->_u.map.c.flags, t->_d))
|| (ret = xt_valid_member_flags (gv, t->_u.map.c.element_flags, MEMBER_FLAG_COLLECTION_ELEMENT))
|| (ret = xt_validate_impl (gv, &t->_u.map.key_type->xt, false))
|| (ret = xt_validate_impl (gv, &t->_u.map.c.element_type->xt, false)))
|| (ret = xt_valid_member_flags (gv, t->_u.map.c.element_flags, MEMBER_FLAG_COLLECTION_ELEMENT, in_key))
|| (ret = xt_validate_impl (gv, &t->_u.map.key_type->xt, false, in_key))
|| (ret = xt_validate_impl (gv, &t->_u.map.c.element_type->xt, false, in_key)))
return ret;
break;
case DDS_XTypes_TK_BOOLEAN: case DDS_XTypes_TK_BYTE:
Expand All @@ -1054,7 +1081,7 @@ static dds_return_t xt_validate_impl (struct ddsi_domaingv *gv, const struct xt_

dds_return_t ddsi_xt_validate (struct ddsi_domaingv *gv, const struct xt_type *t)
{
return xt_validate_impl (gv, t, true);
return xt_validate_impl (gv, t, true, false);
}

static dds_return_t add_minimal_typeobj (struct ddsi_domaingv *gv, struct xt_type *xt, const struct DDS_XTypes_TypeObject *to)
Expand Down
9 changes: 8 additions & 1 deletion src/idl/src/tree.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3700,7 +3700,14 @@ static bool no_specific_key(const void *node)
{
/* @key(FALSE) is equivalent to missing @key(?) */
if (idl_mask(node) & IDL_STRUCT) {
const idl_member_t *member = ((const idl_struct_t *)node)->members;
const idl_struct_t *_struct = (const idl_struct_t *)node;
if (_struct->inherit_spec)
{
if (!no_specific_key(_struct->inherit_spec->base))
return false;
}

const idl_member_t *member = _struct->members;
for (; member; member = idl_next(member)) {
if (member->key.value)
return false;
Expand Down
5 changes: 5 additions & 0 deletions src/tools/idlc/src/libidlc/libidlc__descriptor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2119,6 +2119,11 @@ static idl_retcode_t get_ctype_keys(const idl_pstate_t *pstate, struct descripto
if (parent_is_key && !ctype->has_key_member)
inst->data.opcode.code |= DDS_OP_FLAG_KEY;
if (inst->data.opcode.code & DDS_OP_FLAG_KEY) {
if (inst->data.opcode.code & DDS_OP_FLAG_OPT) {
idl_error (pstate, ctype->node, "A member that is part of a key (possibly nested inside a struct) cannot be optional");
ret = IDL_RETCODE_SYNTAX_ERROR;
goto err;
}
if ((ret = get_ctype_keys_adr(pstate, descriptor, offs, base_type_ops_offs, inst, ctype, n_keys, &ctype_keys)) != IDL_RETCODE_OK)
goto err;
}
Expand Down
Loading