host/l2cap_coc: fix receiving of frames with size 1 #650
Conversation
nimble/host/src/ble_l2cap_coc.c
Outdated
| return rc; | ||
| /* Continuation LE frames might be of size 1, so we skip the pullup step | ||
| * for these */ | ||
| if (om_total >= 2) { |
There was a problem hiding this comment.
looks good but I'd use BLE_L2CAP_SDU_SIZE here (so that it match pullup call)
There was a problem hiding this comment.
of course, my bad -> fixed.
315323c to
ef67e95
Compare
|
btw, could you enlighten me what the But looking at the way the data is received in the l2cap layer, it seems to me that the this is always the case already when entering the |
nimble/host/src/ble_l2cap_coc.c
Outdated
| /* Continuation LE frames might be of size 1, so we skip the pullup step | ||
| * for these */ | ||
| if (om_total >= BLE_L2CAP_SDU_SIZE) { | ||
| rc = ble_hs_mbuf_pullup_base(om, BLE_L2CAP_SDU_SIZE); |
There was a problem hiding this comment.
As you noticed that should be done only for the very first packet.
Therefore instead of that if please move that code below the
/* First LE frame */
if (OS_MBUF_PKTLEN(rx->sdu) == 0) {
Btw this pullup might look paranoid but we want to do it before accessing om_data directly.
There was a problem hiding this comment.
Makes good sense to me, will asap.
ef67e95 to
74a6a2f
Compare
There was a problem hiding this comment.
@haukepetersen Looks good. Just please make sure to squash it before merging :) Thanks!
|
Moved the Doing this, I was wondering if something like the following might also be a (slightly) more efficient variant: uint8_t tmp[2];
os_mbuf_copydata(om, 0, 2, tmp);
sdu_len = get_le16(tmp);if I understand correctly, the |
L2CAP payloads might be split into multiple frames, of which only the first frame contains the 2-byte SDU length field and has thereby a guaranteed length > 2. Any subsequent frame may be of size 1 and are discarded without this fix. This commit fixes the ble_l2cap_coc_rx_fn() so that it only calls ble_hs_mbuf_pullup_base() if the incoming payload is >= 2 bytes.
74a6a2f to
d55dced
Compare
|
Anyhow, the optimization, if valid, might as well be applied in a follow up PR... Squashed, so we can proceed on this fix as is for now. |
|
@haukepetersen looks good, could you try merging this yourself (use 'rebase and merge' policy) ?:) |
I'm not sure if we should bother as in most of cases and in |
|
thanks @rymanluk ;-) |
In the current code, receiving L2CAP COC frames is broken for a number of specific packet sizes
(n * BLE_L2CAP_COC_MPS) - 1, e.g. for an L2CAPMPSvalue of250, all packets with size249,499,749, ... will not be received correctly.Reason (as I analyzed it so far): the call to
ble_hs_mbuf_pullup_base()in the beginning ofble_l2cap_coc_rx_fn()expects the incoming frame to have at least 2 bytes of payload. However, continuation frames might have a payload of 1 byte, leading to theble_hs_mbuf_pullup_base()call failing and thereby to error-exit the whole receive function. This way, the actually correct l2cap payload is never appended to the receive buffer and so the packet in the reassembly buffer is never completed...This PR proposes a crude fix to this problem, by only calling
ble_hs_mbuf_pullup_base()in case the received payload is >= 2. I verified the fix in my raw-l2cap-flooding setup -> with the fix I can successfully transfer those payloads that lead to frames with size 1.However, I don't quite comprehend what
ble_hs_mbuf_pullup_base()is actually good for, so I don't now if this proposed fix has any bad side effects. So please review careful and let me know about better approaches to fix this :-)