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

Fix fifo memory overflow #1789

Merged
merged 20 commits into from
Jan 7, 2023
Merged

Fix fifo memory overflow #1789

merged 20 commits into from
Jan 7, 2023

Conversation

hathach
Copy link
Owner

@hathach hathach commented Dec 12, 2022

Describe the PR

While trying to fix an issue reported in #1729 (detected using fuzzing #1716). I am able to fix a couple of issues with current tusb_fifo when using with overwritable mode

  • Prevent double overflowed to happen which make fifo count invalid. Though due to the cost of moving memory, the data at read index is not replaced by newer one.
  • fix _tu_fifo_remaining() return incorrect value which cause memory overflowed, detected by fuzzing
  • minimize tu_fifo size to 16
  • simplify and remove _tu_fifo_empty, _tu_fifo_full. Also correct full condition check
  • simplify _tu_fifo_count() and _tu_fifo_remaining(), also rename to _ff_count() and _ff_remaining()
  • update advance_pointer/backward_pointer to use depth instead of fifo, also rename to advance/backward_index
  • simplify _ff_correct_read_index()
  • remove _ff_overflowed() due to lack of use
  • using clang with ceedling unit-test with -fsanitize=address

Both of above cases can be reproduced/tested with 2 new unit tests. I also take the chance to add fifo implementation note (since I kind of forgot about it) to help future troubleshooting and rename absolute/relative naming to index/pointer and other minor changes. There is still room to improve fifo, thus submitted this as draft first for feedback.

ref reading: https://www.snellman.net/blog/archive/2016-12-13-ring-buffers/9

probably fix #1039 , and also related to adafruit/circuitpython#7041 since this could be the bug of cdc fifo instead of what we thought is Linux with echo mode.

- rename wr/rd absolute to index, and rel to pointer.
- fix crash with _tu_fifo_remaining()
- change get_relative_pointer() to idx2ptr() and merge with _ff_mod()
- handle/fix double overflowed with write()
- other minor clean upp
src/common/tusb_fifo.c Outdated Show resolved Hide resolved
src/common/tusb_fifo.c Outdated Show resolved Hide resolved
src/common/tusb_fifo.h Outdated Show resolved Hide resolved
src/common/tusb_fifo.h Outdated Show resolved Hide resolved
src/common/tusb_fifo.c Outdated Show resolved Hide resolved
src/common/tusb_fifo.h Outdated Show resolved Hide resolved
@PanRe
Copy link
Collaborator

PanRe commented Dec 15, 2022

Looks good! Nice that the buffer would be fuzzing-proof :)

Is there anything else to improve since you mentioned there would be room therefore!

@hathach
Copy link
Owner Author

hathach commented Dec 16, 2022

Looks good! Nice that the buffer would be fuzzing-proof :)

Is there anything else to improve since you mentioned there would be room therefore!

just update the PR with your feedback, thank you for reviewing. For room to improve, I feel that we could do better to simplify the current code, but it is not urgent. Like merging get_realitve_poiinter() and _ffmod() to idx2ptr(), mostly for readability and maintenance only. I will try to wrap this up soon enough.

@silvergasp
Copy link
Contributor

silvergasp commented Dec 23, 2022

Just a suggestion. It might be worth enabling -sanitizer=address which will detect any overflows in your unit test.

However that particular flag does require clang.

@hathach
Copy link
Owner Author

hathach commented Dec 26, 2022

Just a suggestion. It might be worth enabling -sanitizer=address which will detect any overflows in your unit test.

However that particular flag does require clang.

yeah, this is great idea. I will check to see we could switch to clang. Unit test like fuzzing run on host, so that wouldn't be an issue at all.

@silvergasp
Copy link
Contributor

Apologies the flag is -fsanitize=address docs here https://clang.llvm.org/docs/AddressSanitizer.html

@hathach hathach marked this pull request as ready for review January 6, 2023 05:12
@hathach
Copy link
Owner Author

hathach commented Jan 7, 2023

Apologies the flag is -fsanitize=address docs here https://clang.llvm.org/docs/AddressSanitizer.html

unit-test is now configured to run with clang and -fsanitize=address e885ced , thanks for your suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CDC device receives garbage if it transmits first
3 participants