diff --git a/src/core/ddsc/src/dds_read.c b/src/core/ddsc/src/dds_read.c index 394b7e4..701f50d 100644 --- a/src/core/ddsc/src/dds_read.c +++ b/src/core/ddsc/src/dds_read.c @@ -94,7 +94,6 @@ static dds_return_t dds_read_impl (bool take, dds_entity_t reader_or_condition, rd->m_loan = buf[0]; rd->m_loan_size = maxs; } - nodata_cleanups = NC_RESET_BUF; } else { @@ -103,7 +102,7 @@ static dds_return_t dds_read_impl (bool take, dds_entity_t reader_or_condition, rd->m_loan_size = maxs; } rd->m_loan_out = true; - nodata_cleanups |= NC_CLEAR_LOAN_OUT; + nodata_cleanups = NC_RESET_BUF | NC_CLEAR_LOAN_OUT; } ddsrt_mutex_unlock (&rd->m_entity.m_mutex); } @@ -457,12 +456,11 @@ dds_return_t dds_take_next_wl (dds_entity_t reader, void **buf, dds_sample_info_ dds_return_t dds_return_loan (dds_entity_t reader_or_condition, void **buf, int32_t bufsz) { - const struct ddsi_sertopic *st; dds_reader *rd; dds_entity *entity; - dds_return_t ret = DDS_RETCODE_OK; + dds_return_t ret; - if (buf == NULL || (*buf == NULL && bufsz > 0)) + if (buf == NULL || (buf[0] == NULL && bufsz > 0) || (buf[0] != NULL && bufsz <= 0)) return DDS_RETCODE_BAD_PARAMETER; if ((ret = dds_entity_pin (reader_or_condition, &entity)) < 0) { @@ -476,16 +474,46 @@ dds_return_t dds_return_loan (dds_entity_t reader_or_condition, void **buf, int3 rd = (dds_reader *) entity->m_parent; } - st = rd->m_topic->m_stopic; - for (int32_t i = 0; i < bufsz; i++) - ddsi_sertopic_free_sample (st, buf[i], DDS_FREE_CONTENTS); - - /* If possible return loan buffer to reader */ - ddsrt_mutex_lock (&rd->m_entity.m_mutex); - if (rd->m_loan != 0 && (buf[0] == rd->m_loan)) + if (bufsz <= 0) { - rd->m_loan_out = false; + /* No data whatsoever, or an invocation following a failed read/take call. Read/take + already take care of restoring the state prior to their invocation if they return + no data. Return late so invalid handles can be detected. */ + dds_entity_unpin (entity); + return DDS_RETCODE_OK; + } + assert (buf[0] != NULL); + + const struct ddsi_sertopic *st = rd->m_topic->m_stopic; + + /* The potentially time consuming part of what happens here (freeing samples) + can safely be done without holding the reader lock, but that particular + lock is not used during insertion of data & triggering waitsets (that's + the observer_lock), so holding it for a bit longer in return for simpler + code is a fair trade-off. */ + ddsrt_mutex_lock (&rd->m_entity.m_mutex); + if (buf[0] != rd->m_loan) + { + /* Not so much a loan as a buffer allocated by the middleware on behalf of the + application. So it really is no more than a sophisticated variant of "free". */ + ddsi_sertopic_free_samples (st, buf, (size_t) bufsz, DDS_FREE_ALL); + buf[0] = NULL; + } + else if (!rd->m_loan_out) + { + /* Trying to return a loan that has been returned already */ + ddsrt_mutex_unlock (&rd->m_entity.m_mutex); + dds_entity_unpin (entity); + return DDS_RETCODE_PRECONDITION_NOT_MET; + } + else + { + /* Free only the memory referenced from the samples, not the samples themselves. + Zero them to guarantee the absence of dangling pointers that might cause + trouble on a following operation. FIXME: there's got to be a better way */ + ddsi_sertopic_free_samples (st, buf, (size_t) bufsz, DDS_FREE_CONTENTS); ddsi_sertopic_zero_samples (st, rd->m_loan, rd->m_loan_size); + rd->m_loan_out = false; buf[0] = NULL; } ddsrt_mutex_unlock (&rd->m_entity.m_mutex); diff --git a/src/core/ddsc/tests/CMakeLists.txt b/src/core/ddsc/tests/CMakeLists.txt index e0ed9f1..b2728b9 100644 --- a/src/core/ddsc/tests/CMakeLists.txt +++ b/src/core/ddsc/tests/CMakeLists.txt @@ -30,6 +30,7 @@ set(ddsc_test_sources "instance_get_key.c" "listener.c" "liveliness.c" + "loan.c" "multi_sertopic.c" "participant.c" "publisher.c" @@ -41,7 +42,6 @@ set(ddsc_test_sources "reader_iterator.c" "read_instance.c" "register.c" - "return_loan.c" "subscriber.c" "take_instance.c" "time.c" diff --git a/src/core/ddsc/tests/loan.c b/src/core/ddsc/tests/loan.c new file mode 100644 index 0000000..5e12d84 --- /dev/null +++ b/src/core/ddsc/tests/loan.c @@ -0,0 +1,321 @@ +/* + * Copyright(c) 2006 to 2020 ADLINK Technology Limited and others + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0 which is available at + * http://www.eclipse.org/legal/epl-2.0, or the Eclipse Distribution License + * v. 1.0 which is available at + * http://www.eclipse.org/org/documents/edl-v10.php. + * + * SPDX-License-Identifier: EPL-2.0 OR BSD-3-Clause + */ +#include +#include "dds/dds.h" +#include "test_common.h" + +static dds_entity_t participant, topic, reader, writer, read_condition, read_condition_unread; + +static void create_entities (void) +{ + char topicname[100]; + struct dds_qos *qos; + + create_unique_topic_name ("ddsc_return_loan_test", topicname, sizeof topicname); + participant = dds_create_participant (DDS_DOMAIN_DEFAULT, NULL, NULL); + CU_ASSERT_FATAL (participant > 0); + + qos = dds_create_qos (); + dds_qset_reliability (qos, DDS_RELIABILITY_RELIABLE, 0); + dds_qset_history (qos, DDS_HISTORY_KEEP_ALL, 1); + topic = dds_create_topic (participant, &RoundTripModule_DataType_desc, topicname, qos, NULL); + CU_ASSERT_FATAL (topic > 0); + dds_delete_qos (qos); + + writer = dds_create_writer (participant, topic, NULL, NULL); + CU_ASSERT_FATAL (writer > 0); + reader = dds_create_reader (participant, topic, NULL, NULL); + CU_ASSERT_FATAL (reader > 0); + read_condition = dds_create_readcondition (reader, DDS_ANY_STATE); + CU_ASSERT_FATAL (read_condition > 0); + read_condition_unread = dds_create_readcondition (reader, DDS_ANY_INSTANCE_STATE | DDS_ANY_VIEW_STATE | DDS_NOT_READ_SAMPLE_STATE); + CU_ASSERT_FATAL (read_condition > 0); +} + +static void delete_entities (void) +{ + dds_return_t result; + result = dds_delete (participant); + CU_ASSERT_FATAL (result == DDS_RETCODE_OK); +} + +CU_Test (ddsc_loan, bad_params, .init = create_entities, .fini = delete_entities) +{ + dds_return_t result; + + /* buf = NULL */ + result = dds_return_loan (reader, NULL, -1); + CU_ASSERT (result == DDS_RETCODE_BAD_PARAMETER); + result = dds_return_loan (reader, NULL, 0); + CU_ASSERT (result == DDS_RETCODE_BAD_PARAMETER); + result = dds_return_loan (reader, NULL, 1); + CU_ASSERT (result == DDS_RETCODE_BAD_PARAMETER); + + /* buf[0] = NULL, size > 0 */ + void *buf = NULL; + result = dds_return_loan (reader, &buf, 1); + CU_ASSERT (result == DDS_RETCODE_BAD_PARAMETER); + /* buf[0] != NULL, size <= 0 */ + char dummy = 0; + buf = &dummy; + result = dds_return_loan (reader, &buf, 0); + CU_ASSERT (result == DDS_RETCODE_BAD_PARAMETER); + result = dds_return_loan (reader, &buf, -1); + CU_ASSERT (result == DDS_RETCODE_BAD_PARAMETER); + + /* not a reader or condition (checking only the ones we have at hand) */ + result = dds_return_loan (participant, &buf, 1); + CU_ASSERT (result == DDS_RETCODE_ILLEGAL_OPERATION); + result = dds_return_loan (topic, &buf, 1); + CU_ASSERT (result == DDS_RETCODE_ILLEGAL_OPERATION); +} + +CU_Test (ddsc_loan, success, .init = create_entities, .fini = delete_entities) +{ + const RoundTripModule_DataType s = { + .payload = { + ._length = 1, + ._buffer = (uint8_t[]) { 'a' } + } + }; + const unsigned char zeros[3 * sizeof (s)] = { 0 }; + dds_return_t result; + for (size_t i = 0; i < 3; i++) + { + result = dds_write (writer, &s); + CU_ASSERT_FATAL (result == 0); + } + + /* rely on things like address sanitizer, valgrind for detecting double frees and leaks */ + int32_t n; + void *ptrs[3] = { NULL }; + void *ptr0copy, *ptr1copy; + dds_sample_info_t si[3]; + + /* read 1, return: this should cause memory to be allocated for 1 sample only */ + n = dds_read (reader, ptrs, si, 1, 1); + CU_ASSERT_FATAL (n == 1); + CU_ASSERT_FATAL (ptrs[0] != NULL && ptrs[1] == NULL); + ptr0copy = ptrs[0]; + result = dds_return_loan (reader, ptrs, n); + CU_ASSERT_FATAL (result == DDS_RETCODE_OK); + /* return resets buf[0] (so that it picks up the loan the next time) and zeros the data */ + CU_ASSERT_FATAL (ptrs[0] == NULL); + CU_ASSERT_FATAL (memcmp (ptr0copy, zeros, sizeof (s)) == 0); + + /* read 3, return: should work fine, causes realloc */ + n = dds_read (reader, ptrs, si, 3, 3); + CU_ASSERT_FATAL (n == 3); + CU_ASSERT_FATAL (ptrs[0] != NULL && ptrs[1] != NULL && ptrs[2] != NULL); + ptr0copy = ptrs[0]; + ptr1copy = ptrs[1]; + result = dds_return_loan (reader, ptrs, n); + CU_ASSERT_FATAL (result == DDS_RETCODE_OK); + CU_ASSERT_FATAL (ptrs[0] == NULL); + CU_ASSERT_FATAL (memcmp (ptr0copy, zeros, 3 * sizeof (s)) == 0); + + /* read 1 using loan, expecting to get the same address (no realloc needed), defer return. + Expect ptrs[1] to remain unchanged, although that probably is really an implementation + detail rather than something one might want to rely on */ + n = dds_read (read_condition, ptrs, si, 1, 1); + CU_ASSERT_FATAL (n == 1); + CU_ASSERT_FATAL (ptrs[0] == ptr0copy && ptrs[1] == ptr1copy); + + /* read 3, letting read allocate */ + int32_t n2; + void *ptrs2[3] = { NULL }; + n2 = dds_read (read_condition, ptrs2, si, 3, 3); + CU_ASSERT_FATAL (n2 == 3); + CU_ASSERT_FATAL (ptrs2[0] != NULL && ptrs2[1] != NULL && ptrs2[2] != NULL); + CU_ASSERT_FATAL (ptrs2[0] != ptrs[0]); + + /* contents of first sample should be the same; the point of comparing them + is that valgrind/address sanitizer will get angry with us if one of them + has been freed; can't use memcmp because the sequence buffers should be + at different addresses */ + { + const struct RoundTripModule_DataType *a = ptrs[0]; + const struct RoundTripModule_DataType *b = ptrs2[0]; + CU_ASSERT_FATAL (a->payload._length == b->payload._length); + CU_ASSERT_FATAL (a->payload._buffer != b->payload._buffer); + CU_ASSERT_FATAL (a->payload._buffer[0] == b->payload._buffer[0]); + } + + /* return loan -- to be freed when we delete the reader */ + result = dds_return_loan (read_condition, ptrs, n); + CU_ASSERT_FATAL (result == DDS_RETCODE_OK); + CU_ASSERT_FATAL (ptrs[0] == NULL); + + /* use "dds_return_loan" to free the second result immediately, there's no + easy way to check this happens short of using a custom sertopic */ + ptr0copy = ptrs2[0]; + result = dds_return_loan (read_condition, ptrs2, n2); + CU_ASSERT_FATAL (result == DDS_RETCODE_OK); + CU_ASSERT_FATAL (ptrs2[0] == NULL); + + //This should be a use-after-free + //CU_ASSERT_FATAL (memcmp (ptr0copy, zeros, sizeof (s)) == 0); +} + +CU_Test (ddsc_loan, take_cleanup, .init = create_entities, .fini = delete_entities) +{ + const RoundTripModule_DataType s = { + .payload = { + ._length = 1, + ._buffer = (uint8_t[]) { 'a' } + } + }; + dds_return_t result; + + /* rely on things like address sanitizer, valgrind for detecting double frees and leaks */ + int32_t n; + void *ptrs[3] = { NULL }; + void *ptr0copy; + dds_sample_info_t si[3]; + + /* take 1 from an empty reader: this should cause memory to be allocated for + 1 sample only, be stored as the loan, but not become visisble to the + application */ + n = dds_take (reader, ptrs, si, 1, 1); + CU_ASSERT_FATAL (n == 0); + CU_ASSERT_FATAL (ptrs[0] == NULL && ptrs[1] == NULL); + + /* take 1 that's present: allocates a loan */ + result = dds_write (writer, &s); + CU_ASSERT_FATAL (result == 0); + n = dds_take (reader, ptrs, si, 1, 1); + CU_ASSERT_FATAL (n == 1); + CU_ASSERT_FATAL (ptrs[0] != NULL && ptrs[1] == NULL); + ptr0copy = ptrs[0]; + result = dds_return_loan (reader, ptrs, n); + CU_ASSERT_FATAL (result == DDS_RETCODE_OK); + + /* if it really got handled as a loan, the same address must come out again + (rely on address sanitizer allocating at a different address each time) */ + result = dds_write (writer, &s); + CU_ASSERT_FATAL (result == 0); + n = dds_take (reader, ptrs, si, 1, 1); + CU_ASSERT_FATAL (n == 1); + CU_ASSERT_FATAL (ptrs[0] == ptr0copy && ptrs[1] == NULL); + ptr0copy = ptrs[0]; + result = dds_return_loan (reader, ptrs, n); + CU_ASSERT_FATAL (result == DDS_RETCODE_OK); + + /* take that fails (for lack of data in this case) must reuse the loan, but + hand it back and restore the null pointer */ + n = dds_take (reader, ptrs, si, 1, 1); + CU_ASSERT_FATAL (n == 0); + CU_ASSERT_FATAL (ptrs[0] == NULL && ptrs[1] == NULL); + + /* take that succeeds again must therefore still be using the same address */ + result = dds_write (writer, &s); + CU_ASSERT_FATAL (result == 0); + n = dds_take (reader, ptrs, si, 1, 1); + CU_ASSERT_FATAL (n == 1); + CU_ASSERT_FATAL (ptrs[0] == ptr0copy && ptrs[1] == NULL); + + /* take that fails (with the loan still out) must allocate new memory and + free it */ + int32_t n2; + void *ptrs2[3] = { NULL }; + n2 = dds_take (reader, ptrs2, si, 1, 1); + CU_ASSERT_FATAL (n2 == 0); + CU_ASSERT_FATAL (ptrs2[0] == NULL && ptrs2[1] == NULL); + + /* return the loan and the next take should reuse the memory */ + result = dds_return_loan (reader, ptrs, n); + CU_ASSERT_FATAL (result == DDS_RETCODE_OK); + result = dds_write (writer, &s); + CU_ASSERT_FATAL (result == 0); + n = dds_take (reader, ptrs, si, 1, 1); + CU_ASSERT_FATAL (n == 1); + CU_ASSERT_FATAL (ptrs[0] == ptr0copy && ptrs[1] == NULL); + result = dds_return_loan (reader, ptrs, n); + CU_ASSERT_FATAL (result == DDS_RETCODE_OK); +} + +CU_Test (ddsc_loan, read_cleanup, .init = create_entities, .fini = delete_entities) +{ + const RoundTripModule_DataType s = { + .payload = { + ._length = 1, + ._buffer = (uint8_t[]) { 'a' } + } + }; + dds_return_t result; + + /* rely on things like address sanitizer, valgrind for detecting double frees and leaks */ + int32_t n; + void *ptrs[3] = { NULL }; + void *ptr0copy; + dds_sample_info_t si[3]; + + /* read 1 from an empty reader: this should cause memory to be allocated for + 1 sample only, be stored as the loan, but not become visisble to the + application */ + n = dds_read (reader, ptrs, si, 1, 1); + CU_ASSERT_FATAL (n == 0); + CU_ASSERT_FATAL (ptrs[0] == NULL && ptrs[1] == NULL); + + /* read 1 that's present: allocates a loan */ + result = dds_write (writer, &s); + CU_ASSERT_FATAL (result == 0); + n = dds_take (reader, ptrs, si, 1, 1); + CU_ASSERT_FATAL (n == 1); + CU_ASSERT_FATAL (ptrs[0] != NULL && ptrs[1] == NULL); + ptr0copy = ptrs[0]; + result = dds_return_loan (reader, ptrs, n); + CU_ASSERT_FATAL (result == DDS_RETCODE_OK); + + /* if it really got handled as a loan, the same address must come out again + (rely on address sanitizer allocating at a different address each time) */ + result = dds_write (writer, &s); + CU_ASSERT_FATAL (result == 0); + n = dds_read (read_condition_unread, ptrs, si, 1, 1); + CU_ASSERT_FATAL (n == 1); + CU_ASSERT_FATAL (ptrs[0] == ptr0copy && ptrs[1] == NULL); + ptr0copy = ptrs[0]; + result = dds_return_loan (reader, ptrs, n); + CU_ASSERT_FATAL (result == DDS_RETCODE_OK); + + /* take that fails (for lack of data in this case) must reuse the loan, but + hand it back and restore the null pointer */ + n = dds_read (read_condition_unread, ptrs, si, 1, 1); + CU_ASSERT_FATAL (n == 0); + CU_ASSERT_FATAL (ptrs[0] == NULL && ptrs[1] == NULL); + + /* take that succeeds again must therefore still be using the same address */ + result = dds_write (writer, &s); + CU_ASSERT_FATAL (result == 0); + n = dds_read (read_condition_unread, ptrs, si, 1, 1); + CU_ASSERT_FATAL (n == 1); + CU_ASSERT_FATAL (ptrs[0] == ptr0copy && ptrs[1] == NULL); + + /* take that fails (with the loan still out) must allocate new memory and + free it */ + int32_t n2; + void *ptrs2[3] = { NULL }; + n2 = dds_read (read_condition_unread, ptrs2, si, 1, 1); + CU_ASSERT_FATAL (n2 == 0); + CU_ASSERT_FATAL (ptrs2[0] == NULL && ptrs2[1] == NULL); + + /* return the loan and the next take should reuse the memory */ + result = dds_return_loan (reader, ptrs, n); + CU_ASSERT_FATAL (result == DDS_RETCODE_OK); + result = dds_write (writer, &s); + CU_ASSERT_FATAL (result == 0); + n = dds_read (read_condition_unread, ptrs, si, 1, 1); + CU_ASSERT_FATAL (n == 1); + CU_ASSERT_FATAL (ptrs[0] == ptr0copy && ptrs[1] == NULL); + result = dds_return_loan (reader, ptrs, n); + CU_ASSERT_FATAL (result == DDS_RETCODE_OK); +} diff --git a/src/core/ddsc/tests/return_loan.c b/src/core/ddsc/tests/return_loan.c deleted file mode 100644 index 812e024..0000000 --- a/src/core/ddsc/tests/return_loan.c +++ /dev/null @@ -1,140 +0,0 @@ -/* - * Copyright(c) 2006 to 2018 ADLINK Technology Limited and others - * - * This program and the accompanying materials are made available under the - * terms of the Eclipse Public License v. 2.0 which is available at - * http://www.eclipse.org/legal/epl-2.0, or the Eclipse Distribution License - * v. 1.0 which is available at - * http://www.eclipse.org/org/documents/edl-v10.php. - * - * SPDX-License-Identifier: EPL-2.0 OR BSD-3-Clause - */ -#include "dds/dds.h" -#include "RoundTrip.h" - -#include -#include "CUnit/Test.h" - -static dds_entity_t participant = 0, topic = 0, reader = 0, read_condition = 0; - -static void create_entities(void) -{ - participant = dds_create_participant(DDS_DOMAIN_DEFAULT, NULL, NULL); - CU_ASSERT_FATAL(participant > 0); - - topic = dds_create_topic(participant, &RoundTripModule_DataType_desc, "ddsc_reader_return_loan_RoundTrip", NULL, NULL); - CU_ASSERT_FATAL(topic > 0); - - reader = dds_create_reader(participant, topic, NULL, NULL); - CU_ASSERT_FATAL(reader > 0); - - read_condition = dds_create_readcondition(reader, DDS_ANY_STATE); - CU_ASSERT_FATAL(read_condition > 0); -} - -static void delete_entities(void) -{ - dds_return_t result; - result = dds_delete(participant); - CU_ASSERT_EQUAL_FATAL(result, DDS_RETCODE_OK); - dds_delete(read_condition); -} - -static void** create_loan_buf(size_t sz, bool empty) -{ - size_t i; - void **buf = NULL; - buf = dds_alloc(sz * sizeof(*buf)); - for (i = 0; i < sz; i++) { - buf[i] = dds_alloc(sizeof(RoundTripModule_DataType)); - if (empty) { - memset(buf[i], 0, sizeof(RoundTripModule_DataType)); - } else { - RoundTripModule_DataType *s = buf[i]; - s->payload._maximum = 0; - s->payload._length = 25; - s->payload._buffer = dds_alloc(25); - memset(s->payload._buffer, 'z', 25); - s->payload._release = true; - } - } - return buf; -} - -static void delete_loan_buf(void **buf, size_t sz, bool empty) -{ - size_t i; - for (i = 0; i < sz; i++) { - RoundTripModule_DataType *s = buf[i]; - if (!empty) { - CU_ASSERT(s->payload._length > 0); - if (s->payload._length > 0) { - /* Freed by a successful dds_return_loan */ - dds_free(s->payload._buffer); - } - } - /* dds_return_loan only free's sample contents */ - dds_free(s); - } - dds_free(buf); -} - -/* Verify DDS_RETCODE_BAD_PARAMETER is returned */ -CU_Test(ddsc_reader, return_loan_bad_params, .init = create_entities, .fini = delete_entities) -{ - dds_return_t result; - void **buf = NULL; - - result = dds_return_loan(reader, NULL, 0); - CU_ASSERT_EQUAL(result, DDS_RETCODE_BAD_PARAMETER); - -#ifdef _MSC_VER -#pragma warning(push) -#pragma warning(disable: 6387) -#endif - result = dds_return_loan(reader, buf, 10); -#ifdef _MSC_VER -#pragma warning(pop) -#endif - CU_ASSERT_EQUAL(result, DDS_RETCODE_BAD_PARAMETER); - - buf = create_loan_buf(10, false); -#ifdef _MSC_VER -#pragma warning(push) -#pragma warning(disable: 28020) -#endif - result = dds_return_loan(0, buf, 10); -#ifdef _MSC_VER -#pragma warning(pop) -#endif - CU_ASSERT_EQUAL(result, DDS_RETCODE_BAD_PARAMETER); - - result = dds_return_loan(participant, buf, 0); - CU_ASSERT_EQUAL(result, DDS_RETCODE_ILLEGAL_OPERATION); - - delete_loan_buf(buf, 10, false); -} - -/* Verify DDS_RETCODE_OK is returned */ -CU_Test(ddsc_reader, return_loan_success, .init = create_entities, .fini = delete_entities) -{ - void **buf; - void *buf2 = NULL; - dds_return_t result; - - buf = create_loan_buf(10, false); - result = dds_return_loan(reader, buf, 10); - CU_ASSERT_EQUAL(result, DDS_RETCODE_OK); - - result = dds_return_loan(reader, &buf2, 0); - CU_ASSERT_EQUAL(result, DDS_RETCODE_OK); - delete_loan_buf(buf, 10, true); - - buf = create_loan_buf(10, false); - result = dds_return_loan(read_condition, buf, 10); - CU_ASSERT_EQUAL(result, DDS_RETCODE_OK); - - result = dds_return_loan(read_condition, &buf2, 0); - CU_ASSERT_EQUAL(result, DDS_RETCODE_OK); - delete_loan_buf(buf, 10, true); -}