search

femtoduino / ArduinoCore-atsamd21e18a

Arduino Core and Bootloader for the ATMEL SAM D21/R21 (E Variant) chips.
https://femto.io
29 stars 15 forks source link

FreeIMUUtils/CommunicationUtils.cpp should not use %s to buffer in non-ASCII output #21

Closed drogge closed 5 years ago

drogge commented 5 years ago

The toPrintableArr function used to buffer the IMU data in the 'b' command ends up using the %s printf format to try to add a byte to output buffer. This would cause the byte to be interpreted as a char * and some random amount of garbage will be copied to the buffer. Probably overflowing into adjacent memory.

zrecore commented 5 years ago

Ah, that explains the occasional random garbage then. :-)

On Fri, Sep 28, 2018, 6:27 PM Drew Rogge notifications@github.com wrote:

The toPrintableArr function used to buffer the IMU data in the 'b' command ends up using the %s printf format to try to add a byte to output buffer. This would cause the byte to be interpreted as a char * and some random amount of garbage will be copied to the buffer. Probably overflowing into adjacent memory.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/femtoduino/ArduinoCore-atsamd21e18a/issues/21, or mute the thread https://github.com/notifications/unsubscribe-auth/AAaLbHOLuAFnsIPCtPchaI-WKtpXB_e7ks5ufsx0gaJpZM4XAQll .

drogge commented 5 years ago

I'm not sure that the FemtoCore code is actually able to handle transmitting raw data. In the case of the 'b' command the buffer with the raw data is handed off to _reply which assumes the buffer contains ASCII chars. The calls to sprintf would probably yield unexpected results with raw data.

If you're going to be sending raw data then lots of functions are going to have to not depend on dealing with NULL terminated strings. This include send, broadcast, _networkingSendMessage* and maybe more.

You might want to think about not supporting raw data for now.

zrecore commented 5 years ago

Makes sense. The raw data function was originally a FreeIMU Serial command.

On Fri, Sep 28, 2018, 7:09 PM Drew Rogge notifications@github.com wrote:

I'm not sure that the FemtoCore code is actually able to handle transmitting raw data. In the case of the 'b' command the buffer with the raw data is handed off to _reply which assumes the buffer contains ASCII chars. The calls to sprintf would probably yield unexpected results with raw data.

If you're going to be sending raw data then lots of functions are going to have to not depend on dealing with NULL terminated strings. This include send, broadcast, _networkingSendMessage* and maybe more.

You might want to think about not supporting raw data for now.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/femtoduino/ArduinoCore-atsamd21e18a/issues/21#issuecomment-425607440, or mute the thread https://github.com/notifications/unsubscribe-auth/AAaLbBWIvqlhe3ZGw0-GIKp_eOOKyDgQks5uftZRgaJpZM4XAQll .