msgbuf: Rework error handling to improve soundness
Design the implementation to uphold the invariant that a non-negative
remaining value implies the cursor pointer is valid, and that under
other conditions error values must be observed by the msgbuf user. The
former is tested with assertions in the implementation. The latter is
enforced by construction.
With this change, all msgbuf instances for which
pldm_msgbuf_init_errno() succeeds must be either completed or discarded
by calls to the pldm_msgbuf_complete*() or pldm_msgbuf_discard() APIs
respectively.
We then build on the properties that:
- pldm_msgbuf_init_errno() is marked with the warn_unused_result
function attribute
- pldm_msgbuf_init_errno() returns errors for invalid buffer
configurations
- The complete and discard APIs are marked with the warn_unused_result
function attribute
- The complete APIs test for negative remaining values and return an
error if encountered.
- The discard API propagates the provided error code
Together these provide the foundation to ensure that buffer access
errors are (eventually) detected.
A msgbuf object is always in one of the uninitialized, valid, invalid,
or completed states. The states are defined as follows:
- Uninitialized: Undefined values for remaining and cursor
- Valid: cursor points to a valid object, remaining is both non-negative
and describes a range contained within the object pointed to
by cursor
- Invalid: The value of remaining is negative. The value of cursor is
unspecified.
- Completed: the value of remaining is INTMAX_MIN and cursor is NULL
msgbuf instances must always be in the completed state by the time
their storage is reclaimed. To enforce this, PLDM_MSGBUF_DEFINE_P()
is introduced both to simplify definition of related variables, and
to exploit the compiler's 'cleanup' attribute. The cleanup function
associated with the msgbuf object asserts that the referenced object is
in the completed state.
From there, update the implementations of the msgbuf APIs such that
exceeding implementation type limits forces the msgbuf object to the
invalid state (in addition to returning an error value) to relieve the
caller from testing the result of all API invocations.
Change-Id: I4d78ddc5f567d4148f2f6d8f3e7570e97c316bbb
Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
diff --git a/src/dsp/pdr.c b/src/dsp/pdr.c
index 789ceeb..2d90821 100644
--- a/src/dsp/pdr.c
+++ b/src/dsp/pdr.c
@@ -1553,28 +1553,21 @@
int rc = 0;
uint16_t header_length = 0;
uint8_t num_children = 0;
- struct pldm_msgbuf _src;
- struct pldm_msgbuf *src = &_src;
- struct pldm_msgbuf _dst;
- struct pldm_msgbuf *dst = &_dst;
+ PLDM_MSGBUF_DEFINE_P(src);
+ PLDM_MSGBUF_DEFINE_P(dst);
pldm_pdr_find_record_by_handle(&record, &prev, pdr_record_handle);
if (!record) {
return -EINVAL;
}
- // Initialize msg buffer for record and record->data
- rc = pldm_msgbuf_init_errno(src, PDR_ENTITY_ASSOCIATION_MIN_SIZE,
- record->data, record->size);
- if (rc) {
- return rc;
- }
// check if adding another entity to record causes overflow before
// allocating memory for new_record.
if (record->size + sizeof(pldm_entity) < sizeof(pldm_entity)) {
return -EOVERFLOW;
}
+
pldm_pdr_record *new_record = malloc(sizeof(pldm_pdr_record));
if (!new_record) {
return -ENOMEM;
@@ -1590,12 +1583,19 @@
new_record->size = record->size + sizeof(struct pldm_entity);
new_record->is_remote = record->is_remote;
+ // Initialize msg buffer for record and record->data
+ rc = pldm_msgbuf_init_errno(src, PDR_ENTITY_ASSOCIATION_MIN_SIZE,
+ record->data, record->size);
+ if (rc) {
+ goto cleanup_new_record_data;
+ }
+
// Initialize new PDR record with data from original PDR record.
// Start with adding the header of original PDR
rc = pldm_msgbuf_init_errno(dst, PDR_ENTITY_ASSOCIATION_MIN_SIZE,
new_record->data, new_record->size);
if (rc) {
- goto cleanup_new_record_data;
+ goto cleanup_src_msgbuf;
}
pldm_msgbuf_copy(dst, src, uint32_t, hdr_record_handle);
@@ -1606,13 +1606,13 @@
// size of pldm_entity before inserting the value into new_record.
rc = pldm_msgbuf_extract(src, header_length);
if (rc) {
- goto cleanup_new_record_data;
+ goto cleanup_dst_msgbuf;
}
static_assert(UINT16_MAX < (SIZE_MAX - sizeof(pldm_entity)),
"Fix the following bounds check.");
if (header_length + sizeof(pldm_entity) > UINT16_MAX) {
rc = -EOVERFLOW;
- goto cleanup_new_record_data;
+ goto cleanup_dst_msgbuf;
}
header_length += sizeof(pldm_entity);
pldm_msgbuf_insert(dst, header_length);
@@ -1625,11 +1625,11 @@
// by 1 before insert the value to new record.
rc = pldm_msgbuf_extract(src, num_children);
if (rc) {
- goto cleanup_new_record_data;
+ goto cleanup_dst_msgbuf;
}
if (num_children == UINT8_MAX) {
rc = -EOVERFLOW;
- goto cleanup_new_record_data;
+ goto cleanup_dst_msgbuf;
}
num_children += 1;
pldm_msgbuf_insert(dst, num_children);
@@ -1643,23 +1643,25 @@
// Add new contained entity as a child of new PDR
rc = pldm_msgbuf_complete(src);
if (rc) {
+ rc = pldm_msgbuf_discard(dst, rc);
goto cleanup_new_record_data;
}
rc = pldm_msgbuf_init_errno(src, sizeof(struct pldm_entity), entity,
sizeof(struct pldm_entity));
if (rc) {
+ rc = pldm_msgbuf_discard(dst, rc);
goto cleanup_new_record_data;
}
pldm_msgbuf_copy(dst, src, uint16_t, child_entity_type);
pldm_msgbuf_copy(dst, src, uint16_t, child_entity_instance_num);
pldm_msgbuf_copy(dst, src, uint16_t, child_entity_container_id);
- rc = pldm_msgbuf_complete(src);
- if (rc) {
- goto cleanup_new_record_data;
- }
rc = pldm_msgbuf_complete(dst);
if (rc) {
+ goto cleanup_src_msgbuf;
+ }
+ rc = pldm_msgbuf_complete(src);
+ if (rc) {
goto cleanup_new_record_data;
}
@@ -1671,6 +1673,10 @@
free(record->data);
free(record);
return rc;
+cleanup_dst_msgbuf:
+ rc = pldm_msgbuf_discard(dst, rc);
+cleanup_src_msgbuf:
+ rc = pldm_msgbuf_discard(src, rc);
cleanup_new_record_data:
free(new_record->data);
cleanup_new_record:
@@ -1697,12 +1703,9 @@
uint16_t new_pdr_size;
uint16_t container_id = 0;
void *container_id_addr;
- struct pldm_msgbuf _dst;
- struct pldm_msgbuf *dst = &_dst;
- struct pldm_msgbuf _src_p;
- struct pldm_msgbuf *src_p = &_src_p;
- struct pldm_msgbuf _src_c;
- struct pldm_msgbuf *src_c = &_src_c;
+ PLDM_MSGBUF_DEFINE_P(dst);
+ PLDM_MSGBUF_DEFINE_P(src_p);
+ PLDM_MSGBUF_DEFINE_P(src_c);
int rc = 0;
pldm_pdr_record *prev = repo->first;
@@ -1757,13 +1760,13 @@
rc = pldm_msgbuf_init_errno(src_p, sizeof(struct pldm_entity), parent,
sizeof(*parent));
if (rc) {
- goto cleanup_new_record_data;
+ goto cleanup_msgbuf_dst;
}
rc = pldm_msgbuf_init_errno(src_c, sizeof(struct pldm_entity), entity,
sizeof(*entity));
if (rc) {
- goto cleanup_new_record_data;
+ goto cleanup_msgbuf_src_p;
}
container_id_addr = NULL;
@@ -1771,7 +1774,7 @@
rc = pldm_msgbuf_span_required(dst, sizeof(container_id),
(void **)&container_id_addr);
if (rc) {
- goto cleanup_new_record_data;
+ goto cleanup_msgbuf_src_c;
}
assert(container_id_addr);
pldm_msgbuf_insert_uint8(dst, PLDM_ENTITY_ASSOCIAION_PHYSICAL);
@@ -1791,15 +1794,15 @@
container_id = htole16(container_id);
memcpy(container_id_addr, &container_id, sizeof(uint16_t));
- rc = pldm_msgbuf_complete(dst);
+ rc = pldm_msgbuf_complete(src_c);
if (rc) {
- goto cleanup_new_record_data;
+ goto cleanup_msgbuf_src_p;
}
rc = pldm_msgbuf_complete(src_p);
if (rc) {
- goto cleanup_new_record_data;
+ goto cleanup_msgbuf_dst;
}
- rc = pldm_msgbuf_complete(src_c);
+ rc = pldm_msgbuf_complete(dst);
if (rc) {
goto cleanup_new_record_data;
}
@@ -1810,6 +1813,12 @@
}
return rc;
+cleanup_msgbuf_src_c:
+ rc = pldm_msgbuf_discard(src_c, rc);
+cleanup_msgbuf_src_p:
+ rc = pldm_msgbuf_discard(src_p, rc);
+cleanup_msgbuf_dst:
+ rc = pldm_msgbuf_discard(dst, rc);
cleanup_new_record_data:
free(new_record->data);
cleanup_new_record:
@@ -1839,10 +1848,10 @@
int rc = 0;
size_t skip_data_size = 0;
pldm_pdr_record *record = repo->first;
- struct pldm_msgbuf _dst;
- struct pldm_msgbuf *dst = &_dst;
while (record != NULL) {
+ PLDM_MSGBUF_DEFINE_P(dst);
+
rc = pldm_msgbuf_init_errno(dst,
PDR_ENTITY_ASSOCIATION_MIN_SIZE,
record->data, record->size);
@@ -1851,7 +1860,10 @@
}
skip_data_size = sizeof(uint32_t) + sizeof(uint8_t);
pldm_msgbuf_span_required(dst, skip_data_size, NULL);
- pldm_msgbuf_extract(dst, hdr_type);
+ rc = pldm_msgbuf_extract(dst, hdr_type);
+ if (rc) {
+ return pldm_msgbuf_discard(dst, rc);
+ }
if (record->is_remote != is_remote ||
hdr_type != PLDM_PDR_ENTITY_ASSOCIATION) {
goto cleanup;
@@ -1860,7 +1872,10 @@
sizeof(uint16_t) + sizeof(uint8_t) +
sizeof(struct pldm_entity);
pldm_msgbuf_span_required(dst, skip_data_size, NULL);
- pldm_msgbuf_extract(dst, num_children);
+ rc = pldm_msgbuf_extract(dst, num_children);
+ if (rc) {
+ return pldm_msgbuf_discard(dst, rc);
+ }
for (int i = 0; i < num_children; ++i) {
struct pldm_entity e;
@@ -1869,12 +1884,12 @@
e.entity_instance_num)) ||
(rc = pldm_msgbuf_extract(dst,
e.entity_container_id))) {
- return rc;
+ return pldm_msgbuf_discard(dst, rc);
}
if (pldm_entity_cmp(entity, &e)) {
*record_handle = record->record_handle;
- return 0;
+ return pldm_msgbuf_complete(dst);
}
}
cleanup:
@@ -1894,10 +1909,8 @@
{
uint16_t header_length = 0;
uint8_t num_children = 0;
- struct pldm_msgbuf _src;
- struct pldm_msgbuf *src = &_src;
- struct pldm_msgbuf _dst;
- struct pldm_msgbuf *dst = &_dst;
+ PLDM_MSGBUF_DEFINE_P(src);
+ PLDM_MSGBUF_DEFINE_P(dst);
int rc;
pldm_pdr_record *record;
pldm_pdr_record *prev;
@@ -1917,12 +1930,6 @@
if (!record) {
return -EINVAL;
}
- // Initialize msg buffer for record and record->data
- rc = pldm_msgbuf_init_errno(src, PDR_ENTITY_ASSOCIATION_MIN_SIZE,
- record->data, record->size);
- if (rc) {
- return rc;
- }
// check if removing an entity from record causes overflow before
// allocating memory for new_record.
if (record->size < sizeof(pldm_entity)) {
@@ -1941,13 +1948,20 @@
new_record->size = record->size - sizeof(struct pldm_entity);
new_record->is_remote = record->is_remote;
+ // Initialize msg buffer for record and record->data
+ rc = pldm_msgbuf_init_errno(src, PDR_ENTITY_ASSOCIATION_MIN_SIZE,
+ record->data, record->size);
+ if (rc) {
+ goto cleanup_new_record_data;
+ }
+
// Initialize new PDR record with data from original PDR record.
// Start with adding the header of original PDR
rc = pldm_msgbuf_init_errno(
dst, (PDR_ENTITY_ASSOCIATION_MIN_SIZE - sizeof(pldm_entity)),
new_record->data, new_record->size);
if (rc) {
- goto cleanup_new_record_data;
+ goto cleanup_msgbuf_src;
}
pldm_msgbuf_copy(dst, src, uint32_t, hdr_record_handle);
pldm_msgbuf_copy(dst, src, uint8_t, hdr_version);
@@ -1957,11 +1971,11 @@
// size of pldm_entity before inserting the value into new_record.
rc = pldm_msgbuf_extract(src, header_length);
if (rc) {
- goto cleanup_new_record_data;
+ goto cleanup_msgbuf_dst;
}
if (header_length < sizeof(pldm_entity)) {
rc = -EOVERFLOW;
- goto cleanup_new_record_data;
+ goto cleanup_msgbuf_dst;
}
header_length -= sizeof(pldm_entity);
pldm_msgbuf_insert(dst, header_length);
@@ -1974,16 +1988,16 @@
// by 1 before insert the value to new record.
rc = pldm_msgbuf_extract(src, num_children);
if (rc) {
- goto cleanup_new_record_data;
+ goto cleanup_msgbuf_dst;
}
if (num_children == 1) {
// This is the last child which is getting removed so we need to delete the Entity Association PDR.
pldm_pdr_remove_record(repo, record,
pldm_pdr_get_prev_record(repo, record));
- goto cleanup_new_record_data;
+ goto cleanup_msgbuf_dst;
} else if (num_children < 1) {
rc = -EOVERFLOW;
- goto cleanup_new_record_data;
+ goto cleanup_msgbuf_dst;
}
num_children -= 1;
pldm_msgbuf_insert(dst, num_children);
@@ -1994,7 +2008,7 @@
if ((rc = pldm_msgbuf_extract(src, e.entity_type)) ||
(rc = pldm_msgbuf_extract(src, e.entity_instance_num)) ||
(rc = pldm_msgbuf_extract(src, e.entity_container_id))) {
- goto cleanup_new_record_data;
+ goto cleanup_msgbuf_dst;
}
if (pldm_entity_cmp(entity, &e)) {
@@ -2006,9 +2020,18 @@
pldm_msgbuf_insert(dst, e.entity_container_id);
}
- if ((rc = pldm_msgbuf_complete(src)) ||
- (rc = pldm_msgbuf_complete(dst)) ||
- (rc = pldm_pdr_replace_record(repo, record, prev, new_record))) {
+ rc = pldm_msgbuf_complete(dst);
+ if (rc) {
+ goto cleanup_msgbuf_src;
+ }
+
+ rc = pldm_msgbuf_complete(src);
+ if (rc) {
+ goto cleanup_new_record_data;
+ }
+
+ rc = pldm_pdr_replace_record(repo, record, prev, new_record);
+ if (rc) {
goto cleanup_new_record_data;
}
@@ -2016,6 +2039,10 @@
free(record);
return rc;
+cleanup_msgbuf_dst:
+ rc = pldm_msgbuf_discard(dst, rc);
+cleanup_msgbuf_src:
+ rc = pldm_msgbuf_discard(src, rc);
cleanup_new_record_data:
free(new_record->data);
cleanup_new_record:
@@ -2066,8 +2093,7 @@
uint16_t record_fru_rsi = 0;
uint8_t *skip_data = NULL;
uint8_t skip_data_size = 0;
- struct pldm_msgbuf _dst;
- struct pldm_msgbuf *dst = &_dst;
+ PLDM_MSGBUF_DEFINE_P(dst);
int rc = 0;
rc = pldm_msgbuf_init_errno(dst, PDR_FRU_RECORD_SET_MIN_SIZE,
@@ -2141,20 +2167,22 @@
record = repo->first;
while (record != NULL) {
- struct pldm_msgbuf _buf;
- struct pldm_msgbuf *buf = &_buf;
+ PLDM_MSGBUF_DEFINE_P(buf);
+
rc = pldm_msgbuf_init_errno(buf, PDR_FRU_RECORD_SET_MIN_SIZE,
record->data, record->size);
if (rc) {
return rc;
}
pldm_msgbuf_span_required(buf, skip_data_size, NULL);
- if ((rc = pldm_msgbuf_extract(buf, hdr_type))) {
+ pldm_msgbuf_extract(buf, hdr_type);
+ rc = pldm_msgbuf_complete(buf);
+ if (rc) {
return rc;
}
if (record->is_remote != is_remote ||
hdr_type != PLDM_PDR_FRU_RECORD_SET) {
- goto cleanup;
+ goto next;
}
match = pldm_pdr_record_matches_fru_rsi(record, fru_rsi);
if (match < 0) {
@@ -2165,11 +2193,7 @@
prev = pldm_pdr_get_prev_record(repo, record);
return pldm_pdr_remove_record(repo, record, prev);
}
- cleanup:
- rc = pldm_msgbuf_complete(buf);
- if (rc) {
- return rc;
- }
+ next:
record = record->next;
}
return rc;