Validate packet burst bounds before writes - #202
Conversation
Sorry, something went wrong.
Greptile SummaryThis PR centralizes burst bounds validation across all four engines (DPDK, ibverbs, RDMA, socket) into a new src/burst_validation.h helper header, and adds a DAQIRI_ENABLE_BURST_VALIDATION CMake option (default ON) to allow the checks to be compiled out for trusted-input performance paths.
Confidence Score: 4/5Safe to merge after fixing the ibverbs set_packet_lengths guard removal; all other engine paths are correct. The ibverbs set_packet_lengths loop lost the seg >= MAX_NUM_SEGS guard that capped writes to burst->pkt_lens. When DAQIRI_ENABLE_BURST_VALIDATION=0 the replacement validation skips the lens.size() == num_segs check, so a caller passing more lengths than MAX_NUM_SEGS will overwrite memory beyond the array. Every other validation addition across the four engines is consistent and correct. src/engines/ibverbs/daqiri_ibverbs_engine.cpp — the set_packet_lengths write loop after the validation call needs the seg >= MAX_NUM_SEGS guard restored or the non-strict path of validate_packet_lengths needs to enforce the size limit. Important Files Changed
Reviews (2): Last reviewed commit: "#202 - Validate packet burst bounds befo..." | Re-trigger Greptile |
Sorry, something went wrong.
| } | ||
|
|
||
| void SocketEngine::free_all_packets(BurstParams* burst) { | ||
| if (burst == nullptr) { return; } | ||
| const int num_segs = std::clamp(burst->hdr.hdr.num_segs, 0, MAX_NUM_SEGS); | ||
| for (int seg = 0; seg < num_segs; ++seg) { | ||
| if (burst_validation::validate_segment_count( | ||
| burst, | ||
| burst_validation::header_limits(burst), | ||
| "SocketEngine::free_all_packets") != Status::SUCCESS) { | ||
| return; | ||
| } | ||
| for (int seg = 0; seg < burst->hdr.hdr.num_segs; ++seg) { | ||
| free_all_segment_packets(burst, seg); | ||
| } | ||
| } | ||
|
|
||
| void SocketEngine::free_packet_segment(BurstParams* burst, int seg, int pkt) { | ||
| if (burst == nullptr || seg < 0 || seg >= MAX_NUM_SEGS || burst->pkts[seg] == nullptr || pkt < 0 || | ||
| pkt >= static_cast<int>(burst->hdr.hdr.num_pkts)) { | ||
| if (burst_validation::validate_segment_packet_storage( | ||
| burst, | ||
| burst_validation::header_limits(burst), | ||
| seg, | ||
| pkt, | ||
| true, | ||
| false, | ||
| "SocketEngine::free_packet_segment") != Status::SUCCESS) { | ||
| return; | ||
| } | ||
| auto* data = reinterpret_cast<uint8_t*>(burst->pkts[seg][pkt]); |
There was a problem hiding this comment.
free_all_packets no longer frees segments when num_segs > MAX_NUM_SEGS
The previous implementation used std::clamp(burst->hdr.hdr.num_segs, 0, MAX_NUM_SEGS) to iterate up to MAX_NUM_SEGS segments even when the stored count was out of range. The replacement validate_segment_count returns early (and skips all free_all_segment_packets calls) whenever num_segs <= 0 || num_segs > MAX_NUM_SEGS. The inner free_all_segment_packets now also validates via validate_segment_index, so the double-rejection is consistent; but any burst that arrives with a corrupt-but-positive segment count above the limit will now have none of its packet buffers freed. For the socket engine, which heap-allocates each packet with new[], those pointers are permanently lost. The num_segs > MAX_NUM_SEGS case is unlikely in normal operation but the behaviour change is silent.
Sorry, something went wrong.
|
This PR appears to add a significant amount of checks that are per-packet or per-segment, which quickly has a large overhead for small packet sizes. Can you characterize the performance loss from having this, and if it's noticeable, put it behind an compilation flag? |
Sorry, something went wrong.
Centralize packet and segment bounds checks for public burst accessors and setters across DPDK, socket, RDMA, and ibverbs engines. Add DAQIRI_ENABLE_BURST_VALIDATION so strict burst validation remains on by default but can be disabled for trusted-input performance testing. Preserve socket cleanup behavior by clamping corrupt segment counts before freeing packet buffers. Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
Summary
Testing