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 potential panic when reporting nom errors #658

Merged
merged 1 commit into from
May 3, 2024
Merged

Fix potential panic when reporting nom errors #658

merged 1 commit into from
May 3, 2024

Conversation

danielocfb
Copy link
Collaborator

We have gotten reports of a crash caused by numeric overflow when attempting to parse a Breakpad file. The reason lies in the error conversion functionality that nom provides. Specifically, the nom::error::convert_error() function expects errors with a context that derefs to an str slice. In order to fulfill that contract we convert everything to Cow.
However, the function also implicitly assumes that substrings belong to the same input slice. That is not the case, and cannot be easily fixed, because there is no sane way of "relocating" arbitrary byte slice after a lossy string conversion. The requirement of derefing into a str while also mapping to byte slices and are assumed to be subslices of each other just seems plain broken.
This change imports a copy of the convert_error() function and fixes up the broken bits.
The upstream issue touching on this problem is:
rust-bakery/nom#1696

We have gotten reports of a crash caused by numeric overflow when
attempting to parse a Breakpad file. The reason lies in the error
conversion functionality that nom provides. Specifically, the
nom::error::convert_error() function expects errors with a context that
derefs to an str slice. In order to fulfill that contract we convert
everything to Cow<str>.
However, the function also implicitly assumes that substrings belong to
the same input slice. That is not the case, and cannot be easily fixed,
because there is no sane way of "relocating" arbitrary byte slice after
a lossy string conversion. The requirement of derefing into a str while
*also* mapping to byte slices and are assumed to be subslices of each
other just seems plain broken.
This change imports a copy of the convert_error() function and fixes up
the broken bits.
The upstream issue touching on this problem is:
rust-bakery/nom#1696

Signed-off-by: Daniel Müller <[email protected]>
Copy link

codecov bot commented May 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.47%. Comparing base (f371817) to head (966a7cf).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #658      +/-   ##
==========================================
+ Coverage   94.39%   94.47%   +0.07%     
==========================================
  Files          47       47              
  Lines        9159     9291     +132     
==========================================
+ Hits         8646     8778     +132     
  Misses        513      513              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielocfb danielocfb merged commit 9b77f89 into libbpf:main May 3, 2024
34 checks passed
@danielocfb danielocfb deleted the topic/nom-err-conversion branch May 3, 2024 20:45
danielocfb pushed a commit that referenced this pull request May 3, 2024
Add a CHANGELOG entry for #658, which fixed potential numeric overflows
caused by broken usage of nom error conversion functionality.

Signed-off-by: Daniel Müller <[email protected]>
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.

2 participants