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

Jump to bottom

Correctly initialize defaulted external sequence #1956

Merged
merged 2 commits into from Mar 28, 2024
Merged

Correctly initialize defaulted external sequence #1956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d07b14
Correctly initialize defaulted external sequence
eboasson Mar 28, 2024
f671ec3
Test initializing defaulted external sequence
eboasson Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 14 additions & 12 deletions src/core/cdr/src/dds_cdrstream.c
View file
Open in desktop
Expand Up @@ -1579,6 +1579,18 @@ static bool stream_is_member_present (uint32_t insn, dds_istream_t * __restrict
return !op_type_optional (insn) || is_mutable_member || dds_is_get1 (is);
}

static const uint32_t *initialize_and_skip_sequence (dds_sequence_t *seq, uint32_t insn, const uint32_t * __restrict ops, enum sample_data_state sample_state)
{
if (sample_state == SAMPLE_DATA_UNINITIALIZED)
{
seq->_buffer = NULL;
seq->_maximum = 0;
seq->_release = true;
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 this could be set to false. Seems that this could result in freeing a NULL pointer in src/core/cdr/src/dds_cdrstream.c:3283, but that won't do any harm in current allocators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I wrote initialization code at line 1958 (or thereabouts) and wondered about how to initialise _release. Then I realised I could refactor things and have a single implementation. The true is simply what it used to do. Therefore I am tempted to leave it as-is in this PR.

}
seq->_length = 0;
return skip_sequence_insns (insn, ops);
}

static const uint32_t *dds_stream_read_seq (dds_istream_t * __restrict is, char * __restrict addr, const struct dds_cdrstream_allocator * __restrict allocator, const uint32_t * __restrict ops, uint32_t insn, enum cdr_data_kind cdr_kind, enum sample_data_state sample_state)
{
dds_sequence_t * const seq = (dds_sequence_t *) addr;
Expand All @@ -1592,16 +1604,7 @@ static const uint32_t *dds_stream_read_seq (dds_istream_t * __restrict is, char

const uint32_t num = dds_is_get4 (is);
if (num == 0)
{
if (sample_state == SAMPLE_DATA_UNINITIALIZED)
{
seq->_buffer = NULL;
seq->_maximum = 0;
seq->_release = true;
}
seq->_length = 0;
return skip_sequence_insns (insn, ops);
}
return initialize_and_skip_sequence (seq, insn, ops, sample_state);

switch (subtype)
{
Expand Down Expand Up @@ -1954,8 +1957,7 @@ static const uint32_t *dds_stream_skip_adr_default (uint32_t insn, char * __rest
return ops + 4;
case DDS_OP_VAL_SEQ: case DDS_OP_VAL_BSQ: {
dds_sequence_t * const seq = (dds_sequence_t *) addr;
seq->_length = 0;
return skip_sequence_insns (insn, ops);
return initialize_and_skip_sequence (seq, insn, ops, sample_state);
}
case DDS_OP_VAL_ARR: {
return skip_array_default (insn, addr, allocator, ops, sample_state);
Expand Down
54 changes: 54 additions & 0 deletions src/core/ddsc/tests/cdrstream.c
View file
Open in desktop
Expand Up @@ -2194,3 +2194,57 @@ CU_Test(ddsc_cdrstream, key_flags_ext)
}
}
#undef D

typedef struct MutStructSeq
{
dds_sequence_long b;
uint8_t c;
} MutStructSeq;

typedef struct ExternMutStructSeq
{
struct MutStructSeq * x;
} ExternMutStructSeq;

static const uint32_t ExternMutStructSeq_ops [] =
{
/* ExternMutStructSeq */
DDS_OP_DLC,
DDS_OP_ADR | DDS_OP_FLAG_OPT | DDS_OP_FLAG_EXT | DDS_OP_TYPE_EXT, offsetof (ExternMutStructSeq, x), (4u << 16u) + 5u /* MutStructSeq */, sizeof (MutStructSeq),
DDS_OP_RTS,

/* MutStructSeq */
DDS_OP_PLC,
DDS_OP_PLM | 5, 1u,
DDS_OP_PLM | 6, 2u,
DDS_OP_RTS,
DDS_OP_ADR | DDS_OP_TYPE_SEQ | DDS_OP_SUBTYPE_4BY | DDS_OP_FLAG_SGN, offsetof (MutStructSeq, b),
DDS_OP_RTS,
DDS_OP_ADR | DDS_OP_TYPE_1BY, offsetof (MutStructSeq, c),
DDS_OP_RTS
};

CU_Test(ddsc_cdrstream, init_sequence_in_external_struct)
{
static uint8_t cdr[] = {
0x0d, 0x00, 0x00, 0x00, // 13 bytes follow for ExternMutStructSeq
0x01, 0x00, 0x00, 0x00, // optional member present + 3x pad
0x05, 0x00, 0x00, 0x00, // 5 bytes follow for MutStructSeq
0x02, 0x00, 0x00, 0x00, // EM: id=2, length code 0 = 1B
0x7b // 123: magic value for "c"
};
struct dds_cdrstream_desc descr;
memset (&descr, 0, sizeof (descr));
dds_cdrstream_desc_init (&descr, &dds_cdrstream_default_allocator, sizeof (ExternMutStructSeq), dds_alignof (ExternMutStructSeq), 0, ExternMutStructSeq_ops, NULL, 0);
uint32_t actual_size;
const bool byteswap = (DDSRT_ENDIAN != DDSRT_LITTLE_ENDIAN);
const bool norm_ok = dds_stream_normalize (cdr, sizeof (cdr), byteswap, DDSI_RTPS_CDR_ENC_VERSION_2, &descr, false, &actual_size);
CU_ASSERT_FATAL (norm_ok && actual_size == sizeof (cdr));
dds_istream_t is;
dds_istream_init (&is, sizeof (cdr), cdr, DDSI_RTPS_CDR_ENC_VERSION_2);
ExternMutStructSeq * sample = ddsrt_calloc (1, sizeof (*sample));
dds_stream_read_sample (&is, sample, &dds_cdrstream_default_allocator, &descr);
dds_stream_free_sample (sample, &dds_cdrstream_default_allocator, descr.ops.ops);
ddsrt_free (sample);
dds_cdrstream_desc_fini (&descr, &dds_cdrstream_default_allocator);
}