Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors the CSP (Cubesat Space Protocol) packet encryption to perform in-place cryptographic operations, eliminating static buffers and reducing unnecessary memory copies. The changes improve thread safety by removing shared static buffers and enhance code clarity with better variable naming and comprehensive documentation.
Key changes:
- Removed static
packet_encandpacket_decbuffers from the interface data structure, replacing them with a singlerx_packetpointer for better memory efficiency - Refactored
crypto_encryptandcrypto_decryptfunctions to operate directly on packet buffers using pointer arithmetic - Added validation for packet size limits and buffer overflow protection
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/csp_if_cblk.c | Modified TX/RX functions to work with in-place encryption; added packet size validation and buffer overflow checks; moved lock acquisition after encryption to avoid holding locks during crypto operations |
| src/crypto/crypto.c | Refactored encryption/decryption functions to operate in-place on packet structures; removed randombytes function (tweetnacl-specific); added comprehensive documentation for memory layout transformations; added compile-time assertion for buffer padding requirements |
| include/crypto/crypto.h | Updated function signatures to accept csp_packet_t* instead of separate buffer pointers; added CSP_ID2_HEADER_SIZE constant |
| include/cblk/csp_if_cblk.h | Renamed data_length field to packet_length for clarity; replaced static buffer arrays with single rx_packet pointer; added CBLK_MAX_FRAMES_PER_PACKET and CRYPTO_MAC_SIZE constants |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 11 comments.
Comments suppressed due to low confidence (1)
src/csp_if_cblk.c:181
- Resource leak: When frames are missing (out of sequence),
ifdata->rx_packetis not reset. The partially filled packet buffer from the previous reception is leaked. Consider addingcsp_buffer_free(ifdata->rx_packet)andifdata->rx_packet = csp_buffer_get(0)to properly clean up and reset state when packet reception is aborted.
} else if (ifdata->rx_frame_idx + 1 != frame->hdr.ccsds_frame_idx || ifdata->rx_packet_idx != frame->hdr.csp_packet_idx) {
/* We are missing part of the received CSP frame */
if (_cblk_rx_debug >= 1) {
printf("Part of CSP frame is missing: Received part %"PRIu8" of %"PRIu8", expected part %"PRIu8" of %"PRIu8"\n",
frame->hdr.ccsds_frame_idx, frame->hdr.csp_packet_idx, ifdata->rx_frame_idx+1, ifdata->rx_packet_idx);
}
iface->frame++;
return CSP_ERR_HMAC;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Removed all static buffers. Removed a bunch of memcpy operations. Better thread safety.
Backwards compatible with old version that has 16 byte zero fill after cipher, before nonce + tx id.
Changed variable named to better distinguish between frames (layer 2) packets (layer 3)
Added more comments to describe the multiple headers and offsets involved in the NaCl libs.