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

Panic on control buffer overflow #95

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

haata
Copy link
Contributor

@haata haata commented Jun 5, 2022

  • When writing larger interface descriptors it's possible to overflow
    the buffer (even on LS/FS USB 2.0)
  • Cleanup get_descriptor and accept_writer to forward the error
  • Then unwrap() it inside get_descriptor
    • unwrap() will panic regardless of the logging mechanism and give a
      useful error message (e.g. defmt):
      ERROR panicked at 'called Result::unwrap() on an Err value: BufferOverflow'
    • We want to panic as continuing usually halts/suspends the USB
      device which will usually retry enumeration a few times until the
      host gives up
    • This is most common during initial enumeration, but it's also
      possible when requesting debug descriptors (e.g. lsusb)
  • Ideally we should return some sort of error that recommends enabling
    the control-buffer-256 feature but this isn't straightforward with the
    current design

- When writing larger interface descriptors it's possible to overflow
  the buffer (even on LS/FS USB 2.0)
- Cleanup get_descriptor and accept_writer to forward the error
- Then unwrap() it inside get_descriptor
  * unwrap() will panic regardless of the logging mechanism and give a
    useful error message (e.g. defmt):
    ERROR panicked at 'called `Result::unwrap()` on an `Err` value: BufferOverflow'
  * We want to panic as continuing usually halts/suspends the USB
    device which will usually retry enumeration a few times until the
    host gives up
  * This is most common during initial enumeration, but it's also
    possible when requesting debug descriptors (e.g. lsusb)
- Ideally we should return some sort of error that recommends enabling
  the control-buffer-256 feature but this isn't straightforward with the
  current design
Copy link
Member

@ryan-summers ryan-summers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix PR!

Can you please add a CHANGELOG entry for the fix?

It would also be helpful to file an issue to describe the defect that you're seeing. After reviewing the code, I think I understand, but it wasn't immediately clear to me from the start.

In any case, this one wasn't too hard to figure out, but it would be helpful for others looking for fixes and/or looking up similar errors in the future :)

}
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think unwrap()ing the result here is the best approach. This will inject fmt bloat into applications for people that may not be able to pay the memory overhead.

We should likely bubble the error up instead and let the user handle the Result

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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.

None yet

2 participants