Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AP_Proximity: prevent buffer overflow in LD06 driver #27059

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

peterbarker
Copy link
Contributor

We're using a value off the wire before it has been validated. That value is used to limit indexing into a buffer, and that buffer isn't big enough to handle all possible "bad" values that index could take on. Note that "read" here returns int16_t....

I've tried to simply make minimal changes in this driver to avoid the buffer overrun.

@tridge
Copy link
Contributor

tridge commented May 14, 2024

@peterbarker if this PR can't be easily tested then we could do a much simpler patch that just checks the array bounds before adding data. That would prefer the buffer overrun while being a minimal change

@rishabsingh3003
Copy link
Contributor

Unfortunately, I dont not have the sensor either so I cannot test. Thanks @peterbarker for getting this fixed!
@lvale I remember you had helped test this driver when we had this merged, would it be possible for you to test it?

@burgeruser
Copy link
Contributor

Hello, I have an LD06 laser radar that I purchased early this year. Unfortunately, its performance is subpar; it has a short range of only 12 meters, and the measurement noise is significant. As a result, the SLAM mapping using Cartographer algorithm does not yield satisfactory results.

Unfortunately, the model has become obsolete since it was quite niche. There are now many cheaper, lighter, and more accurate alternatives available in the market. In my testing with the LD06 on the developer version of Ardupilot4.5.0, directly connecting it to the flight controller for obstacle avoidance proved unreliable. The original 'AP_Proximity_LD06.cpp' file, which handles UART communication, heavily utilizes the STM32 microcontroller's RAM and lacks redundancy. In case of a physical UART cable error, it fails to parse data properly. This file tends to cause issues on STM32F4 platforms but runs smoothly on STM32H7 ones.

@lvale
Copy link
Member

lvale commented Jun 1, 2024

@peterbarker
sorry for coming late to the discussion. I have a pair of these working fine on Rover. What do you want to get tested ?

@rmackay9
Copy link
Contributor

rmackay9 commented Jun 2, 2024

Hi @lvale, thanks very much! We would just like to confirm that the driver hasn't been broken by these changes so we'd like it if you could just do a quick test to confirm that it still appears to work. If you ping me the name of the autopilot you're using we can create a firmware to load on the autopilot for the test.

We're using a value off the wire before it has been validated.  That value is used to limit indexing into a buffer, and that buffer isn't big enough to handle all possible "bad" values that index could take on.  Note that "read" here returns int16_t....
@peterbarker
Copy link
Contributor Author

I've now tested this on a real LD06 and there's no change in behaviour that I can discern.

Unplugged/replugged a few times live, no problems there, seems to resync just fine.

@peterbarker peterbarker merged commit cb69079 into ArduPilot:master Dec 19, 2024
99 checks passed
@peterbarker peterbarker deleted the pr/ld06-fix branch December 21, 2024 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Pending
Development

Successfully merging this pull request may close these issues.

6 participants