Fixed bug in composite_cdcacm_peek_ex

[arjanmels]

Fixed bug in composite_cdcacm_peek_ex.

rx_unread was calculated incorrectly in case offset>0. The too low rx_unread leads to len being set to a very high number in the next lines, causing the software to crash.

[stevstrong]

Can you please post an example sketch which shows the issue? The function you pointed out is only used to reset the controller for a following upload, so I am not sure that is wrong.

[arjanmels]

I encountered the problem when using the function in Marlin firmware, so extracting example is non-trivial. (With the reset detection it probably goes accidentally correct as the offset would be 0 if exactly 4 bytes are sent.)

However can be easily verified to be a problem by following the logic, with following example input: vcom_rx_tail=20 vcom_rx_head =25 offset=3 len=2

Calculations in routine: composite_cdcacm_peek_ex, now lead to:

uint32 tail = (vcom_rx_tail + offset) & CDC_SERIAL_RX_BUFFER_SIZE_MASK ; => tail=20+3=23; uint32 rx_unread = (vcom_rx_head-tail) & CDC_SERIAL_RX_BUFFER_SIZE_MASK; => rx_unread=25-23=2;

if (len + offset > rx_unread) { => 2+3 > 2 len = rx_unread - offset; => len = 2-3 = 0xffffffff }

BTW same error is present in (so I added this to this pull request): https://github.com/rogerclarkmelbourne/Arduino_STM32/blob/fedf4d4c3bfdc2377b20dc0d8e5339bb6c505fbe/STM32F1/cores/maple/libmaple/usb/stm32f1/usb_cdcacm.c#L511

[stevstrong]

I think the error is not using tail to calculate rx_unread, but the condition for the if. I think the correct fix would be:

    if (len > (rx_unread - offset) ) {
        len = rx_unread - offset;
    }

Please try this and let me know if it works.

[arjanmels]

I don't think your fix is correct: in case the if is true: len would be set to: rx_unread - offset = vcom_rx_head - (vcom_rx_tail + offset) - offset = vcom_rx_head - vcom_rx_tail - 2 * offset

Why not have the correct amount of unread bytes in rx_unread to start with?

[stevstrong]

Ok, I see the point. In this case the correct fix would be to keep the identical condition as used in usb_cdcacm_peek, namely:

    if (len > rx_unread) {
        len = rx_unread;
    }

[arjanmels]

I think that is indeed also correct. (It would not be my choice to fix it that way: rx_unread is a misnomer then. Why not stick with my original commit or otherwise rename rx_unread to max_len?)

[stevstrong]

The variable rx_unread was specially calculated separately in order to make the comparison more optimized, there is no need to subtract variables in the if structure, like in your version.

[stevstrong]

Thank you for pointing out this.

[arjanmels]

Thanks for fixing it. I hadn't considered the optimization. (I think a good c compiler would have taken care of it, but this way it will always be ok.)