stm32-rs / stm32-eth

Embedded Ethernet driver in Rust
Apache License 2.0
147 stars 47 forks source link

Ensure that packet_id overflow does not panic #87

Closed tdittr closed 1 year ago

tdittr commented 1 year ago

Description

While reading through the PTP implementation I noticed that next_packet_id just increments an internal counter with +=. This might panic once the packet_id reaches u32::MAX. I changed this to be a wrapping_add.

I briefly checked the code base for usages of PacketIds to see if at any place it is assumed that they are unique but did not find any such place. Maybe this should also be made explicit in the docs?

I'm happy about any feedback.

TODO

  1. [ ] Check all usages for if they assume that packet_ids are unique.
  2. [x] Update CHANGELOG.md
datdenkikniet commented 1 year ago

Thank you for the PR!

To answer your question: we only need as many unique PacketIDs as the amount of entries in the RX + TX rings (so that you can always identify each packet in either ring uniquely). The main reason that they're u32 is so that you can potentially use them for longer than that duration to do "other stuff", and because we need can only reserve bytes in increments of 4 in a descriptor anyways, so we have some extra space.

Especially when considering that, even at full speed and minimum frame size, it takes at like 200 hours to overflow this value, we should be good :D

In the future we can maybe improve/change this, but for now it should be fine :)

datdenkikniet commented 1 year ago

This is now released as v0.5.1

tdittr commented 1 year ago

Thanks for the quick review and release! :+1: