Skip to content

Commit 1b318b3

Browse files
serges147serges147
authored
Fixed issue #216 "canardMemFree must provide size" (#233)
- Extended `CanardTxQueueItem` with extra `allocated_size` field to remember original size allocated for the item (and its embedded payload). - Extended `CanardRxTransfer` with extra `allocated_size` field to report to the client original size allocated for the payload buffer (which is normally equal to session `extent`).
1 parent 03fc5fe commit 1b318b3

10 files changed

Lines changed: 148 additions & 69 deletions

File tree

‎README.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,10 @@ static void* memAllocate(CanardInstance* const canard, const size_t amount)
7171
return o1heapAllocate(my_allocator, amount);
7272
}
7373

74-
static void memFree(CanardInstance* const canard, void* const pointer)
74+
static void memFree(CanardInstance* const canard, void* const pointer, const size_t amount)
7575
{
7676
(void) canard;
77+
(void) amount;
7778
o1heapFree(my_allocator, pointer);
7879
}
7980
```

‎libcanard/canard.c‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -303,9 +303,12 @@ CANARD_PRIVATE TxItem* txAllocateQueueItem(CanardInstance* const ins,
303303
{
304304
CANARD_ASSERT(ins != NULL);
305305
CANARD_ASSERT(payload_size > 0U);
306-
TxItem* const out = (TxItem*) ins->memory_allocate(ins, (sizeof(TxItem) - CANARD_MTU_MAX) + payload_size);
306+
const size_t tx_item_size = (sizeof(TxItem) - CANARD_MTU_MAX) + payload_size;
307+
TxItem* const out = (TxItem*) ins->memory_allocate(ins, tx_item_size);
307308
if (out != NULL)
308309
{
310+
out->base.allocated_size = tx_item_size;
311+
309312
out->base.base.up = NULL;
310313
out->base.base.lr[0] = NULL;
311314
out->base.base.lr[1] = NULL;
@@ -532,7 +535,7 @@ CANARD_PRIVATE int32_t txPushMultiFrame(CanardTxQueue* const que,
532535
while (head != NULL)
533536
{
534537
CanardTxQueueItem* const next = head->next_in_transfer;
535-
ins->memory_free(ins, head);
538+
ins->memory_free(ins, head, head->allocated_size);
536539
head = next;
537540
}
538541
}
@@ -729,11 +732,13 @@ CANARD_PRIVATE int8_t rxSessionWritePayload(CanardInstance* const ins,
729732
return out;
730733
}
731734

732-
CANARD_PRIVATE void rxSessionRestart(CanardInstance* const ins, CanardInternalRxSession* const rxs)
735+
CANARD_PRIVATE void rxSessionRestart(CanardInstance* const ins,
736+
CanardInternalRxSession* const rxs,
737+
const size_t allocated_size)
733738
{
734739
CANARD_ASSERT(ins != NULL);
735740
CANARD_ASSERT(rxs != NULL);
736-
ins->memory_free(ins, rxs->payload); // May be NULL, which is OK.
741+
ins->memory_free(ins, rxs->payload, allocated_size); // May be NULL, which is OK.
737742
rxs->total_payload_size = 0U;
738743
rxs->payload_size = 0U;
739744
rxs->payload = NULL;
@@ -773,7 +778,7 @@ CANARD_PRIVATE int8_t rxSessionAcceptFrame(CanardInstance* const ins,
773778
if (out < 0)
774779
{
775780
CANARD_ASSERT(-CANARD_ERROR_OUT_OF_MEMORY == out);
776-
rxSessionRestart(ins, rxs); // Out-of-memory.
781+
rxSessionRestart(ins, rxs, extent); // Out-of-memory.
777782
}
778783
else if (frame->end_of_transfer)
779784
{
@@ -785,6 +790,7 @@ CANARD_PRIVATE int8_t rxSessionAcceptFrame(CanardInstance* const ins,
785790
out_transfer->timestamp_usec = rxs->transfer_timestamp_usec;
786791
out_transfer->payload_size = rxs->payload_size;
787792
out_transfer->payload = rxs->payload;
793+
out_transfer->allocated_size = (rxs->payload != NULL) ? extent : 0U;
788794

789795
// Cut off the CRC from the payload if it's there -- we don't want to expose it to the user.
790796
CANARD_ASSERT(rxs->total_payload_size >= rxs->payload_size);
@@ -797,7 +803,7 @@ CANARD_PRIVATE int8_t rxSessionAcceptFrame(CanardInstance* const ins,
797803

798804
rxs->payload = NULL; // Ownership passed over to the application, nullify to prevent freeing.
799805
}
800-
rxSessionRestart(ins, rxs); // Successful completion.
806+
rxSessionRestart(ins, rxs, extent); // Successful completion.
801807
}
802808
else
803809
{
@@ -978,6 +984,7 @@ CANARD_PRIVATE int8_t rxAcceptFrame(CanardInstance* const ins,
978984
out_transfer->timestamp_usec = frame->timestamp_usec;
979985
out_transfer->payload_size = payload_size;
980986
out_transfer->payload = payload;
987+
out_transfer->allocated_size = payload_size;
981988
// Clang-Tidy raises an error recommending the use of memcpy_s() instead.
982989
// We ignore it because the safe functions are poorly supported; reliance on them may limit the portability.
983990
(void) memcpy(payload, frame->payload, payload_size); // NOLINT
@@ -1236,8 +1243,11 @@ int8_t canardRxUnsubscribe(CanardInstance* const ins,
12361243
out = 1;
12371244
for (size_t i = 0; i < RX_SESSIONS_PER_SUBSCRIPTION; i++)
12381245
{
1239-
ins->memory_free(ins, (sub->sessions[i] != NULL) ? sub->sessions[i]->payload : NULL);
1240-
ins->memory_free(ins, sub->sessions[i]);
1246+
if (sub->sessions[i] != NULL)
1247+
{
1248+
ins->memory_free(ins, sub->sessions[i]->payload, sub->extent);
1249+
}
1250+
ins->memory_free(ins, sub->sessions[i], sizeof(*sub->sessions[i]));
12411251
sub->sessions[i] = NULL;
12421252
}
12431253
}

‎libcanard/canard.h‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,10 @@ struct CanardTxQueueItem
285285
/// Frames whose transmission deadline is in the past shall be dropped.
286286
CanardMicrosecond tx_deadline_usec;
287287

288+
/// Amount of memory allocated for the whole this item, including the payload frame.
289+
/// In use to deallocate the item by passing this value to the memory manager (`memFree`).
290+
size_t allocated_size;
291+
288292
/// The actual CAN frame data.
289293
CanardFrame frame;
290294
};
@@ -332,10 +336,19 @@ typedef struct CanardRxTransfer
332336
/// The time system may be arbitrary as long as the clock is monotonic (steady).
333337
CanardMicrosecond timestamp_usec;
334338

339+
/// Size of the payload data in bytes.
340+
/// The value is always less than or equal to the extent specified in the subscription.
335341
/// If the payload is empty (payload_size = 0), the payload pointer may be NULL.
336-
/// The application is required to deallocate the payload buffer after the transfer is processed.
337342
size_t payload_size;
338-
void* payload;
343+
344+
/// The application is required to deallocate the payload buffer after the transfer is processed.
345+
/// Allocated buffer size (`allocated_size`, not `payload_size`) should be used to deallocate the buffer.
346+
void* payload;
347+
348+
/// Size of the allocated payload buffer in bytes.
349+
/// Normally equal to the extent specified in the subscription, but could be less (equal to `payload_size`)
350+
/// in case of single frame transfer, or even zero if the payload pointer is NULL.
351+
size_t allocated_size;
339352
} CanardRxTransfer;
340353

341354
/// A pointer to the memory allocation function. The semantics are similar to malloc():
@@ -349,11 +362,12 @@ typedef struct CanardRxTransfer
349362
typedef void* (*CanardMemoryAllocate)(CanardInstance* ins, size_t amount);
350363

351364
/// The counterpart of the above -- this function is invoked to return previously allocated memory to the allocator.
352-
/// The semantics are similar to free():
365+
/// The semantics are similar to free(), but with additional `amount` parameter:
353366
/// - The pointer was previously returned by the allocation function.
354367
/// - The pointer may be NULL, in which case the function shall have no effect.
355368
/// - The execution time should be constant (O(1)).
356-
typedef void (*CanardMemoryFree)(CanardInstance* ins, void* pointer);
369+
/// - The amount is the same as it was during allocation.
370+
typedef void (*CanardMemoryFree)(CanardInstance* ins, void* pointer, size_t amount);
357371

358372
/// This is the core structure that keeps all of the states and allocated resources of the library instance.
359373
struct CanardInstance

‎tests/exposed.hpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ auto rxSessionWritePayload(CanardInstance* const ins,
107107
const std::size_t payload_size,
108108
const void* const payload) -> std::int8_t;
109109

110-
void rxSessionRestart(CanardInstance* const ins, RxSession* const rxs);
110+
void rxSessionRestart(CanardInstance* const ins, RxSession* const rxs, const std::size_t allocated_size);
111111

112112
auto rxSessionUpdate(CanardInstance* const ins,
113113
RxSession* const rxs,

‎tests/helpers.hpp‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ class TestAllocator
103103
return p;
104104
}
105105

106-
void deallocate(void* const user_pointer)
106+
void deallocate(void* const user_pointer, const std::size_t amount)
107107
{
108108
if (user_pointer != nullptr)
109109
{
@@ -114,7 +114,12 @@ class TestAllocator
114114
throw std::logic_error("Attempted to deallocate memory that was never allocated; ptr=" +
115115
std::to_string(reinterpret_cast<std::uint64_t>(user_pointer)));
116116
}
117-
const auto [p, amount] = *it;
117+
const auto [p, expected_amount] = *it;
118+
if (amount != expected_amount)
119+
{
120+
throw std::logic_error("Attempted to deallocate wrong size memory at ptr=" +
121+
std::to_string(reinterpret_cast<std::uint64_t>(user_pointer)));
122+
}
118123
if ((0 != std::memcmp(p - canary_.size(), canary_.begin(), canary_.size())) ||
119124
(0 != std::memcmp(p + amount, canary_.begin(), canary_.size())))
120125
{
@@ -240,10 +245,10 @@ class Instance
240245
return p->allocator_.allocate(amount);
241246
}
242247

243-
static void trampolineDeallocate(CanardInstance* const ins, void* const pointer)
248+
static void trampolineDeallocate(CanardInstance* const ins, void* const pointer, const std::size_t amount)
244249
{
245250
auto* p = reinterpret_cast<Instance*>(ins->user_reference);
246-
p->allocator_.deallocate(pointer);
251+
p->allocator_.deallocate(pointer, amount);
247252
}
248253

249254
CanardInstance canard_ = canardInit(&Instance::trampolineAllocate, &Instance::trampolineDeallocate);

‎tests/test_private_rx.cpp‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,7 @@ TEST_CASE("rxSessionWritePayload")
229229
REQUIRE(rxs.payload[9] == 9);
230230

231231
// Restart frees the buffer. The transfer-ID will be incremented, too.
232-
rxSessionRestart(&ins.getInstance(), &rxs);
232+
rxSessionRestart(&ins.getInstance(), &rxs, 10);
233233
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 0);
234234
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 0);
235235
REQUIRE(rxs.payload_size == 0);
@@ -242,7 +242,7 @@ TEST_CASE("rxSessionWritePayload")
242242
rxs.calculated_crc = 0x1234U;
243243
rxs.transfer_id = 23;
244244
rxs.toggle = false;
245-
rxSessionRestart(&ins.getInstance(), &rxs);
245+
rxSessionRestart(&ins.getInstance(), &rxs, 10);
246246
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 0);
247247
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 0);
248248
REQUIRE(rxs.payload_size == 0);
@@ -255,7 +255,7 @@ TEST_CASE("rxSessionWritePayload")
255255
rxs.calculated_crc = 0x1234U;
256256
rxs.transfer_id = 31;
257257
rxs.toggle = false;
258-
rxSessionRestart(&ins.getInstance(), &rxs);
258+
rxSessionRestart(&ins.getInstance(), &rxs, 10);
259259
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 0);
260260
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 0);
261261
REQUIRE(rxs.payload_size == 0);
@@ -340,10 +340,11 @@ TEST_CASE("rxSessionUpdate")
340340
REQUIRE(transfer.metadata.remote_node_id == 55);
341341
REQUIRE(transfer.metadata.transfer_id == 11);
342342
REQUIRE(transfer.payload_size == 3);
343+
REQUIRE(transfer.allocated_size == 16);
343344
REQUIRE(0 == std::memcmp(transfer.payload, "\x01\x01\x01", 3));
344345
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);
345346
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16);
346-
ins.getAllocator().deallocate(transfer.payload);
347+
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);
347348

348349
// Valid next transfer, wrong transport.
349350
frame.timestamp_usec = 10'000'100;
@@ -376,10 +377,11 @@ TEST_CASE("rxSessionUpdate")
376377
REQUIRE(transfer.metadata.remote_node_id == 55);
377378
REQUIRE(transfer.metadata.transfer_id == 12);
378379
REQUIRE(transfer.payload_size == 3);
380+
REQUIRE(transfer.allocated_size == 16);
379381
REQUIRE(0 == std::memcmp(transfer.payload, "\x03\x03\x03", 3));
380382
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);
381383
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16);
382-
ins.getAllocator().deallocate(transfer.payload);
384+
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);
383385

384386
// Same TID.
385387
frame.timestamp_usec = 10'000'200;
@@ -413,10 +415,11 @@ TEST_CASE("rxSessionUpdate")
413415
REQUIRE(transfer.metadata.remote_node_id == 55);
414416
REQUIRE(transfer.metadata.transfer_id == 12);
415417
REQUIRE(transfer.payload_size == 3);
418+
REQUIRE(transfer.allocated_size == 16);
416419
REQUIRE(0 == std::memcmp(transfer.payload, "\x05\x05\x05", 3));
417420
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);
418421
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16);
419-
ins.getAllocator().deallocate(transfer.payload);
422+
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);
420423

421424
// Restart due to TID timeout, switch iface.
422425
frame.timestamp_usec = 20'000'000;
@@ -437,10 +440,11 @@ TEST_CASE("rxSessionUpdate")
437440
REQUIRE(transfer.metadata.remote_node_id == 55);
438441
REQUIRE(transfer.metadata.transfer_id == 11);
439442
REQUIRE(transfer.payload_size == 3);
443+
REQUIRE(transfer.allocated_size == 16);
440444
REQUIRE(0 == std::memcmp(transfer.payload, "\x05\x05\x05", 3));
441445
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);
442446
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16);
443-
ins.getAllocator().deallocate(transfer.payload);
447+
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);
444448

445449
// Multi-frame, first.
446450
frame.timestamp_usec = 20'000'100;
@@ -515,10 +519,11 @@ TEST_CASE("rxSessionUpdate")
515519
REQUIRE(transfer.metadata.remote_node_id == 55);
516520
REQUIRE(transfer.metadata.transfer_id == 13);
517521
REQUIRE(transfer.payload_size == 16);
522+
REQUIRE(transfer.allocated_size == 16);
518523
REQUIRE(0 == std::memcmp(transfer.payload, "\x06\x06\x06\x06\x06\x06\x06\x07\x07\x07\x07\x07\x07\x07\x09\x09", 16));
519524
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);
520525
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16);
521-
ins.getAllocator().deallocate(transfer.payload);
526+
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);
522527

523528
// TID timeout does not occur until SOT; see https://github.com/OpenCyphal/libcanard/issues/212.
524529
frame.timestamp_usec = 30'000'000;
@@ -616,10 +621,11 @@ TEST_CASE("rxSessionUpdate")
616621
REQUIRE(transfer.metadata.remote_node_id == 55);
617622
REQUIRE(transfer.metadata.transfer_id == 10);
618623
REQUIRE(transfer.payload_size == 10);
624+
REQUIRE(transfer.allocated_size == 16);
619625
REQUIRE(0 == std::memcmp(transfer.payload, "\x0B\x0B\x0B\x0B\x0B\x0B\x0B\x0D\x0D\x0D", 10));
620626
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);
621627
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16);
622-
ins.getAllocator().deallocate(transfer.payload);
628+
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);
623629

624630
// CRC SPLIT -- first frame.
625631
frame.timestamp_usec = 30'000'000;
@@ -663,10 +669,11 @@ TEST_CASE("rxSessionUpdate")
663669
REQUIRE(transfer.metadata.remote_node_id == 55);
664670
REQUIRE(transfer.metadata.transfer_id == 0);
665671
REQUIRE(transfer.payload_size == 7); // ONE CRC BYTE BACKTRACKED!
672+
REQUIRE(transfer.allocated_size == 16);
666673
REQUIRE(0 == std::memcmp(transfer.payload, "\x0E\x0E\x0E\x0E\x0E\x0E\x0E", 7));
667674
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);
668675
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16);
669-
ins.getAllocator().deallocate(transfer.payload);
676+
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);
670677

671678
// BAD CRC -- first frame.
672679
frame.timestamp_usec = 30'001'000;

‎tests/test_public_roundtrip.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -191,9 +191,10 @@ TEST_CASE("RoundtripSimple")
191191
else
192192
{
193193
REQUIRE(transfer.payload_size == 0U);
194+
REQUIRE(transfer.allocated_size == 0U);
194195
}
195196

196-
ins_rx.getAllocator().deallocate(transfer.payload);
197+
ins_rx.getAllocator().deallocate(transfer.payload, transfer.allocated_size);
197198
std::free(ref_payload); // NOLINT
198199
}
199200
else
@@ -212,7 +213,7 @@ TEST_CASE("RoundtripSimple")
212213

213214
{
214215
const std::lock_guard locker(lock);
215-
ins_tx.getAllocator().deallocate(ti);
216+
ins_tx.getAllocator().deallocate(ti, (ti != nullptr) ? ti->allocated_size : 0);
216217
}
217218

218219
if (std::chrono::steady_clock::now() > deadline)

0 commit comments

Comments
 (0)